chungen0126 commented on code in PR #11302:
URL: https://github.com/apache/ozone/pull/11302#discussion_r4085692491
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/StreamBlockInputStream.java:
##########
@@ -585,8 +584,7 @@ public void
onNext(ContainerProtos.ContainerCommandResponseProto containerComman
try {
ByteBuffer data = readBlock.getData().asReadOnlyByteBuffer();
if (verifyChecksum) {
- ChecksumData checksumData =
ChecksumData.getFromProtoBuf(readBlock.getChecksumData());
- Checksum.verifyChecksum(data, checksumData, 0);
+ Checksum.validateChecksums(data, readBlock.getOffset(), 0,
readBlock.getChunkInfoListList());
Review Comment:
With the current approach, if a new client interacts with an older server,
reading data will completely fail because chunkInfoList will be empty, which
triggers an `OzoneChecksumException`. Even if checksum verification cannot be
performed in that case, I think we should still ensure the client can fall back
and read the data normally.
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/common/Checksum.java:
##########
@@ -441,37 +439,39 @@ public static void verifyChecksum(List<ByteBuffer>
bufferList, int startIndex, C
public static void validateChecksums(ByteBuffer data, long blockOffset, int
startIndex,
Review Comment:
Adding those check is nice, but could you clarify the motivation behind
refactoring this method? From my perspective, this change seems unnecessary.
Please leave this part as it is.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -2348,76 +2346,31 @@ private long readBlockImpl(ContainerCommandRequestProto
request, RandomAccessFil
"Requested offset " + readBlock.getOffset() + " is beyond the end of
block " + blockID + " with size "
+ blockData.getSize()));
}
- final List<ContainerProtos.ChunkInfo> chunkInfos = blockData.getChunks();
- final ChecksumType checksumType =
chunkInfos.get(0).getChecksumData().getType();
- int bytesPerChecksum = STREAMING_BYTES_PER_CHUNK;
- if (checksumType != ContainerProtos.ChecksumType.NONE) {
- bytesPerChecksum =
chunkInfos.get(0).getChecksumData().getBytesPerChecksum();
- }
-
- // TODO: Support client-side flag to toggle checksum verification.
- // If checksum is disabled, chunk offset adjustment can be skipped.
- int chunkIndex = ReadBlockComputation.searchChunk(readBlock.getOffset(),
chunkInfos);
- ReadBlockComputation readBlockComputation =
- new ReadBlockComputation(responseDataSize, bytesPerChecksum,
chunkInfos, chunkIndex);
- long adjustedOffset =
readBlockComputation.computeAdjustedOffset(readBlock.getOffset());
-
- long adjustLength = readBlockComputation.computeAdjustedLength(
- readBlock.getOffset(), readBlock.getLength(), adjustedOffset);
-
- ChecksumData checksumData = new ChecksumData(checksumType,
bytesPerChecksum);
- final ByteBuffer buffer = ByteBuffer.allocate(responseDataSize);
- blockFile.position(adjustedOffset);
- long totalDataLength = 0;
- int numResponses = 0;
- Preconditions.checkState(adjustLength <= blockData.getSize() -
adjustedOffset);
- LOG.debug("adjustedOffset {}, requiredLength {}, blockSize {}",
- adjustedOffset, adjustLength, blockData.getSize());
- for (boolean shouldRead = true; totalDataLength < adjustLength &&
shouldRead;) {
-
- int bufferLimit =
readBlockComputation.computeBufferLimit(adjustedOffset, adjustLength -
totalDataLength);
-
- buffer.limit(bufferLimit);
-
- shouldRead = blockFile.read(buffer);
+ if (readBlock.getOffset() < 0 || readBlock.getLength() < 0
+ || responseDataSize < 0 || responseDataSize >
OZONE_SCM_CHUNK_MAX_SIZE) {
+ return rejectReadBlock(blockFile, streamObserver,
Status.INVALID_ARGUMENT.withDescription(
+ "Invalid ReadBlock range or response size: " + readBlock));
+ }
Review Comment:
Is this validation needed on the server side? The client should already be
preventing these invalid requests from being sent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
