Copilot commented on code in PR #581:
URL: https://github.com/apache/avro-rs/pull/581#discussion_r3566724245
##########
avro/src/util.rs:
##########
@@ -184,6 +184,37 @@ 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
Review Comment:
The doc comment states that reserving for `n` elements allocates exactly `n
* item_size` bytes. In practice, standard collections may over-allocate (growth
strategy / internal table overhead), so this should be phrased as an estimate
or lower bound to avoid misleading readers about the strictness of the budget
check.
##########
avro/src/decode.rs:
##########
@@ -263,6 +273,13 @@ pub(crate) fn decode_internal<R: Read, S: Borrow<Schema>>(
break;
}
+ // See the array case above; a map entry additionally carries a
+ // String key, so bound by the size of a `(String, Value)`
pair.
Review Comment:
This comment implies the map reservation is well-modeled by
`size_of::<(String, Value)>()`, but `HashMap::reserve` also allocates hash
table metadata and spare capacity. Clarify that this is a lower-bound estimate
so readers don't assume the allocation budget is enforced exactly for maps.
##########
avro/src/decode.rs:
##########
@@ -241,6 +241,16 @@ pub(crate) fn decode_internal<R: Read, S: Borrow<Schema>>(
break;
}
+ // Bound the cumulative element count against the allocation
+ // budget before reserving: `reserve(len)` allocates
+ // `len * size_of::<Value>()` bytes, and a tiny block count
could
+ // otherwise drive a huge allocation. This also caps zero-byte
+ // on-wire elements (e.g. `null`), which consume no input.
+ let total = items
+ .len()
+ .checked_add(len)
+ .ok_or(Details::IntegerOverflow)?;
+ safe_collection_len(total, std::mem::size_of::<Value>())?;
items.reserve(len);
Review Comment:
`Vec::reserve` may grow capacity beyond `len` (amortized growth), so even if
`safe_collection_len(total, size_of::<Value>())` passes, this call can still
request an allocation larger than `max_allocation_bytes` (e.g., when decoding
multiple blocks and capacity doubles). Using `reserve_exact` keeps the
allocation request aligned with the checked element count.
--
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]
