This is an automated email from the ASF dual-hosted git repository.
Jefffrey pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow-rs.git
The following commit(s) were added to refs/heads/main by this push:
new b8b59fb38d avro: bound VLQDecoder::long against overlong varints
(#10407)
b8b59fb38d is described below
commit b8b59fb38dcc5930d7a087bc3e17bd955a6c9f21
Author: Hill Patel <[email protected]>
AuthorDate: Sat Jul 25 08:45:53 2026 +0530
avro: bound VLQDecoder::long against overlong varints (#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.
---------
Co-authored-by: Claude Sonnet 5 <[email protected]>
Co-authored-by: Jeffrey Vo <[email protected]>
---
arrow-avro/src/reader/block.rs | 4 +--
arrow-avro/src/reader/header.rs | 8 +++---
arrow-avro/src/reader/vlq.rs | 62 +++++++++++++++++++++++++++++++++++++++--
3 files changed, 65 insertions(+), 9 deletions(-)
diff --git a/arrow-avro/src/reader/block.rs b/arrow-avro/src/reader/block.rs
index 8df76c4b15..3ec6e507ea 100644
--- a/arrow-avro/src/reader/block.rs
+++ b/arrow-avro/src/reader/block.rs
@@ -80,7 +80,7 @@ impl BlockDecoder {
while !buf.is_empty() {
match self.state {
BlockDecoderState::Count => {
- if let Some(c) = self.vlq_decoder.long(&mut buf) {
+ if let Some(c) = self.vlq_decoder.long(&mut buf)? {
self.in_progress.count = c.try_into().map_err(|_| {
AvroError::ParseError(format!(
"Block count cannot be negative, got {c}"
@@ -91,7 +91,7 @@ impl BlockDecoder {
}
}
BlockDecoderState::Size => {
- if let Some(c) = self.vlq_decoder.long(&mut buf) {
+ if let Some(c) = self.vlq_decoder.long(&mut buf)? {
self.bytes_remaining = c.try_into().map_err(|_| {
AvroError::ParseError(format!("Block size cannot
be negative, got {c}"))
})?;
diff --git a/arrow-avro/src/reader/header.rs b/arrow-avro/src/reader/header.rs
index c5593ba0ad..235166bb76 100644
--- a/arrow-avro/src/reader/header.rs
+++ b/arrow-avro/src/reader/header.rs
@@ -253,7 +253,7 @@ impl HeaderDecoder {
}
}
HeaderDecoderState::BlockCount => {
- if let Some(block_count) = self.vlq_decoder.long(&mut buf)
{
+ if let Some(block_count) = self.vlq_decoder.long(&mut
buf)? {
match block_count.try_into() {
Ok(0) => {
self.state = HeaderDecoderState::Sync;
@@ -271,7 +271,7 @@ impl HeaderDecoder {
}
}
HeaderDecoderState::BlockLen => {
- if self.vlq_decoder.long(&mut buf).is_some() {
+ if self.vlq_decoder.long(&mut buf)?.is_some() {
self.state = HeaderDecoderState::KeyLen
}
}
@@ -301,13 +301,13 @@ impl HeaderDecoder {
}
}
HeaderDecoderState::KeyLen => {
- if let Some(len) = self.vlq_decoder.long(&mut buf) {
+ if let Some(len) = self.vlq_decoder.long(&mut buf)? {
self.bytes_remaining = len as _;
self.state = HeaderDecoderState::Key;
}
}
HeaderDecoderState::ValueLen => {
- if let Some(len) = self.vlq_decoder.long(&mut buf) {
+ if let Some(len) = self.vlq_decoder.long(&mut buf)? {
self.bytes_remaining = len as _;
self.state = HeaderDecoderState::Value;
}
diff --git a/arrow-avro/src/reader/vlq.rs b/arrow-avro/src/reader/vlq.rs
index 26bf656159..cc028458ed 100644
--- a/arrow-avro/src/reader/vlq.rs
+++ b/arrow-avro/src/reader/vlq.rs
@@ -15,6 +15,8 @@
// specific language governing permissions and limitations
// under the License.
+use crate::errors::AvroError;
+
/// Decoder for zig-zag encoded variable length (VLW) integers
///
/// See also:
@@ -29,8 +31,18 @@ pub struct VLQDecoder {
impl VLQDecoder {
/// Decode a signed long from `buf`
- pub fn long(&mut self, buf: &mut &[u8]) -> Option<i64> {
+ ///
+ /// Returns `Err` if more than 10 continuation bytes accumulate across
calls without a
+ /// terminator, which would otherwise overflow the accumulated `i64`.
+ 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 {
+ self.in_progress = 0;
+ self.shift = 0;
+ return Err(AvroError::ParseError(
+ "Malformed Avro varint: too many continuation
bytes".to_string(),
+ ));
+ }
*buf = &buf[1..];
self.in_progress |= ((byte & 0x7F) as u64) << self.shift;
self.shift += 7;
@@ -38,10 +50,10 @@ impl VLQDecoder {
let val = self.in_progress;
self.in_progress = 0;
self.shift = 0;
- return Some((val >> 1) as i64 ^ -((val & 1) as i64));
+ return Ok(Some((val >> 1) as i64 ^ -((val & 1) as i64)));
}
}
- None
+ Ok(None)
}
}
@@ -163,4 +175,48 @@ mod tests {
varint_test(rand::random());
}
}
+
+ fn zigzag_encode(n: i64) -> u64 {
+ ((n << 1) ^ (n >> 63)) as u64
+ }
+
+ fn long_test(n: i64) {
+ let mut buf = [0_u8; 10];
+ let len = encode_var(zigzag_encode(n), &mut buf);
+ let mut decoder = VLQDecoder::default();
+ let mut slice = &buf[..len];
+ assert_eq!(decoder.long(&mut slice).unwrap(), Some(n));
+ assert!(slice.is_empty());
+ }
+
+ #[test]
+ fn test_long_roundtrip() {
+ long_test(0);
+ long_test(1);
+ long_test(-1);
+ long_test(4395932);
+ long_test(-4395932);
+ long_test(i64::MAX);
+ long_test(i64::MIN);
+
+ for _ in 0..1000 {
+ long_test(rand::random());
+ }
+ }
+
+ #[test]
+ fn test_long_overlong_varint_returns_error() {
+ // The Avro file magic followed by 13 continuation bytes and a
terminator, as
+ // reported against #10290: previously drove `self.shift` past 63 and
panicked
+ // with "attempt to shift left with overflow" inside `<< self.shift`.
+ let overlong = [0xFFu8; 13];
+ let mut decoder = VLQDecoder::default();
+ let mut buf = &overlong[..];
+ assert!(decoder.long(&mut buf).is_err());
+
+ // The decoder's state must be reset after a malformed varint, so a
subsequent
+ // call decodes a fresh varint cleanly rather than staying stuck
mid-decode.
+ let mut fresh = &[0x02u8][..]; // zigzag-encoded 1
+ assert_eq!(decoder.long(&mut fresh).unwrap(), Some(1));
+ }
}
