Reject malformed compressed Arrow IPC buffers before allocating for them - #113006
Reject malformed compressed Arrow IPC buffers before allocating for them#113006groeneai wants to merge 17 commits into
Conversation
Reading a corrupted ArrowStream file could reach the allocator with an enormous allocation size and fail with LOGICAL_ERROR, an internal-error code, from ordinary file input (an abort in debug and sanitizer builds). In a compressed Arrow IPC record batch each buffer carries an 8-byte little-endian uncompressed-length prefix read verbatim from the file. RecordBatchDecoder::prepareBuffers rejected a prefix below -1 and checked the running total for wraparound, but never compared the value against anything real, then sized the whole decompressed body in a single resize. In the file that surfaced this, nine of 42 prefixes are corrupt and sum to 11575148819126828176, which PODArray::resize passes to Allocator::checkSize. Each compressed payload is a complete codec frame, and both codecs report the uncompressed size the frame itself declares from its header alone, without decompressing. Reject, before allocating: a payload whose frame does not parse, a prefix disagreeing with the size its own frame declares, and an accumulated size the allocator would refuse. The frame's declared size is an optional field (upstream Arrow's LZ4 writer omits it, so the comparison is skipped when it is absent) and is file-controlled itself, so it is used only as a consistency check between two copies of the same number, never as an allocation bound. A prefix that agrees with its frame on a large size is deliberately not treated as corrupt: it remains MEMORY_LIMIT_EXCEEDED, the correct outcome for a size the query cannot afford. The aggregate bound is derived from the expression the allocator actually sees, because PODArray::resize rounds up to a power of two and so anything above 2^62 already reaches the 2^63 ceiling; it is evaluated at the alignment step so a zero-length buffer cannot bypass it. The frame-header parse uses its own codec context rather than the cached decompression context, because reading a header consumes it into the context. Uncompressed Arrow IPC is untouched. The cost on a compressed batch is one frame-header parse per buffer, which is O(buffers) and involves no decompression. Found by mutation fuzzing.
Reading a Parquet file with corrupt thrift-encoded metadata reported
Code: 1001 STD_EXCEPTION, a generic "some std::exception happened" that says
nothing about the input and cannot be classified programmatically.
deserializeThriftStruct caught only std::exception and rethrew through
Exception::CreateFromSTDTag, whose getCodeForSTDException has no entry for
thrift's exception types, so every malformed-metadata failure fell through to
STD_EXCEPTION.
Catch apache::thrift::TException, the base of both TProtocolException and
TTransportException, ahead of the existing generic clause. The base type is
needed rather than the two derived ones: TCompactProtocol's getTType throws the
base TException directly ("don't know what type") from readFieldBegin on a
corrupt field-type byte, which is a likely corrupt-file path and the one the new
test exercises. The generic clause is kept unchanged, so an unexpected
std::bad_alloc is still not relabelled as a data error.
deserializeThriftStruct is the single point through which all five Parquet
thrift structs are decoded on both the schema-inference and the read path, so
one clause covers them. This mirrors the existing orc::ParseError handling in
NativeORCBlockInputFormat.
Found by mutation fuzzing.
…describes it The per-buffer uncompressed-length check compared the buffer's 8-byte prefix against ZSTD_getFrameContentSize, which reads the first frame's header only, while the size it was compared against comes from ZSTD_decompressDCtx, which sums every concatenated frame. A valid payload of two or more frames was therefore rejected as INCORRECT_DATA, as was one beginning with a skippable frame (those report a content size of 0). Both shapes are read correctly by upstream Arrow, so this rejected interoperable data. Read the size only when ZSTD_findFrameCompressedSize proves the payload is exactly one frame, and treat a skippable frame's 0 as "declares nothing" - the design's existing path for a frame that carries no size, which skips the comparison. LZ4 needs no such scoping: its decompressor requires the whole payload to be consumed and otherwise reports trailing data, so a multi-frame LZ4 payload never reaches the comparison. The accumulated-body-size guard no longer restates PODArray and Allocator internals (the power-of-two growth policy, the 2^63 ceiling, PODArrayDetails). It bounds the running total an octave below that ceiling instead, which is what the guard needs and cannot drift out of step with the allocator. Cover the ZSTD branch in 04702: every case there was LZ4-derived, so deleting the whole ZSTD branch left the test green. Two corruption arms now fail without it, and two arms assert the multi-frame and skippable-prefixed payloads are still read. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ZSTD_getFrameContentSize returns 0 both for a skippable frame and for a genuinely empty one, so a previous revision mapped 0 onto the "declares nothing" path. That was wrong: ZSTD signals "declares nothing" separately, with ZSTD_CONTENTSIZE_UNKNOWN, so 0 is a truthful size once the payload has been proved to be exactly one frame. Mapping it to nullopt skipped the prefix/frame consistency check for a lone empty non-skippable frame, so a forged positive prefix below 2^62 was allocated for and only caught afterwards by the decompressor's own post-condition. Measured before this change: a 100 GiB prefix over a 9-byte empty frame reached a 128 GiB allocation attempt. The LZ4 branch keeps mapping contentSize == 0 to nullopt, which is correct there: lz4frame.h documents 0 as "unknown" and defines no separate sentinel. Adds two 04702 arms: a forged prefix over a lone empty frame must be rejected by the frame comparison, and an honest zero prefix must still read.
Internal second-model review (3 rounds, 0 open findings)Before publishing I reviewed this change cold against a second model, three rounds. Both Round 1 -
|
Pre-PR validation gate (click to expand)
Session id: cron:clickhouse-impl-slot-47:20260802-115500 |
|
cc @grantholly-clickhouse @tavplubix, could you review this? A compressed Arrow IPC record batch's per-buffer uncompressed-length prefix was read from the file and accumulated into a single |
alexey-milovidov
added
the
can be tested
|
Workflow [PR], commit [542db85] Summary: ✅
AI ReviewSummaryThis PR hardens Findings
Final VerdictChanges requested. LLVM Coverage Report
Changed lines: Changed C/C++ lines covered: 89/108 (82.41%) · Uncovered code |
clickhouse-gh
Bot
added
the
pr-bugfix
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
clang-tidy google-runtime-int rejects the bare `unsigned long long` local holding ZSTD_getFrameContentSize's result, which fails Build (arm_tidy) since -Werror is on. uint64_t (UInt64) has the same width, so both sentinel comparisons against ZSTD_CONTENTSIZE_ERROR and ZSTD_CONTENTSIZE_UNKNOWN keep their meaning; verified by static_assert on x86-64 and aarch64, and by running clang-tidy-21 with the repo config over the translation unit.
`declaredFrameContentSize` read the ZSTD size with `ZSTD_getFrameContentSize`, guarded by `ZSTD_findFrameCompressedSize` so the size was only used when the payload was exactly one frame. For the two interoperable shapes that are not (`frame1 || frame2`, and a skippable frame followed by a real one) it returned `nullopt`, which skipped the prefix/frame consistency check in `RecordBatchDecoder::prepareBuffers` entirely. A forged 8-byte prefix on such a payload therefore still reached `decompressed_body.resize`, and was only caught afterwards by the size post-condition of `ZSTD_decompressDCtx`. Measured on a 100 GiB forged prefix over both shapes: a 128 GiB allocation attempt, where the documented contract is a rejection before allocating. Use `ZSTD_findDecompressedSize` instead, which sums the declared size of every frame and skips skippable ones, so the value stays comparable to the prefix for these shapes too. It traverses the payload with the same loop as `ZSTD_decompressDCtx` (same skippable handling, same rejection of a partial tail), so it cannot refuse a payload that decompression would accept, and it reports `ZSTD_CONTENTSIZE_UNKNOWN` when any real frame omits its size, which keeps the existing "no comparable size" path. It also subsumes the previous one-frame gate, so the separate `ZSTD_findFrameCompressedSize` call goes away. Zero remains a truthful content size for a lone empty frame rather than a "declares nothing" marker. LZ4 is deliberately unchanged: `contentSize` is documented as `0 == unknown` with no separate sentinel, and the decompression path already rejects a frame with trailing bytes, so there is no LZ4 analogue of this shape. `ZSTD_findDecompressedSize` is in the static-linking-only section of `zstd.h`, hence the `ZSTD_STATIC_LINKING_ONLY` define; ClickHouse links the vendored `contrib/zstd` statically, and other in-tree users such as `contrib/librdkafka` already rely on that section. Extends the 04702 regression test with forged-prefix arms over both shapes. Verified they fail before this change and pass after it, with every other arm byte-identical.
The prefix/frame consistency check only ran when a frame recorded its uncompressed size. Upstream Arrow's LZ4 writer omits that optional field, so for pyarrow-written files the check was skipped and the file-controlled prefix was trusted straight into the allocation: a 100 GiB prefix over a 16 KB frame reached a 128 GiB allocation attempt, measured on the previous head and identically on unmodified master. An LZ4 frame's block headers bound its output without decompressing anything: LZ4F_decompress caps each block's output at the frame's maximum block size, so that cap summed over the blocks bounds the frame, and an uncompressed block expands to exactly its stored bytes. Walking those headers therefore yields a bound for every frame, whether or not one is recorded. The walk was checked against the decompression loop over 21 payload shapes: it never rejects a payload decompression accepts, and its bound is never below the real output. A recorded size is still preferred when it is not larger, because decompression enforces it exactly, which keeps a prefix differing from a recorded size rejected rather than merely one exceeding the bound. A recorded zero is not treated as exact: the API reports 0 both when the field is absent and when it reads zero, and a hand-forged frame can record zero over blocks that do produce data. This also closes the same gap for an empty LZ4 frame, which has no blocks and so is bounded at zero. Two 04702 arms changed meaning and were rewritten rather than left asserting something else: consistent_large pledged 8 GiB from a single block that can produce at most 4 MiB, which the bound now correctly rejects, so it pledges a size its block really can produce and the budget is what is small; aggregate_too_large is now rejected per buffer rather than on the accumulated total, and pledge_mismatch was added to keep the pledge-disagreement path distinct from the bound-exceeded one.
The previous commit downgraded a recorded content size larger than the block bound to a non-exact bound, which let a prefix equal to the bound through: it disagreed with the recorded size, but only the bound was compared, so the body was sized before the decompressor rejected the frame. Measured on that commit's binary, a frame pledging 1 GiB over one 4 MiB block with a 4 MiB prefix reached an 8 MiB allocation attempt; the head before it rejected the same input as INCORRECT_DATA, so this was a regression rather than a pre-existing gap. A recorded size above the block bound describes no possible frame and decompression rejects it, so reject it directly instead of choosing between two values that both contradict the file. Every other nonzero recorded size stays exact, as decompression enforces it. A recorded zero is still not exact, and now says so in one place rather than as part of the same condition: the API reports 0 both when the field is absent and when it records zero, and a frame can record zero over blocks that do produce data, so only the blocks can bound it.
ZSTD_findDecompressedSize reports UNKNOWN as soon as any frame of the payload omits its content size, and that was mapped to "no bound", leaving the file-controlled prefix trusted into the allocation for such payloads. A streaming writer produces exactly that shape, so this was the ZSTD counterpart of the LZ4 gap the previous commits closed. ZSTD_decompressBound covers it: for frames that declare their size it agrees with ZSTD_findDecompressedSize, and otherwise bounds each frame by its block count times min(128 KB, window size). Measured against ZSTD_decompressDCtx over eleven payload shapes, using the vendored copy the binary links: it is sound wherever decompression accepts (within 1.1x of the real output, so it still rejects a forged prefix), and returns an error in exactly the two cases decompression rejects, a truncated frame and a garbage tail. So it cannot reject a payload decompression would accept. The new test arm builds the frame by hand, since neither ClickHouse nor pyarrow omits the field, and asserts pyarrow still reads the honest counterpart.
The block-header walk stopped at the frame's end marker and returned a bound without looking at what followed it. Decompression accepts exactly one frame and no trailing bytes, so a payload carrying a truncated content checksum or a trailing byte still got a bound, was allocated for, and was only then rejected by the decompression call. That is the allocate-then-reject order the walk exists to avoid. The validated prototype this walk was ported from did check both, at walk.c:51-52; the shipped version dropped them. Restore them: consume the content checksum when the frame header says one is present, then require the walk to have consumed the whole payload. Two 04702 arms cover the two shapes, both with an honest prefix so only the suffix is wrong, and both matching the walk's own message rather than a bare INCORRECT_DATA - reverting the source alone reddens exactly those two arms and leaves every other line of the output identical. A third arm reads a frame with a whole content checksum, which must still be accepted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Build profile diff (arm_release)No arm_release build profile data for commit 542db85 - the build was skipped, reused from cache, or predates profile upload. |
| @@ -0,0 +1,533 @@ | |||
| #!/usr/bin/env bash | |||
| # Tags: no-fasttest | |||
There was a problem hiding this comment.
@groeneai is it possible to trim this test file, including the comments?
There was a problem hiding this comment.
Done in 31eb02a: 533 -> 424 lines, comments 87 -> 47.
Most of it was not prose. There were two near-duplicate flatbuffer walkers, which are now one; a
second buffer-repointing helper that was the first one with a hardcoded frame, deleted; and the
16-byte-lane path of the xxh32 helper, which no call site can reach, dropped and asserted. The
fixtures that differ only in a forged prefix are now built in one loop. I also trimmed 04703 in the
same push rather than wait to be asked twice.
No assertion was weakened, and I did not want to rely on a green run to claim that: every one of the
27 generated fixtures is byte-identical to before, and so are all 17 assertions, so the arms the
mutation probes established still redden.
Trimming did surface one real gap, fixed in 8da6174. Nothing pinned the
ZSTD_decompressBound rejection: zstd_bad_frame corrupts a frame that declares its size, so it
dies earlier in ZSTD_findDecompressedSize, and the unknown-size case used a valid frame, so it only
reached the numeric comparison. Deleting that rejection left the whole file green. The new arm is a
frame truncated mid-block, which declares no size and has no walkable block structure either, so
nothing can bound it. It reddens on its own: with the two-line check removed the file fails exactly
that arm and nothing else.
Merge the two flatbuffer walkers into one, drop a duplicated buffer-repointing helper and the unreachable 16-byte-lane path of the xxh32 helper, group the fixtures that differ only in a forged prefix, and cut the comments to what a reader cannot re-derive at that spot. Every generated fixture is byte-identical to before, as are all 17 assertions, so the arms established by the mutation probes still redden.
A frame truncated mid-block declares no content size and has no walkable block structure either, so nothing can bound it and it must be rejected outright. The existing arms reach only the numeric comparison, so removing that rejection left them all green; this one reddens.
lz4BlockOutputBound credited every compressed block a whole maxBlockSize regardless of how few bytes that block actually stored, so a valid frame that omits its content size could be bounded thousands of times above what it can emit. A frame of 256 literals-only blocks is 2315 bytes on disk and produces 1024 bytes, but was bounded at 256 * 4 MiB = 1 GiB. A forged 512 MiB buffer prefix then passed the comparison in RecordBatchDecoder and prepareBuffers sized decompressed_body to 512 MiB before the decompressor rejected the frame with "LZ4 produced 1024 bytes, expected 536870912" -- the allocate-then-reject order this bound exists to prevent. Cap each compressed block by the lesser of maxBlockSize and 255 times its stored size. The block format's densest encoding is a match length carried by 0xff extension bytes, each buying 255 output bytes for one input byte, so 255 is a hard per-block ceiling; the format notes the same ~250 limit. Measured against the vendored liblz4: 864 hand-built maximally dense blocks peak at 254.66x, and the compressor's own densest output reaches 254.83x. Validated over 76 frame shapes (4 blockSizeIDs x 8 payload sizes x pledged and unpledged, plus all-zeros, incompressible stored blocks and the attack shape) that the tightened bound never falls below what the decompressor really produces, so no valid frame is rejected. It cuts the attack shape's bound by 3290x, and the reproduction above is now rejected at 326400 bytes before anything is allocated. consistent_large forged a 4 MiB content size onto a single 16004-byte block, which the old loose bound permitted and the tightened one does not. It moves to 2 MiB, and an assertion now derives the frame's real bound from the writer's own output so the fixture cannot silently drift again.
| continue; | ||
| } | ||
| /// A compressed block cannot expand past what its own stored bytes can encode either. | ||
| bound += std::min(max_block_size, block_size * MAX_LZ4_BLOCK_EXPANSION); |
There was a problem hiding this comment.
block_size * 255 is still loose enough to preserve the original allocate-before-reject behavior for omitted-contentSize LZ4 frames.
04702 already contains the valid carrier for this: lz4_frame_with_tiny_compressed_blocks builds 256 literals-only compressed blocks, and the honest arm lz4_tiny_blocks.arrows proves that shape is accepted. Each block stores 5 bytes and emits exactly 4, so the real output is 1024 bytes, but this line bounds it at 256 * min(4 MiB, 5 * 255) = 326400. A forged prefix like 65536 is therefore larger than the true output but still below the bound, so RecordBatchDecoder::prepareBuffers will still size decompressed_body to 64 KiB and only later fail in Decompressor::decompress.
So the omitted-contentSize LZ4 path still does not satisfy the PR's "reject malformed compressed buffers before allocating for them" contract for the whole interval (1024, 326400]; the current test only exercises the much larger 512 MiB prefix, so it doesn't pin this gap. The bound needs to depend on the block's actual token structure, not only on block_size, or the pre-allocation guard remains too loose here.
There was a problem hiding this comment.
Confirmed against my own build, and fixed in d09849b.
The arithmetic holds exactly as stated. Rebuilding the carrier with 04702's own builders: the frame
is 2315 bytes, 256 blocks of 5 stored bytes, true output 1024, and the old bound was
256 * min(4 MiB, 5 * 255) = 326400, i.e. 319x the truth. The discriminator is the error message,
not just the exit status. Pre-fix, a 65536 prefix fails with LZ4 produced 1024 bytes, expected 65536, which comes from inside the decompressor after decompressed_body was already sized. Post-fix
the same file fails with declares 65536 uncompressed bytes but its 2315-byte codec frame declares 1024, from the pre-allocation comparison. A 326401 prefix was already rejected before the fix, so the
gap was exactly the interval (1024, 326400] you named.
I did not tighten 255 to a smaller multiple, because any multiple of a block's stored size is the
wrong shape of answer. An LZ4 block states its output in its own sequence tokens, so lz4BlockOutputBound
now walks them: per sequence, the literal length and the match length are both readable without
decompressing, and the walk visits each stored byte at most once. That makes the bound exact rather
than merely smaller.
Two properties I measured before trusting it, against the vendored liblz4 rather than the system copy:
- Soundness. Over 400780 blocks (real compressor output across 5 payload shapes at both compression
levels, plus 400000 random byte strings), the decoder accepted 7083. The walk was exact on all
7083, never under-bounded one, and never rejected one. Under-bounding is the dangerous direction
since it would make a valid frame unreadable, so this is the property that had to hold. - Cost. The walk runs before allocation on every compressed buffer, so it cannot be expensive: 4.3 us
against 168.4 us to decompress a 4 MiB block, 39x cheaper.
A block whose tokens do not describe a whole block keeps the old stored-size estimate instead of being
rejected there, so an unparseable block is still bounded and decompression remains what judges whether
it decodes.
On the test: you are right that pinning only the 512 MiB prefix could not detect a bound that is merely
loose. The new arm forges a prefix at 256 * 5 * 255, inside the old interval, so it fails on any bound
derived from a block's stored size. Verified by mutation: reverting only the bound line moved the build
ID and reddened exactly that one arm, with every other line byte-identical.
One correction to my earlier reply in the other thread. I wrote that this shape "is now rejected at
326400 bytes before anything is allocated". That was true only for prefixes above 326400; for the
interval you identified it was not, and the wording was wrong. It is accurate now that the bound is
1024.
|
The parquet changes are small, focused, and safe to make. I can't really speak with any authority on the ArrowStream changes |
|
Thanks, that is the useful half of an answer and I will not ask you about the ArrowStream side again. Acting on it: the Parquet change is independent of the Arrow IPC one. It is The Arrow IPC part stays here. It still has an open pre-allocation bound: for an LZ4 frame that omits its content size, the per-block bound is |
The per-block bound credited every compressed block 255 times its stored bytes, which is the format's densest possible encoding rather than what the block in front of us actually encodes. For a frame of many small blocks those differ by orders of magnitude: 256 literals-only blocks of 5 stored bytes each decode to 1024 bytes but were bounded at 256 * 5 * 255 = 326400, so any forged prefix in (1024, 326400] passed the comparison and prepareBuffers sized decompressed_body for it before the decompressor rejected it with "LZ4 produced 1024 bytes, expected N". That is the allocate-before-reject order this series exists to close, and 04702's existing arm pinned only a 512 MiB prefix, far above the bound, so it could not detect a bound that is merely loose. An LZ4 block states its output in its own sequence tokens, so walk them instead of estimating: each sequence's literal length and match length are readable without decompressing, and the walk visits each stored byte at most once. Measured against the vendored liblz4 over 400780 blocks (real compressor output across 5 payload shapes and both compression levels, plus 400000 random byte strings): of the 7083 the decoder accepted, the walk was exact on all 7083, never under-bounded one, and never rejected one, so it cannot make a valid frame unreadable. It is also 39x cheaper than decompressing a 4 MiB block (4.3 us against 168.4 us), which matters because this runs before allocation on every compressed buffer. A block whose tokens do not describe a whole block keeps the old stored-size estimate rather than being rejected here, leaving decompression to judge whether it decodes. The new 04702 arm forges a prefix inside the old interval, so it fails on any bound derived from a block's stored size rather than from its sequences.
| if (declared != ZSTD_CONTENTSIZE_UNKNOWN) | ||
| return FrameContentBound{declared, true}; | ||
| /// A frame may omit its size, and then only its block structure bounds it. | ||
| const UInt64 bound = ZSTD_decompressBound(src, size); |
There was a problem hiding this comment.
ZSTD_decompressBound is still too loose to make this a pre-allocation guard for size-omitting ZSTD frames.
A valid unknown-size frame made of many tiny raw blocks still takes this branch. One concrete carrier is 1000 raw 1-byte blocks: the frame is only 4006 bytes on disk and ZSTD_decompress produces 1000 bytes, but ZSTD_decompressBound returns 131072000 (1000 * 128 KiB). With a forged Arrow prefix of 1 MiB, RecordBatchDecoder::prepareBuffers accepts it because exact == false and 1048576 <= 131072000, then allocates 1 MiB in decompressed_body before Decompressor::decompress returns the real 1000 bytes and the post-condition throws.
So the original allocate-before-reject bug is still present for accepted "no content size" ZSTD payloads whenever the frame has many small blocks. The current zstd_no_declared_size arm does not catch this because it uses one raw block, where the bound happens to be exact. This path needs a tighter block walk (or rejection of these frames) rather than treating ZSTD_decompressBound as comparable to the real output size.
`std::min` deduces a single `_Tp` from both arguments, so they must have the same type rather than merely be convertible. `max_block_size` was `size_t` while `decoded.value_or(...)` is `UInt64`. On Linux x86_64 and aarch64 both spell `unsigned long`, so deduction succeeds and all 12 Linux builds compile; on aarch64-apple-darwin `UInt64` is `unsigned long long` and `size_t` is `unsigned long`, so deduction fails and the build stops with -Werror. Declare `max_block_size` as `UInt64` rather than annotating the call: `bound` is already `UInt64` and accumulates it, and the `LZ4F_max*` cases are small constants, so this makes the whole function consistent instead of widening at one call site. Both types are 64-bit and unsigned on LP64, and `block_size` is at most 4 MiB so the multiplication cannot wrap, leaving Linux behaviour unchanged. The public CI did not catch this because no darwin build ran on this branch. Verified by compiling the function under darwin's type model, where the reverted declaration reproduces the reported error and the fix compiles; the whole translation unit is clean under that model.
|
Fixed in 76f85fe.
I declared I swept the rest of the added code for the same shape rather than fixing only the reported line, since Reverting the declaration reproduces the reported error verbatim under that model, including the |
CI finish ledger - 76f85feEvery failure below has an owner: a fixing PR (mine or external), or a full-effort fix task
All 175 other check-runs on this head are Session id: cron:our-pr-ci-monitor:20260804-050000 |
|
Correction to the ledger above: it carries a |
|
The |
|
|
||
| /// What one LZ4 block's sequences decode to, by reading only its tokens and lengths, or nullopt when | ||
| /// they do not describe a whole block. Visits each stored byte at most once and never decompresses. | ||
| std::optional<UInt64> lz4BlockOutputLength(const char * src, size_t size) |
There was a problem hiding this comment.
@groeneai why do you need such a large change for the arrow fix?
There was a problem hiding this comment.
Fair, and it grew for a reason I do not think justifies its current size.
The fix for the reported defect is small: +66 source lines in the first commit (17eb3783d9). Everything after that is answers to five automated review rounds, each of which found the previous round's bound too loose. That is how +66 became +244 source. Reviewing the result as one diff is the wrong shape for it, so here is what I cut.
I reverted the LZ4 token walk (d09849b03d, the lz4BlockOutputLength function, -53 source / -54 test lines). That leaves the committed fallback min(max_block_size, block_size * 255), and 255 is LZ4's structural ceiling: a match costs one stored byte per 255 claimed output bytes, so the walk can only ever tighten the fallback, never loosen it. I measured what the walk actually buys on the shapes in the test: nothing on the carrier the last round was about (both give exactly 1 GiB, because a legitimate 4 MiB-of-one-byte frame and the forged one differ by 0.03% on disk and bound identically), and a tighter bound only on frames of many tiny blocks. That is a real but narrow gain, and it is not worth ~110 lines on a PR you are being asked to review. One test arm (lz4_tiny_blocks_prefix_under_stored_bound) exists only to pin the walk and goes with it.
I have also split out the Parquet half, as promised earlier in this PR: ThriftUtil.cpp +14 and 04703 +37, no file shared with the Arrow half. It is now #113311, so that self-contained error-code fix does not wait on this one.
That leaves the Arrow IPC change at +178 source: parse each buffer's codec frame header, and reject a prefix that disagrees with a size the frame pledges or exceeds what its blocks can produce. The test file is still large (+489) because each arm is a distinct malformed shape plus its non-corrupt twin, and the twins are what caught an earlier version rejecting files upstream pyarrow accepts. I would rather trim source than lose those, but say the word and I will cut the test to the shapes you care about.
Both removals are pushed. This PR is now +667 over 5 files, down from +824 over 8, and 04702 still passes unchanged. One disclosure: the two cuts landed in a single commit whose message describes only the walk revert, because .claude/CLAUDE.md rules out amending and I would rather not add a no-op commit to a PR you are asking to shrink.
I will not add anything further in response to an automated round without asking you first.
| /// LZ4F_getFrameInfo consumes the header into the context, so the context must be fresh (it is | ||
| /// discarded here) and cannot be one that a later decompression pass reuses. | ||
| LZ4F_dctx * dctx = nullptr; | ||
| if (LZ4F_isError(LZ4F_createDecompressionContext(&dctx, LZ4F_getVersion()))) |
There was a problem hiding this comment.
@groeneai compression is used elsewhere in clickhouse, should these checks be used in other places as well?
There was a problem hiding this comment.
Good question, and the answer differs per reader because the field widths differ.
I checked the three other places that size a decompression from a number in the file:
- Native compressed format (
CompressedReadBufferBase): the block header's decompressed size isUInt32(ICompressionCodec::readDecompressedBlockSize), so a forged value buys at most 4 GiB, and the codec verifies the actual output equals it afterwards (ICompressionCodec.cpp:125). Bounded by the type, and checked. - Parquet V3 pages (
Reader::decompressPageIfCompressed):uncompressed_page_sizeis thrifti32, so at most 2 GiB per page, and every codec path verifies the produced size (decompressLZ4Rawcomparesn != uncompressed_size, snappy comparesGetUncompressedLength). Same story. - Arrow IPC (this PR): the prefix is
Int64read verbatim from the file (RecordBatchDecoder.cpp:1535-1537), and the sizes are summed across all buffers of the batch and allocated in oneresizebefore any decompression runs. The file that surfaced this had nine corrupt prefixes summing to 1.16e19.
So the asymmetry is not that Arrow IPC forgot a check the others have. It is that the others cannot express a damaging number and validate after the fact, while Arrow IPC can and must validate before, because the allocation happens first. That is why I put the check here rather than in a shared helper: there is no second caller whose bug it would fix.
There was one real gap in that surface, and it is not a bound at all: deserializeThriftStruct in Parquet/ThriftUtil.cpp used the 1-argument TCompactProtocolT constructor, which leaves string_limit_ and container_limit_ at 0, and thrift reads 0 as "no limit" (contrib/thrift/lib/cpp/src/thrift/protocol/TCompactProtocol.h:77-87), while arrow's own reader passes both (contrib/arrow/cpp/src/parquet/thrift_internal.h:614-615). That is the mechanism behind the other case the same fuzzing run found: a small Parquet file making a bare DESCRIBE allocate tens of GB with max_memory_usage not stopping it. It is already fixed in #113212, open and separate from this PR.
Two residuals I should name rather than let you find: for ZSTD frames that omit their content size, the bound is ZSTD_decompressBound, which I measured at ~32700x for 1000 raw one-byte blocks; legitimate size-omitting output from a streaming writer reaches ~32500x, so the two are not separable by size alone and I did not tighten it. The LZ4 fallback has the same shape at ~255x. Both mean a malformed file can still cost a bounded multiple of its own size in memory, which the memory tracker charges and max_memory_usage stops. Neither reaches the allocator's ceiling, which is what this PR is about.
Reverts the per-block token walk. What it bought over the committed fallback min(max_block_size, block_size * 255) is narrow: 255 is LZ4's structural ceiling, because a match costs one stored byte per 255 claimed output bytes, so the walk can only ever tighten the fallback and never loosen it. On the shapes that matter it changes nothing at all: a frame of 4 MiB blocks of one repeated byte and a forged frame of the same size bound identically to 1 GiB, differing by 0.03% on disk, so no size-only bound separates them. The walk only tightened frames made of many tiny blocks, which is not worth its parsing surface on a review of this size. The test arm that existed solely to pin the walk goes with it. Every other arm is unchanged and still passes. MAX_LZ4_BLOCK_EXPANSION is UInt64 rather than size_t so std::min deduces one type from both of its arguments on targets where the two spellings differ, keeping the aarch64-apple-darwin build fixed.
CI finish ledger - 0174de3Every failure below has an owner: a fixing PR (mine or external), or a full-effort fix task
Not PR-caused. This diff rejects malformed compressed Arrow IPC buffers and touches only the Session id: cron:our-pr-ci-monitor:20260804-160000 |
| } | ||
| } | ||
|
|
||
| std::optional<FrameContentBound> frameContentBound(CompressionCodec codec, const char * src, size_t size) |
There was a problem hiding this comment.
@groeneai frameContentBound never returns nullopt, so I guess the std::optional can be removed?
There was a problem hiding this comment.
Right, and it has been dead for a while. Every one of the five returns constructs a FrameContentBound: the function either returns a bound or throws. The nullopt path existed until 902a1da, which gave a ZSTD frame that omits its content size a structural bound via ZSTD_decompressBound; that removed the last "no bound available" outcome and the std::optional was left behind. Worse, the caller's bound.has_value() && read as though a buffer could legitimately skip the check.
Returning by value in 542db85, and the caller now reads bound.exact ? out_len != bound.size : out_len > bound.size.
| UInt64 max_block_size = 0; | ||
| switch (info.blockSizeID) | ||
| { | ||
| case LZ4F_default: [[fallthrough]]; |
There was a problem hiding this comment.
@groeneai replace LZ4F_default with default to be safe and make it the last case.
There was a problem hiding this comment.
Done in 542db85, and LZ4F_default was in fact unreachable: LZ4F_decodeHeader rejects any blockSizeID below 4 (contrib/lz4/lib/lz4frame.c:1410, "4-7 only supported values for the time being"), so LZ4F_getFrameInfo can never report 0.
Measured rather than assumed. Forging each of the eight 3-bit BD values into a real frame header and brute-forcing the header checksum so every one is well-formed:
bsid_forged=0..3 -> REJECTED for all 256 header checksums
bsid_forged=4 -> ACCEPTED, info.blockSizeID=4
bsid_forged=5 -> ACCEPTED, info.blockSizeID=5
bsid_forged=6 -> ACCEPTED, info.blockSizeID=6
bsid_forged=7 -> ACCEPTED, info.blockSizeID=7
One trade-off worth naming: without a default:, -Wswitch would flag a new enumerator if upstream lz4 ever adds a block size, and a default: silences that. I kept the four real sizes enumerated and added default: last, bounding anything unexpected by the smallest block size, so the fallback is conservative rather than absent.
Test 04702 is byte-identical to its reference with both patches, and still fails 18 of its assertions on unmodified master.
|
@groeneai update the PR description, the Parquet fixes moved into another PR |
…switch Review feedback on ClickHouse#113006. frameContentBound returned std::optional but every one of its five returns constructs a FrameContentBound: it either returns a bound or throws. The nullopt path existed until 902a1da gave a ZSTD frame that omits its content size a structural bound via ZSTD_decompressBound, which removed the last "no bound available" outcome. The optional was left behind, and the caller's bound.has_value() read as though a buffer could skip the check. Returning by value makes the two outcomes the signature has. The block-size switch named LZ4F_default (value 0) as a case, but LZ4F_getFrameInfo can never report it: LZ4F_decodeHeader rejects any blockSizeID below 4 (lz4frame.c, "4-7 only supported values for the time being"). Forging each of the eight 3-bit values into a real frame header and brute-forcing the header checksum so every one is well-formed, 0-3 are rejected for all 256 checksums and 4-7 are accepted reporting themselves, so that case was dead. A default arm bounding an unexpected value by the smallest block size is what the enumeration cannot express. No behaviour change: test 04702 is byte-identical to its reference with these patches, and still fails 18 of its assertions on unmodified master. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Corrected. The changelog entry no longer mentions Parquet, the |
4 participants
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Fixed reading corrupted
ArrowStreamfiles. A malformed per-buffer uncompressed-length prefix in a compressed Arrow IPC record batch could reach the allocator as an enormous allocation size and produce aLOGICAL_ERROR; it is now rejected asINCORRECT_DATA.Description
A mutation-fuzzing finding reported by @ PedroTadim, from a file ClickHouse wrote itself with a few bytes corrupted, reached by a plain
SELECToverfile()with default settings. No public issue.In a compressed record batch, every Arrow buffer carries an 8-byte uncompressed-length prefix read straight from the file.
RecordBatchDecoder::prepareBufferschecked it for< -1and the running total for wraparound, but never against anything real, then sized the whole decompressed body in oneresize. In the reported file nine corrupt prefixes sum to ~1.16e19, soAllocator::checkSizerejected it asLOGICAL_ERROR: an internal-error code from ordinary file input, aborting in debug and sanitizer builds.Every prefix is now bounded by its own payload before anything is allocated for it. The codec frame may declare the uncompressed size, which is a pledge enforced exactly, so any difference is forged. That field is optional (upstream Arrow's LZ4 writer omits it), so when absent the frame's structure supplies the bound: for LZ4 by summing what each block header can expand to, for ZSTD via
ZSTD_decompressBoundover every frame. Either way there is a bound, and exceeding it is rejected without decompressing. Also rejected before allocating: an unparsable frame, a frame recording more than its blocks can produce, bytes after the frame, and an accumulated size the allocator would refuse.A prefix within what its payload can produce is deliberately not corrupt even when large: it stays
MEMORY_LIMIT_EXCEEDED, which a test asserts. Uncompressed Arrow IPC is untouched; the cost is one header walk per buffer, no decompression.One new test, failing on unmodified master, every assertion verified load-bearing. Well-formed files are asserted to still be read: multi-frame and skippable-prefixed ZSTD payloads, a frame omitting its size, and one recording 0 over blocks that produce data.
The Parquet thrift error-code half moved to #113311.