avro: bound an OCF block's object count and stop block bleed-through - #37972
Open
def- wants to merge 2 commits into
Open
avro: bound an OCF block's object count and stop block bleed-through#37972def- wants to merge 2 commits into
def- wants to merge 2 commits into
Conversation
def-
marked this pull request as ready for review
July 31, 2026 09:58
`Reader` reuses one buffer across object-container blocks, and `fill_buf` only ever grew it. A block shorter than its predecessor therefore left that predecessor's tail readable past its own payload, because everything downstream reads the buffer by its length: `read_next` slices `self.buf[self.buf_idx..]`, and `Codec::decompress` hands the whole buffer to the decompressor. A block whose declared object count outran its own bytes would then decode those stale bytes into values rather than running out of input. Truncate to the payload after reading it. `Vec::truncate` keeps the allocation, so the buffer is still reused. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A data block's object count is read straight off the wire and `safe_len` capped it only at `MAX_ALLOCATION_BYTES` — a sensible ceiling for a byte length, an enormous one for a count, and unrelated to the block it describes. A zero-width schema (an empty record, or a record of only `null` fields) encodes every object to no bytes, so a handful of wire bytes could claim hundreds of millions of objects and the reader would decode every one, allocating a `Vec` and per-field `String` each time. Measured: a ~30-byte file spends 47s on 100M objects, minutes at the old ceiling. Bound the count against the block that is supposed to hold those objects, deciding which bound applies the way `decode.rs` already does for an array block: the byte floor when the object schema has a proven positive one, otherwise the node-weighted `MAX_VALUE_NODES` cap, since no payload length can constrain a count of zero-width objects. Unlike an array block the two are alternatives, not both applied. An array materializes all of its elements at once so its cap is about retained memory, while a block's objects are yielded and dropped one at a time, so this cap is about work amplified out of a few bytes. Once the byte floor holds the work is linear in the file, and capping nodes as well would reject a legitimate large block of small records. The count is bounded after decompression, since the declared byte size is the compressed size. Found by the reader_decode cargo-fuzz target, which timed out within seconds of drawing block counts from the range `safe_len` accepts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Two fixes to the Avro object-container-file reader. Both are on the path that
decodes an OCF supplied by a source, so the bytes are untrusted.
A block could read the previous block's bytes.
Readerreuses one bufferacross blocks and
fill_bufonly ever grew it, so a block shorter than itspredecessor left that predecessor's tail readable past its own payload,
because everything downstream reads the buffer by its length (
read_nextslicesself.buf[self.buf_idx..], andCodec::decompresshands the whole buffer tothe decompressor). A block whose declared object count outran its own bytes
then decoded stale bytes into values instead of running out of input.
Truncating to the payload after reading fixes it.
Vec::truncatekeeps theallocation, so the buffer is still reused.
A block's object count was effectively unbounded. The count is read
straight off the wire and
safe_lencapped it only atMAX_ALLOCATION_BYTES, a sensible ceiling for a byte length, an enormous onefor a count, and unrelated to the block it describes. A zero-width schema
(an empty record, or a record of only
nullfields) encodes every object tono bytes, so a handful of wire bytes could claim hundreds of millions of
objects and the reader would decode every one, allocating a
Vecandper-field
Stringeach time. Measured: a ~30-byte file spends 47s on 100Mobjects, minutes at the old ceiling.
The count is now bounded against the block that is supposed to hold those
objects, choosing the bound the way
decode.rsalready does for an arrayblock: the byte floor when the object schema has a proven positive one,
otherwise the node-weighted
MAX_VALUE_NODEScap, since no payload lengthcan constrain a count of zero-width objects. Unlike an array block the two
are alternatives rather than both applied. An array materializes all its
elements at once so its cap is about retained memory, while a block's objects
are yielded and dropped one at a time, so this cap is about work amplified
out of a few bytes. Once the byte floor holds, the work is linear in the file,
and capping nodes as well would reject a legitimate large block of small
records. The bound is applied after decompression, since the declared byte
size is the compressed size.
Tests
Both are covered by the
reader_decodecargo-fuzz target, which timed outwithin seconds of drawing block counts from the range
safe_lenaccepted. Thattarget is sharpened in #37979, which should land after this.