avro: bound VLQDecoder::long against overlong varints - #10407
Merged
Jefffrey merged 3 commits intoJul 25, 2026
Conversation
A malformed or malicious Avro file with an unterminated run of varint
continuation bytes drove `self.shift` past 63, panicking inside
`<< self.shift` ("attempt to shift left with overflow") in debug builds.
In release builds without overflow-checks the shift is silently masked
instead, which is arguably worse: the decoder would keep running and
produce a wrong value rather than fail. Any service that reads
attacker-controlled Avro bytes through `ReaderBuilder::build` could hit
this.
`long` now bounds the varint to 10 bytes (mirroring the bound already
enforced by `read_varint_array`'s handling of its 10th byte a few lines
below) and returns `AvroError::ParseError` instead of panicking or
silently truncating. All 6 call sites in block.rs/header.rs already sit
inside functions returning `Result<_, AvroError>` and already propagate
sibling parse errors via `?`, so this is a purely internal, non-breaking
change - `vlq`, `block`, and `header` are private modules.
This directly resolves the open question from @alamb's review on the
stale, closed apache#9887 (apache#9887 (comment)):
whether malformed input should error rather than be silently absorbed.
The bound also fixes a gap in that PR's `checked_shl(..).unwrap_or(0)`
approach, which capped the accumulated value but placed no limit on how
many continuation bytes it would consume.
Fixes apache#10290
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
github-actions
Bot
added
arrow
etseidl
reviewed
Jul 23, 2026
| /// bound already enforced by [`read_varint_array`] for the stateless decoders below). | ||
| pub fn long(&mut self, buf: &mut &[u8]) -> Result<Option<i64>, AvroError> { | ||
| while let Some(byte) = buf.first().copied() { | ||
| if self.shift == 63 && byte >= 0x02 { |
Contributor
There was a problem hiding this comment.
Adding more branching to the loop makes me nervous...I'd like to see some benchmarks for this.
Perhaps something like
self.in_progress |=
((byte & 0x7F) as u64)
.checked_shl(self.shift)
.ok_or(AvroError::ParseError(
"Malformed Avro varint: too many continuation bytes".to_string(),
))?;would work?
Contributor
Author
There was a problem hiding this comment.
Good news on the benchmark front first: Jefffrey's bot run above shows every avro_reader case at ~1.00x ratio vs. base (both wall-clock and CPU), so the extra branch isn't measurable here.
On the checked_shl suggestion specifically — I don't think it's equivalent, and I'd like to explain why before swapping it in. checked_shl(shift) only returns None when the shift amount itself is >= 64 (i.e. an 11th+ continuation byte). It does not detect the case that actually matters at the boundary: on the 10th byte (shift == 63), a byte like 0x7F (0b111_1111) shifted left by 63 doesn't overflow the shift-amount check at all — it just silently drops the top 6 bits of that byte's payload via ordinary << semantics, and checked_shl returns Some(...) with a truncated, wrong value rather than None. So that version would still decode a malformed 10-byte varint successfully, just to the wrong i64 — which is the same "silently produce something instead of erroring" outcome @alamb's review on the stale #9887 was trying to move away from (discussion).
The byte >= 0x02 check at shift == 63 is what actually catches that: it mirrors the exact bound read_varint_array already enforces for its own 10th byte a few lines below ((b < 0x02).then_some(...)), so this isn't a new convention, just applying the file's existing one to the stateful decoder too.
Given the benchmarks come back flat, I'd lean toward keeping the explicit check for correctness — but happy to revisit if you'd rather accept the narrower checked_shl guarantee (catches unbounded/DoS-style inputs, just not this one specific truncation case) for less branching.
Contributor
|
run benchmarks decoder avro_reader |
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Jefffrey
reviewed
Jul 23, 2026
Per @Jefffrey's review: the decoder is stateful across calls, so "buf starts with more than 10 continuation bytes" was misleading - those bytes can accumulate across multiple `long` calls, not just within a single `buf` slice. Simplified the wording and referenced `i64`, the type actually being decoded, rather than the internal `u64` accumulator.
Jefffrey
approved these changes
Jul 23, 2026
Jefffrey
left a comment
Jefffrey
left a comment
Contributor
There was a problem hiding this comment.
we're probably fine in terms of performance; this decode path isnt exactly a hot path, its mainly for decoding lengths before blocks where blocks hold the actual data
Contributor
This was referenced Jul 30, 2026
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
4 participants
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.
Which issue does this PR close?
VLQDecoder::longdoesn't protect against invalid input #10290.Rationale for this change
A malformed or malicious Avro file with an unterminated run of varint continuation bytes drives
VLQDecoder::long'sself.shiftpast 63, panicking inside<< self.shift("attempt to shift left with overflow") in debug builds. In release builds without overflow-checks the shift is silently masked instead — arguably worse, since the decoder keeps running and can produce a wrong value rather than fail loudly. Any service that reads attacker-controlled Avro bytes throughReaderBuilder::buildcan hit this from the public API.There's a prior attempt at this in #9887 (closed by the stale bot, not rejected — no activity for 60 days). That PR used
checked_shl(self.shift).unwrap_or(0)to silently drop overflowing contribution bits, which stops the panic but places no bound on how many continuation bytes it will consume, and @alamb asked whether malformed input should return an error instead of being silently absorbed — that question was never answered before the PR went stale.What changes are included in this PR?
VLQDecoder::longnow bounds the varint to 10 bytes, mirroring the bound already enforced byread_varint_array's handling of its 10th byte a few lines below in the same file, and returnsAvroError::ParseErroron an overlong/malformed varint instead of panicking or silently truncating. The decoder's internal state is reset on error so a subsequent call decodes a fresh varint cleanly.long's signature fromOption<i64>toResult<Option<i64>, AvroError>, updating its 6 call sites inblock.rs/header.rs. All of them already sit inside functions returningResult<_, AvroError>and already propagate sibling parse errors via?, so each call site only needed a?added. This is a purely internal, non-breaking change —vlq,block, andheaderare all private (non-pub) modules withinarrow-avro, not part of the crate's public API.Are these changes tested?
test_long_overlong_varint_returns_errorreproducing the exact 19-byte input from the issue's cargo-fuzz repro (Avro magic + 130xFFcontinuation bytes), asserting the decoder now errors instead of panicking, and that it resets cleanly for the next call.test_long_roundtrip, a direct unit test forVLQDecoder::long's happy path (zig-zag decode), which had no dedicated test before this change — only the unsignedread_varint/read_varint_arraysiblings were covered bytest_varint.ReaderBuilder::buildAPI (ad-hoc integration test, not committed).cargo test -p arrow-avro --lib(402 passed),cargo clippy -p arrow-avro --all-targets -- -D warnings, andcargo fmt -p arrow-avro -- --checkall clean.Are there any user-facing changes?
No public API changes (
vlq/block/headerare private modules). Behaviorally, malformed/malicious Avro input that previously panicked (or silently produced garbage in a release build) now returns aAvroError::ParseErrorfrom the publicdecode/ReaderBuilder::buildpath, which is the intended, documented behavior for malformed input elsewhere in these same decoders.This PR was prepared with AI assistance (Claude Code). I reviewed the root cause in the source, traced every call site of
VLQDecoder::longto confirm the signature change is safe and non-breaking, verified the module-privacy claim before choosing this approach, and ran the fuzzer repro from the issue against the built fix myself before opening this PR.