ndimiduk commented on code in PR #6740:
URL: https://github.com/apache/hbase/pull/6740#discussion_r1983272164
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:
##########
@@ -1742,15 +1757,21 @@ protected HFileBlock
readBlockDataInternal(FSDataInputStream is, long offset,
onDiskSizeWithHeader = getOnDiskSizeWithHeader(headerBuf,
checksumSupport);
}
- // The common case is that onDiskSizeWithHeader was produced by a read
without checksum
- // validation, so give it a sanity check before trying to use it.
- if (!checkOnDiskSizeWithHeader(onDiskSizeWithHeader)) {
+ // The common case is that onDiskSizeWithHeader or headerBuf was
produced by a read without
+ // checksum validation, so give it a sanity check before trying to use
it.
+ // if checksumType on headerBuf validate fail,the headerBuf may corrupted
+ if (
+ !checkOnDiskSizeWithHeader(onDiskSizeWithHeader)
Review Comment:
Instead of doubling up the checks, why not have two separate if-statements
with two different error results? You could move the new
`checkCheckSumTypeOnHeaderBuf` up to before `onDiskSizeWithHeader` is populated.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:
##########
@@ -1587,6 +1587,21 @@ public HFileBlock readBlockData(long offset, long
onDiskSizeWithHeaderL, boolean
}
}
+ /**
+ * Check that checksumType on {@code headerBuf} read from a block header
seems reasonable,
+ * within the known value range.
+ * @return {@code true} if the headerBuf is safe to proceed, {@code false}
otherwise.
+ */
+ private boolean checkCheckSumTypeOnHeaderBuf(ByteBuff headerBuf) {
+ byte b = headerBuf.get(HFileBlock.Header.CHECKSUM_TYPE_INDEX);
+ for (ChecksumType t : ChecksumType.values()) {
Review Comment:
Oh, clever. Nice way of sniffing out corruption.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:
##########
@@ -1742,15 +1757,21 @@ protected HFileBlock
readBlockDataInternal(FSDataInputStream is, long offset,
onDiskSizeWithHeader = getOnDiskSizeWithHeader(headerBuf,
checksumSupport);
}
- // The common case is that onDiskSizeWithHeader was produced by a read
without checksum
- // validation, so give it a sanity check before trying to use it.
- if (!checkOnDiskSizeWithHeader(onDiskSizeWithHeader)) {
+ // The common case is that onDiskSizeWithHeader or headerBuf was
produced by a read without
+ // checksum validation, so give it a sanity check before trying to use
it.
+ // if checksumType on headerBuf validate fail,the headerBuf may corrupted
+ if (
+ !checkOnDiskSizeWithHeader(onDiskSizeWithHeader)
+ || (headerBuf != null && !checkCheckSumTypeOnHeaderBuf(headerBuf))
Review Comment:
nit: you could push the null check down into the new function.
--
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]
