github-actions[bot] commented on code in PR #63910:
URL: https://github.com/apache/doris/pull/63910#discussion_r3461175001
##########
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:
`snappy_block_decompressor_test.cpp` is being added to the disabled-test
`REMOVE_ITEM` list, so the new coverage is never compiled into `doris_be_test`.
`UT_FILES` is populated by the glob above, this line removes the new file
again, and the only later add-back path is `storage/index/ann`; there is no
separate target for this snappy test. This leaves the overflow regression
untested despite the new source file. Please remove this entry, or register the
test in an actual target, so the BE UT added for this fix runs in CI.
##########
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:
This still needs to validate the small block against
`remaining_decompressed_large_block_len`, not only against the physical output
buffer. With a malformed SNAPPYBLOCK stream whose large-block header says `1`
but whose Snappy payload expands to `4096`, a caller that has a 4096-byte
output buffer will pass this new check, `RawUncompress` will write all 4096
bytes, and then `remaining_decompressed_large_block_len -=
decompressed_small_block_len` underflows the `uint32_t` counter. The decoder
then reports a bogus need-more-input state instead of rejecting the
inconsistent length before writing. Please also reject
`decompressed_small_block_len > remaining_decompressed_large_block_len` before
calling `RawUncompress` and cover that case with an output buffer large enough
for the Snappy payload.
--
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]
