Copilot commented on code in PR #581:
URL: https://github.com/apache/avro-rs/pull/581#discussion_r3566631719
##########
avro/src/decode.rs:
##########
@@ -388,6 +405,85 @@ mod tests {
Ok(())
}
+ // A block count is decoded from a few bytes but drives
`items.reserve(len)`,
+ // allocating `len * size_of::<Value>()` bytes. A tiny payload declaring a
+ // huge count must be rejected rather than driving a multi-gigabyte
+ // allocation. Zero-byte elements (null) are the worst case: they consume
no
+ // input, so nothing else bounds the count.
+ fn zig_zag(n: i64) -> Vec<u8> {
+ let mut zz = ((n << 1) ^ (n >> 63)) as u64;
+ let mut out = Vec::new();
+ loop {
+ let mut b = (zz & 0x7f) as u8;
+ zz >>= 7;
+ if zz != 0 {
+ b |= 0x80;
+ }
+ out.push(b);
+ if zz == 0 {
+ break;
+ }
+ }
+ out
+ }
Review Comment:
The new `zig_zag` helper re-implements zigzag+varint encoding logic that
already exists in `crate::util::zig_i64`. Reusing the existing implementation
reduces duplication and the risk of tests diverging from the encoder used
elsewhere in the crate.
##########
avro/src/util.rs:
##########
@@ -184,6 +184,32 @@ pub(crate) fn safe_len(len: usize) -> AvroResult<usize> {
}
}
+/// Bound the cumulative number of elements a collection (array or map) may
hold.
+///
+/// [`safe_len`] guards byte-length allocations, but an array or map block is a
+/// count of elements, and reserving capacity for `n` elements allocates
+/// `n * item_size` bytes (`item_size` being the in-memory size of a decoded
+/// element). A malicious or truncated input can declare a huge block count in
a
+/// few bytes, so the count must be validated against the allocation budget,
+/// accounting for the per-element size, before reserving or decoding. This
also
+/// bounds zero-byte on-wire elements (e.g. `null`), which consume no input and
+/// so cannot be bounded by the bytes remaining, since the limit is on the
+/// decoded element count rather than the bytes read.
+pub(crate) fn safe_collection_len(total_items: usize, item_size: usize) ->
AvroResult<()> {
+ let max_bytes = max_allocation_bytes(DEFAULT_MAX_ALLOCATION_BYTES);
+ let desired = total_items.saturating_mul(item_size.max(1));
+
+ if desired <= max_bytes {
+ Ok(())
+ } else {
+ Err(Details::MemoryAllocation {
+ desired,
+ maximum: max_bytes,
+ }
+ .into())
+ }
+}
Review Comment:
`safe_collection_len` uses `saturating_mul`, which can hide multiplication
overflow. If `max_allocation_bytes` is configured to `usize::MAX`, an overflow
will saturate to `usize::MAX` and pass the `desired <= max_bytes` check,
potentially letting `reserve` hit a capacity overflow panic instead of
returning an `AvroResult` error. Prefer `checked_mul` and return
`Details::IntegerOverflow` on overflow.
--
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]
