Re: [PR] fix stale output capacity in Lz4BlockDecompressor decompress [doris]
sahvx655-wq commented on PR #64091: URL: https://github.com/apache/doris/pull/64091#issuecomment-4911952059 Pushed a clang-format fix for the new test file — that was the only thing left red on the formatter job, the change itself is unchanged. -- 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]
Re: [PR] fix stale output capacity in Lz4BlockDecompressor decompress [doris]
sahvx655-wq commented on code in PR #64091: URL: https://github.com/apache/doris/pull/64091#discussion_r3504078720 ## be/src/util/decompressor.cpp: ## @@ -552,10 +552,12 @@ Status Lz4BlockDecompressor::decompress(uint8_t* input, uint32_t input_len, break; } -// Decompress this block. +// Decompress this block. Capacity must track output_ptr: remaining_output_len +// is fixed per large block while output_ptr advances per small block. auto decompressed_small_block_len = LZ4_decompress_safe( reinterpret_cast(input_ptr), reinterpret_cast(output_ptr), -compressed_small_block_len, remaining_output_len); +compressed_small_block_len, +cast_set(output_max_len - (output_ptr - output))); Review Comment: Added be/test/util/lz4_block_decompressor_test.cpp, mirroring the Snappy one. The regression case builds an LZ4BLOCK stream with two small blocks inside a single large block and an output buffer sized to hold only the first (64 bytes for two 40-byte blocks). The declared large-block length is set to 64 so the outer need-more-output check passes and execution reaches the inner loop; the first small block advances output_ptr, and the second must be decompressed against the remaining capacity. With the stale full-block capacity that second call would write past output_max_len; with the fix it gets the real remaining space, LZ4_decompress_safe returns an error, and decompress returns InvalidArgument. The test asserts st.ok() is false there, so it goes red on the pre-fix code and green after. I also added a well-formed two-small-block round-trip so the happy path stays covered. Picked up automatically by the util/*.cpp glob in be/test/CMakeLists.txt, so no build wiring needed. -- 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]
Re: [PR] fix stale output capacity in Lz4BlockDecompressor decompress [doris]
github-actions[bot] commented on code in PR #64091: URL: https://github.com/apache/doris/pull/64091#discussion_r3464295451 ## be/src/util/decompressor.cpp: ## @@ -552,10 +552,12 @@ Status Lz4BlockDecompressor::decompress(uint8_t* input, uint32_t input_len, break; } -// Decompress this block. +// Decompress this block. Capacity must track output_ptr: remaining_output_len +// is fixed per large block while output_ptr advances per small block. auto decompressed_small_block_len = LZ4_decompress_safe( reinterpret_cast(input_ptr), reinterpret_cast(output_ptr), -compressed_small_block_len, remaining_output_len); +compressed_small_block_len, +cast_set(output_max_len - (output_ptr - output))); Review Comment: This memory-safety fix needs an automated test for the exact boundary it changes. The current PR only records a manual guard-page validation, while existing LZ4 coverage exercises normal segment `LZ4`/`LZ4F` round trips or load parsing, not `TFileCompressType::LZ4BLOCK`/`Lz4BlockDecompressor` with one Hadoop large block split into multiple small blocks. Please add a focused BE unit test that builds such a stream where the first small block advances `output_ptr` and the second would exceed the remaining output capacity, then assert the decoder returns an error without writing past the buffer. The related Snappy bounds fix added `be/test/util/snappy_block_decompressor_test.cpp`; this LZ4 fix should have analogous CI coverage. -- 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]
Re: [PR] fix stale output capacity in Lz4BlockDecompressor decompress [doris]
morningman commented on PR #64091: URL: https://github.com/apache/doris/pull/64091#issuecomment-4785497976 /review -- 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]
Re: [PR] fix stale output capacity in Lz4BlockDecompressor decompress [doris]
sahvx655-wq commented on PR #64091: URL: https://github.com/apache/doris/pull/64091#issuecomment-4659305565 Filled the description in against the template. To recap how I got here: reading the lz4 block path I noticed remaining_output_len is computed once per large block at the top of the outer loop, then passed straight to LZ4_decompress_safe as dstCapacity for every small block, while output_ptr keeps advancing per small block. So from the second small block on the decompressor is handed the full large-block capacity at an already-advanced pointer. Root cause is that stale capacity. A crafted lz4block stream (e.g. via a CSV load) can drive the inner loop to write past the line reader's output buffer, a heap out-of-bounds write; left unfixed it's a remotely-triggerable memory corruption on the load path. The one-line fix measures the real remaining space from output_ptr (output_max_len - (output_ptr - output)), so a block that would not fit now returns InvalidArgument and well-formed streams that already fit are untouched. -- 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]
Re: [PR] fix stale output capacity in Lz4BlockDecompressor decompress [doris]
hello-stephen commented on PR #64091: URL: https://github.com/apache/doris/pull/64091#issuecomment-4615130581 Thank you for your contribution to Apache Doris. Don't know what should be done next? See [How to process your PR](https://cwiki.apache.org/confluence/display/DORIS/How+to+process+your+PR). Please clearly describe your PR: 1. What problem was fixed (it's best to include specific error reporting information). How it was fixed. 2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be. 3. What features were added. Why was this function added? 4. Which code was refactored and why was this part of the code refactored? 5. Which functions were optimized and what is the difference before and after the optimization? -- 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]
[PR] fix stale output capacity in Lz4BlockDecompressor decompress [doris]
sahvx655-wq opened a new pull request, #64091: URL: https://github.com/apache/doris/pull/64091 reading the lz4 block path: remaining_output_len is computed once per large block and then handed to LZ4_decompress_safe as the destination capacity for every small block. output_ptr advances per small block but that capacity never moves with it, so from the second small block on the decompress is told it has the full large-block space starting at an already advanced pointer. a crafted lz4block stream from a csv load then writes past the line reader output buffer. capacity now tracks output_ptr. -- 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]
