iemejia commented on code in PR #581:
URL: https://github.com/apache/avro-rs/pull/581#discussion_r3566715756
##########
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:
Fixed in 95a2652: `safe_collection_len` now uses
`checked_mul(item_size.max(1))` and returns `Details::IntegerOverflow` on
overflow, so a `usize::MAX` budget can no longer saturate past the check and
let `reserve` panic.
##########
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:
Fixed in 95a2652: the `zig_zag` test helper now delegates to the crate's
`util::zig_i64` encoder instead of re-implementing zigzag+varint, so the tests
can't diverge from the real encoder.
--
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]
