Repository navigation
Conversation
A compressed block with an original size of 0 made CompressedInput::Decompress allocate Buffer(0), whose data() is null, and pass it as the LZ4/ZSTD destination; LZ4 does pointer arithmetic on it before it checks the capacity, which is undefined behaviour. A well-formed empty block also left CompressedInput::DoNext without data, so it reported end of stream, and Decompress ignored how many bytes LZ4/ZSTD actually produced, so a block that decoded short exposed a zero-filled tail as data. Decompress empty blocks into a zero-capacity stack target, keep reading blocks until one yields data, and reject output whose length differs from the declared original size.
CompressedInput::Decompress allocated a buffer of the declared compressed size before reading the block and zero-filled one of the declared decompressed size before decompressing it, so a few bytes from the server could make the client commit up to 1 GiB or 4 GiB. I now reject decompressed sizes above 1 GiB, the DBMS_MAX_DECOMPRESSED_SIZE limit the server applies to the same header, read the compressed bytes in steps that start at 4 MiB and reserve eightfold ahead, and keep the decompression buffer uninitialized and reused across blocks. The check that a block decompresses to exactly its declared size is what keeps stale bytes in the reused buffer from being returned.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Builds on #586. Only the last commit is new here.
CompressedInput::Decompressallocates a buffer of the declared compressed size before reading the block, and zero-fills one of the declared decompressed size before decompressing. A ~30-byte frame header from the server can therefore make the client commit up to 1 GiB, through the compressed size (already capped by #566), or 4 GiB through the decompressed size, which has no cap.Fix:
DBMS_MAX_DECOMPRESSED_SIZE, the limit the server applies to the same header inCompressedReadBufferBase.The exact-length check from #586 is what stops stale bytes in the reused buffer from being returned, which is why this is stacked on it.
Reproducer: an LZ4 frame header declaring an original size of 0xffffffff, or 2^31, followed by a short payload.
Tests:
ut/stream_ut.cppcovers declared sizes above the limit, and truncated payloads with large declared sizes that must throw without any single allocation above 8 MiB. These fail on Validate what CompressedInput decompresses #586's head.LargeBlocksRoundTripuses 9 MiB + 17 bytes of incompressible data, so the growth path runs.