STiFLeR7 opened a new pull request, #10407:
URL: https://github.com/apache/arrow-rs/pull/10407
# Which issue does this PR close?
- Closes #10290.
# Rationale for this change
A malformed or malicious Avro file with an unterminated run of varint
continuation bytes drives `VLQDecoder::long`'s `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
— 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
through `ReaderBuilder::build` can 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](https://github.com/apache/arrow-rs/pull/9887#discussion_r3189310679)
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::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 in the same file, and returns `AvroError::ParseError` on 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.
- This changes `long`'s signature from `Option<i64>` to `Result<Option<i64>,
AvroError>`, updating its 6 call sites in `block.rs`/`header.rs`. All of them
already sit inside functions returning `Result<_, 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`, and
`header` are all private (non-`pub`) modules within `arrow-avro`, not part of
the crate's public API.
# Are these changes tested?
- Added `test_long_overlong_varint_returns_error` reproducing the exact
19-byte input from the issue's cargo-fuzz repro (Avro magic + 13 `0xFF`
continuation bytes), asserting the decoder now errors instead of panicking, and
that it resets cleanly for the next call.
- Added `test_long_roundtrip`, a direct unit test for `VLQDecoder::long`'s
happy path (zig-zag decode), which had no dedicated test before this change —
only the unsigned `read_varint`/`read_varint_array` siblings were covered by
`test_varint`.
- Verified the exact fuzzer input from the issue no longer panics end-to-end
through the public `ReaderBuilder::build` API (ad-hoc integration test, not
committed).
- `cargo test -p arrow-avro --lib` (402 passed), `cargo clippy -p arrow-avro
--all-targets -- -D warnings`, and `cargo fmt -p arrow-avro -- --check` all
clean.
# Are there any user-facing changes?
No public API changes (`vlq`/`block`/`header` are private modules).
Behaviorally, malformed/malicious Avro input that previously panicked (or
silently produced garbage in a release build) now returns a
`AvroError::ParseError` from the public `decode`/`ReaderBuilder::build` path,
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::long` to 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
