masumi-ryugo opened a new pull request, #9887: URL: https://github.com/apache/arrow-rs/pull/9887
## What `VLQDecoder::long` accumulates a stateful zig-zag varint across multiple calls and shifts the next 7-bit payload by the running shift count. The shift wasn't bounded, so an overlong varint that keeps emitting continuation bytes drives `self.shift` past 63 and panics inside `<< self.shift`: ``` thread '<unnamed>' panicked at arrow-avro/src/reader/vlq.rs:35:33: attempt to shift left with overflow ``` ## Repro The cargo-fuzz `avro_reader` harness being prototyped for #5332 reproduces this in single-digit minutes from an empty corpus with a 19-byte input — the Avro file magic followed by 13 `0xFF` continuation bytes and a terminator: ``` 0000 4f 62 6a 01 ff ff ff ff ff ff ff ff ff ff ff ff |Obj.............| 0010 4f 06 b0 6a b3 |O..j.| ``` Reachable from `arrow_avro::reader::ReaderBuilder::new().build(buf_reader)` on attacker-controlled bytes — i.e. any service that consumes Avro through the public API will crash on this input. `read_varint`, `read_varint_array`, `read_varint_slow`, and `skip_varint*` in the same file already cap at 10 bytes / shift 63 and so are not affected; only the streaming `VLQDecoder::long` path was missing the bound. ## Fix Switch the shift to `checked_shl(self.shift).unwrap_or(0)` so contributions past bit 63 are silently dropped (matching what the eventual `u64` would have to do anyway), and `saturating_add(7)` so a truly malicious continuation run can't wrap `self.shift` past `u32::MAX` either. The terminator byte (high bit clear) still ends the varint, so a malformed varint that never terminates either runs out of input (returns `None`) or yields a truncated `u64` from the trailing terminator. ```rust self.in_progress |= ((byte & 0x7F) as u64).checked_shl(self.shift).unwrap_or(0); self.shift = self.shift.saturating_add(7); ``` ## Tests Adds `reader::vlq::tests::test_long_overlong_varint_does_not_panic` driving 12 continuation bytes plus a terminator (which would otherwise reach `shift = 84`), and asserts the subsequent call decodes a fresh varint cleanly so the decoder's state was reset on the malformed run. The existing `test_varint` continues to pass. ## Alternatives considered - **Track byte count and bail at 10**: matches what the stateless `read_varint_*` paths do. Would need an extra `count: u8` field on `VLQDecoder` and a way to surface "malformed" to the caller. Bigger API change for the same observable behavior under valid input. Happy to switch if you prefer it. - **Saturate to `u64::MAX`**: same panic-free property, but masks the malformed-input signal. The `checked_shl` form drops the contribution silently while still letting the terminator byte end the varint, which I think is the smallest-touch fix that keeps the public API unchanged. xref #5332 -- 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]
