Jefffrey commented on code in PR #10237:
URL: https://github.com/apache/arrow-rs/pull/10237#discussion_r3529025239
##########
arrow-avro/src/reader/record.rs:
##########
@@ -2286,35 +2286,58 @@ fn process_blockwise(
match block_count.cmp(&0) {
Ordering::Equal => break,
Ordering::Less => {
- let count = (-block_count) as usize;
+ // `unsigned_abs` avoids overflowing `-block_count` for
`i64::MIN` (#10235)
+ let count = block_count.unsigned_abs() as usize;
// A negative count is followed by a long of the size in bytes
- let size_in_bytes = buf.get_long()? as usize;
+ let raw_size = buf.get_long()?;
+ let size_in_bytes = usize::try_from(raw_size).map_err(|_| {
+ AvroError::ParseError(format!("Block size cannot be
negative, got {raw_size}"))
+ })?;
match negative_behavior {
NegativeBlockBehavior::ProcessItems => {
// Process items one-by-one after reading size
- for _ in 0..count {
- on_item(buf)?;
- }
+ total = process_block_items(buf, count, total, &mut
on_item)?;
}
NegativeBlockBehavior::SkipBySize => {
// Skip the entire block payload at once
let _ = buf.get_fixed(size_in_bytes)?;
+ total = total.saturating_add(count);
}
}
- total += count;
}
Ordering::Greater => {
let count = block_count as usize;
- for _ in 0..count {
- on_item(buf)?;
- }
- total += count;
+ total = process_block_items(buf, count, total, &mut on_item)?;
}
}
}
Ok(total)
}
+/// Decode `count` items, capping the running total at `i32::MAX` (the largest
index
+/// an Arrow list/map offset holds). Otherwise a crafted `i64::MAX` count of a
zero-byte
+/// item like `null` spins the loop forever (#10235); byte-consuming items
self-terminate
+/// on cursor exhaustion, so valid blocks (including `array<null>`) are
unaffected.
+#[inline]
+fn process_block_items(
+ buf: &mut AvroCursor,
+ count: usize,
+ total: usize,
+ on_item: &mut impl FnMut(&mut AvroCursor) -> Result<(), AvroError>,
+) -> Result<usize, AvroError> {
+ let new_total = total.checked_add(count).filter(|&t| t <= i32::MAX as
usize);
Review Comment:
It makes sense to have this cap them, in terms of other implementations
having a hard cap and the code only reading to a regular List.
##########
arrow-avro/src/reader/record.rs:
##########
@@ -3434,6 +3457,62 @@ mod tests {
assert_eq!(values.value(2), 3);
}
+ /// Zig-zag + unsigned-LEB128 encode, correct for all `i64` including
`MIN`/`MAX`
+ /// (`encode_avro_long` loops forever on those two values).
+ fn encode_avro_long_extreme(value: i64) -> Vec<u8> {
+ let mut n = ((value << 1) ^ (value >> 63)) as u64;
+ let mut out = Vec::new();
+ while n >= 0x80 {
+ out.push((n as u8) | 0x80);
+ n >>= 7;
+ }
+ out.push(n as u8);
+ out
+ }
+
+ // `array<null>` is the worst case: items consume no bytes, so an unbounded
+ // `block_count` spins the item loop without ever advancing the cursor
(#10235).
+ fn array_of_null_decoder() -> Decoder {
+ let list_dt =
avro_from_codec(Codec::List(Arc::new(avro_from_codec(Codec::Null))));
+ Decoder::try_new(&list_dt).unwrap()
+ }
+
+ #[test]
+ fn test_array_of_null_decodes() {
+ // A valid `array<null>` still decodes: zero-byte items make the count
exceed the
+ // bytes left, so the bound must not reject it (#10235 review).
Review Comment:
```suggestion
```
this comment was only relevant in the previous version
##########
arrow-avro/src/reader/record.rs:
##########
@@ -2286,35 +2286,58 @@ fn process_blockwise(
match block_count.cmp(&0) {
Ordering::Equal => break,
Ordering::Less => {
- let count = (-block_count) as usize;
+ // `unsigned_abs` avoids overflowing `-block_count` for
`i64::MIN` (#10235)
+ let count = block_count.unsigned_abs() as usize;
// A negative count is followed by a long of the size in bytes
- let size_in_bytes = buf.get_long()? as usize;
+ let raw_size = buf.get_long()?;
+ let size_in_bytes = usize::try_from(raw_size).map_err(|_| {
+ AvroError::ParseError(format!("Block size cannot be
negative, got {raw_size}"))
+ })?;
match negative_behavior {
NegativeBlockBehavior::ProcessItems => {
// Process items one-by-one after reading size
- for _ in 0..count {
- on_item(buf)?;
- }
+ total = process_block_items(buf, count, total, &mut
on_item)?;
}
NegativeBlockBehavior::SkipBySize => {
// Skip the entire block payload at once
let _ = buf.get_fixed(size_in_bytes)?;
+ total = total.saturating_add(count);
}
}
- total += count;
}
Ordering::Greater => {
let count = block_count as usize;
- for _ in 0..count {
- on_item(buf)?;
- }
- total += count;
+ total = process_block_items(buf, count, total, &mut on_item)?;
}
}
}
Ok(total)
}
+/// Decode `count` items, capping the running total at `i32::MAX` (the largest
index
+/// an Arrow list/map offset holds). Otherwise a crafted `i64::MAX` count of a
zero-byte
+/// item like `null` spins the loop forever (#10235); byte-consuming items
self-terminate
+/// on cursor exhaustion, so valid blocks (including `array<null>`) are
unaffected.
+#[inline]
+fn process_block_items(
+ buf: &mut AvroCursor,
+ count: usize,
+ total: usize,
+ on_item: &mut impl FnMut(&mut AvroCursor) -> Result<(), AvroError>,
+) -> Result<usize, AvroError> {
+ let new_total = total.checked_add(count).filter(|&t| t <= i32::MAX as
usize);
+ let Some(new_total) = new_total else {
+ return Err(AvroError::ParseError(format!(
+ "Array/map item count {count} exceeds the maximum {} addressable
by 32-bit offsets",
Review Comment:
we can probably omit the numbers in this error message; `count` here
technically doesnt exceed it, it just causes the total to exceed and `i32::MAX`
is just going to print as a big number that isnt really usable as an error
maybe something like
```
Capacity overflow when decoding array/map item blocks
```
(since this would probably be a rare error anyway, in regular usage)
--
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]
