sahvx655-wq commented on code in PR #63910:
URL: https://github.com/apache/doris/pull/63910#discussion_r3465697402
##########
be/src/util/decompressor.cpp:
##########
@@ -676,6 +676,16 @@ Status SnappyBlockDecompressor::decompress(uint8_t* input,
uint32_t input_len,
&decompressed_small_block_len))
{
return Status::InternalError("Failed to do snappy
decompress.");
}
+ // snappy::RawUncompress writes decompressed_small_block_len bytes
to output_ptr
+ // without a destination-capacity argument, so the header-declared
length must be
+ // checked against the remaining output buffer to avoid an
out-of-bounds write.
+ std::size_t available_output_len = output_max_len - (output_ptr -
output);
+ if (decompressed_small_block_len > available_output_len) {
Review Comment:
Good catch, you're right that the physical-buffer check on its own left the
underflow open. With the large-block header saying 1 but the snappy payload
expanding to 4096 into a 4096-byte buffer, the available_output_len check
passes, RawUncompress writes the whole payload, then
remaining_decompressed_large_block_len -= decompressed_small_block_len wraps
the uint32_t and the inner loop keeps spinning until it reports a bogus
need-more-input instead of rejecting the inconsistent stream.
Added a second guard that rejects decompressed_small_block_len >
remaining_decompressed_large_block_len before RawUncompress, so the stream is
refused before anything is written or the counter is decremented. Also added a
beut (SmallBlockExceedsLargeBlockLength) that sizes the output buffer to the
real payload so only this check can catch it.
##########
be/test/CMakeLists.txt:
##########
@@ -61,6 +61,7 @@ list(REMOVE_ITEM UT_FILES
${CMAKE_CURRENT_SOURCE_DIR}/storage/segment/segment_iterator_apply_index_expr_test.cpp
${CMAKE_CURRENT_SOURCE_DIR}/runtime/decimal_value_test.cpp
${CMAKE_CURRENT_SOURCE_DIR}/util/decompress_test.cpp
+ ${CMAKE_CURRENT_SOURCE_DIR}/util/snappy_block_decompressor_test.cpp
Review Comment:
You're right, that was my mistake. The file landed in the 'todo: need fix
those ut' REMOVE_ITEM block, so it got globbed into UT_FILES and then stripped
straight back out before doris_be_test was built, which meant the regression
never actually compiled or ran. Removed that line so the test is part of the
target and runs in CI now.
--
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]
