STiFLeR7 commented on code in PR #10407:
URL: https://github.com/apache/arrow-rs/pull/10407#discussion_r3635754708
##########
arrow-avro/src/reader/vlq.rs:
##########
@@ -29,19 +31,30 @@ pub struct VLQDecoder {
impl VLQDecoder {
/// Decode a signed long from `buf`
- pub fn long(&mut self, buf: &mut &[u8]) -> Option<i64> {
+ ///
+ /// 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<Option<i64>, AvroError> {
while let Some(byte) = buf.first().copied() {
+ if self.shift == 63 && byte >= 0x02 {
Review 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](https://github.com/apache/arrow-rs/pull/9887#discussion_r3189310679)).
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.
--
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]
