From 8737345db07f7d836a2c7d2aae3bf11d89da5bce Mon Sep 17 00:00:00 2001 From: STiFLeR7 Date: Wed, 22 Jul 2026 12:23:44 +0530 Subject: [PATCH 1/2] avro: bound VLQDecoder::long against overlong varints 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 #9887 (https://github.com/apache/arrow-rs/pull/9887#discussion_r3189310679): 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 #10290 Co-Authored-By: Claude Sonnet 5 --- arrow-avro/src/reader/block.rs | 4 +-- arrow-avro/src/reader/header.rs | 8 ++--- arrow-avro/src/reader/vlq.rs | 63 +++++++++++++++++++++++++++++++-- 3 files changed, 66 insertions(+), 9 deletions(-) diff --git a/arrow-avro/src/reader/block.rs b/arrow-avro/src/reader/block.rs index 8df76c4b15f0..3ec6e507ea51 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 c5593ba0ad70..235166bb7658 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 26bf656159c0..5fb88348cfc1 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,19 @@ pub struct VLQDecoder { impl VLQDecoder { /// Decode a signed long from `buf` - pub fn long(&mut self, buf: &mut &[u8]) -> Option { + /// + /// Returns `Err` if `buf` starts with more than 10 continuation bytes without a + /// terminator, which would otherwise overflow the accumulated `u64` (matching the + /// bound already enforced by [`read_varint_array`] for the stateless decoders below). + pub fn long(&mut self, buf: &mut &[u8]) -> Result, 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 +51,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 +176,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)); + } } From b57884bd77c8e5aae7acb5356688c44ab5e8838f Mon Sep 17 00:00:00 2001 From: STiFLeR7 Date: Thu, 23 Jul 2026 10:58:22 +0530 Subject: [PATCH 2/2] avro: clarify long()'s docstring per review 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. --- arrow-avro/src/reader/vlq.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/arrow-avro/src/reader/vlq.rs b/arrow-avro/src/reader/vlq.rs index 5fb88348cfc1..cc028458edac 100644 --- a/arrow-avro/src/reader/vlq.rs +++ b/arrow-avro/src/reader/vlq.rs @@ -32,9 +32,8 @@ pub struct VLQDecoder { impl VLQDecoder { /// Decode a signed long from `buf` /// - /// Returns `Err` if `buf` starts with more than 10 continuation bytes without a - /// terminator, which would otherwise overflow the accumulated `u64` (matching the - /// bound already enforced by [`read_varint_array`] for the stateless decoders below). + /// 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, AvroError> { while let Some(byte) = buf.first().copied() { if self.shift == 63 && byte >= 0x02 {