This is an automated email from the ASF dual-hosted git repository.
Kriskras99 pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/avro-rs.git
The following commit(s) were added to refs/heads/main by this push:
new a88ad17 fix: bound array/map allocation by decoded element count
(#581)
a88ad17 is described below
commit a88ad17b0ebad5555b8942d4d2a086e5d4d848bb
Author: Ismaël Mejía <[email protected]>
AuthorDate: Tue Jul 14 12:28:03 2026 +0200
fix: bound array/map allocation by decoded element count (#581)
* fix: bound array/map allocation by decoded element count
An array or map block is encoded as an element count followed by that many
items. When decoding, `items.reserve(len)` allocates `len *
size_of::<Value>()`
bytes (Value is 56 bytes), but the count was only validated with `safe_len`,
which bounds it as if it were a byte count. So a tiny payload declaring a
huge
block count could drive a multi-gigabyte allocation far exceeding the
configured
`max_allocation_bytes` budget (up to ~28 GiB at the safe_len ceiling), and a
count of zero-byte on-wire elements (e.g. `null`) could grow the collection
unboundedly since such elements consume no input.
Add `util::safe_collection_len`, which bounds the cumulative element count
against the allocation budget accounting for the in-memory size of each
element,
and apply it in the array and map decode loops before reserving. This caps
both
the up-front reservation and the total number of decoded elements, covering
zero-byte and non-zero-byte element types alike. The limit stays
configurable
through `max_allocation_bytes`.
Assisted-by: GitHub Copilot:claude-opus-4.8
* test: cover i64::MIN array block count rejection
decode_seq_len already rejects a block count whose absolute value is
i64::MIN
via checked_neg (which returns None, yielding Details::IntegerOverflow), so
no
code change is needed. Add a regression test decoding the 10-byte i64::MIN
varint as a negative block count and asserting it is rejected, for parity
with
the negation-overflow coverage in the other Avro SDKs.
Assisted-by: GitHub Copilot:claude-opus-4.8
* refactor: address review on collection-length bound
- safe_collection_len: use checked_mul instead of saturating_mul so a
usize::MAX allocation budget cannot mask a multiplication overflow (which
would let reserve() hit a capacity-overflow panic); return
Details::IntegerOverflow on overflow.
- tests: build block-count varints via the crate's zig_i64 encoder rather
than
a duplicated zigzag/varint implementation.
Assisted-by: GitHub Copilot:claude-opus-4.8
* refactor: reserve_exact for arrays; clarify budget is a lower-bound
estimate
Addresses review:
- Array decoding uses reserve_exact so the reservation matches the checked
element count, rather than reserve's amortized (larger) capacity which
could
request more than max_allocation_bytes across multiple blocks.
- Clarify in the safe_collection_len doc and the map decode comment that the
per-element size is a lower-bound estimate: HashMap::reserve (and Vec
growth)
can over-allocate for metadata/spare capacity, so the budget is enforced
approximately for maps (there is no HashMap::reserve_exact).
Assisted-by: GitHub Copilot:claude-opus-4.8
* docs: note item_size is clamped to at least 1 in safe_collection_len
Document that safe_collection_len uses item_size.max(1), so a zero-sized
element
type is treated as one byte and the reported `desired` allocation is
total_items * max(item_size, 1). This matches the implementation and
clarifies
the error's `desired` interpretation.
Assisted-by: GitHub Copilot:claude-opus-4.8
* chore: simplify docs and code
---------
Co-authored-by: Kriskras99 <[email protected]>
---
avro/src/decode.rs | 103 +++++++++++++++++++++++++++++++++++++++++++++++++++--
avro/src/util.rs | 23 ++++++++++++
2 files changed, 124 insertions(+), 2 deletions(-)
diff --git a/avro/src/decode.rs b/avro/src/decode.rs
index c4eabf3..d78da71 100644
--- a/avro/src/decode.rs
+++ b/avro/src/decode.rs
@@ -24,7 +24,7 @@ use crate::{
error::Details,
schema::{DecimalSchema, EnumSchema, FixedSchema, Name, RecordSchema,
ResolvedSchema, Schema},
types::Value,
- util::{safe_len, zag_i32, zag_i64},
+ util::{safe_collection_len, safe_len, zag_i32, zag_i64},
};
use std::{
borrow::Borrow,
@@ -241,7 +241,15 @@ pub(crate) fn decode_internal<R: Read, S: Borrow<Schema>>(
break;
}
- items.reserve(len);
+ // Check that the Vec won't grow past the max allocation size
+ let total = items
+ .len()
+ .checked_add(len)
+ .ok_or(Details::IntegerOverflow)?;
+ safe_collection_len::<Value>(total)?;
+ // Use reserve_exact as reserve can allocate more than needed
defeating the purpose
+ // of the previous check
+ items.reserve_exact(len);
for _ in 0..len {
items.push(decode_internal(
&inner.items,
@@ -263,6 +271,15 @@ pub(crate) fn decode_internal<R: Read, S: Borrow<Schema>>(
break;
}
+ // Check that the HashMap won't grow past the max allocation
size. This is less
+ // precise than the Vec check above as HashMap allocates in
buckets and doesn't have
+ // a reserve_exact
+ let total = items
+ .len()
+ .checked_add(len)
+ .ok_or(Details::IntegerOverflow)?;
+ safe_collection_len::<(String, Value)>(total)?;
+
items.reserve(len);
for _ in 0..len {
match decode_internal(&Schema::String, names,
enclosing_namespace, reader)? {
@@ -388,6 +405,88 @@ mod tests {
Ok(())
}
+ // Create an array/map block with an object count of `n` and no items
+ fn create_block(n: i64) -> Vec<u8> {
+ // Reuse the crate's encoder so the tests cannot diverge from it.
+ let mut out = Vec::new();
+ crate::util::zig_i64(n, &mut out).unwrap();
+ out.push(0x00);
+ out
+ }
+
+ #[test]
+ fn test_decode_array_of_null_huge_count_is_rejected() -> TestResult {
+ // 200,000,000 nulls => ~11 GiB reserve, far above the default budget.
+ let payload = create_block(200_000_000);
+ let result = decode(
+ &Schema::array(Schema::Null).build(),
+ &mut payload.as_slice(),
+ );
+ assert!(
+ result.is_err(),
+ "a huge array<null> block count must be rejected, got {result:?}"
+ );
+
+ Ok(())
+ }
+
+ #[test]
+ fn test_decode_array_of_long_huge_count_is_rejected() -> TestResult {
+ // Non-zero-byte elements are also affected: the reserve happens before
+ // any element is read.
+ let payload = create_block(200_000_000);
+ let result = decode(
+ &Schema::array(Schema::Long).build(),
+ &mut payload.as_slice(),
+ );
+ assert!(
+ result.is_err(),
+ "a huge array<long> block count must be rejected, got {result:?}"
+ );
+
+ Ok(())
+ }
+
+ #[test]
+ fn test_decode_map_huge_count_is_rejected() -> TestResult {
+ let payload = create_block(200_000_000);
+ let result = decode(&Schema::map(Schema::Long).build(), &mut
payload.as_slice());
+ assert!(
+ result.is_err(),
+ "a huge map block count must be rejected, got {result:?}"
+ );
+
+ Ok(())
+ }
+
+ #[test]
+ fn test_decode_small_array_of_null_still_decodes() -> TestResult {
+ // A modest array of nulls within the budget must still decode.
+ let payload = create_block(3);
+ let result = decode(
+ &Schema::array(Schema::Null).build(),
+ &mut payload.as_slice(),
+ )?;
+ assert_eq!(Array(vec!(Value::Null, Value::Null, Value::Null)), result);
+
+ Ok(())
+ }
+
+ #[test]
+ fn test_decode_array_int64_min_block_count_is_rejected() -> TestResult {
+ // i64::MIN as a negative block count cannot be negated (checked_neg
+ // returns None); decoding must fail rather than wrap. i64::MIN zig-zag
+ // encodes as the 10-byte varint below, followed by a block byte-size.
+ let payload = create_block(i64::MIN);
+ let result = decode(&Schema::array(Schema::Int).build(), &mut
payload.as_slice());
+ assert!(
+ result.is_err(),
+ "an i64::MIN array block count must be rejected, got {result:?}"
+ );
+
+ Ok(())
+ }
+
#[test]
fn test_decode_map_without_size() -> TestResult {
let mut input: &[u8] = &[0x02, 0x08, 0x74, 0x65, 0x73, 0x74, 0x02,
0x00];
diff --git a/avro/src/util.rs b/avro/src/util.rs
index a0bcf29..4e4b106 100644
--- a/avro/src/util.rs
+++ b/avro/src/util.rs
@@ -186,6 +186,29 @@ pub(crate) fn safe_len(len: usize) -> AvroResult<usize> {
}
}
+/// Bound the cumulative number of elements a collection (array or map) may
hold.
+///
+/// This is equivalent to `safe_len(total_items * size_of::<T>)`
+pub(crate) fn safe_collection_len<T>(total_items: usize) -> AvroResult<()> {
+ let max_bytes = max_allocation_bytes(DEFAULT_MAX_ALLOCATION_BYTES);
+ // Use checked_mul (not saturating_mul): saturating to usize::MAX could
pass
+ // the check below when max_bytes is configured to usize::MAX, letting the
+ // subsequent reserve() hit a capacity-overflow panic instead of erroring.
+ let desired = total_items
+ .checked_mul(size_of::<T>())
+ .ok_or(Details::IntegerOverflow)?;
+
+ if desired <= max_bytes {
+ Ok(())
+ } else {
+ Err(Details::MemoryAllocation {
+ desired: Some(desired),
+ maximum: max_bytes,
+ }
+ .into())
+ }
+}
+
/// Set whether the serializer and deserializer should indicate to types that
the format is human-readable.
///
/// This function only changes the setting once. On subsequent calls the value
will stay the same
