Repository navigation
fix(ipc): bound decompressed buffer size by the codec before allocating - #51
Merged
Merged
Conversation
DecompressBuffer allocated the uncompressed length from each buffer's 8-byte prefix before decompressing, so a 1 KiB stream could claim a multi-GiB or multi-TiB buffer. Reject a declared size the codec cannot produce from the compressed bytes (LZ4 frame: 255x, ZSTD: 32768x, since a block yields at most 128 KiB from at least 4 bytes), and reject negative sizes other than the -1 "uncompressed" marker. Buffers up to 64 MiB keep the one-shot decode. Larger ones decode through the codec's streaming decompressor into a buffer that starts at 64 MiB and doubles up to the declared size, so an overstated size costs only what the stream really yields. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
moshap-firebolt
approved these changes
Sep 28, 2026
| constexpr int64_t kMaxTrustedUncompressedSize = int64_t{64} << 20; | ||
|
|
||
| // Most output one input byte can produce: an LZ4 match length grows by 255 per | ||
| // extension byte, and a ZSTD block yields at most 128 KiB from at least 4 bytes. |
There was a problem hiding this comment.
This is an interesting approach - we probably can translate it to other cases - it's been a real struggle to validate uncompressed_length, because in principal it can be unbounded
| return 255; | ||
| case Compression::ZSTD: | ||
| return (int64_t{128} << 10) / 4; | ||
| default: |
There was a problem hiding this comment.
so other schemes are still vulnerable ?
Collaborator
Author
There was a problem hiding this comment.
afaik arrow IPC only supports these two, see e.g. https://arrow.apache.org/docs/cpp/api/ipc.html#_CPPv4N5arrow3ipc15IpcWriteOptions5codecE / https://arrow.apache.org/rust/arrow_ipc/compression/index.html
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.
DecompressBuffer allocated the uncompressed length from each buffer's 8-byte prefix before decompressing, so a 1 KiB stream could claim a multi-GiB or multi-TiB buffer.
Reject a declared size the codec cannot produce from the compressed bytes (LZ4 frame: 255x, ZSTD: 32768x, since a block yields at most 128 KiB from at least 4 bytes), and reject negative sizes other than the -1 "uncompressed" marker. Buffers up to 64 MiB keep the one-shot decode. Larger ones decode through the codec's streaming decompressor into a buffer that starts at 64 MiB and doubles up to the declared size, so an overstated size allocates only what the stream really yields.
Note
Medium Risk
Changes decompression allocation and validation on the IPC read path; behavior for malformed or adversarial metadata improves but legitimate edge cases near size limits need correct streaming and expansion math.
Overview
Hardens IPC buffer decompression so untrusted declared uncompressed lengths cannot force huge allocations before decode.
DecompressBuffernow rejects invalid negative lengths (other than-1), and uses codec-specific max expansion checks (LZ4 frame 255×, ZSTD ~32K×) to reject impossible declared sizes. Declarations above 64 MiB no longer get a single upfront allocation at the claimed size; they go through newStreamDecompressBuffer, which starts at 64 MiB, grows by doubling up to the declared cap, and errors if output exceeds the declared length or does not match it exactly.Reviewed by Cursor Bugbot for commit 153b538. Bugbot is set up for automated code reviews on this repo. Configure here.