From 6fb34c87b0b282e61075d7526bff6ffa803ada7b Mon Sep 17 00:00:00 2001 From: Juan Mantica Date: Thu, 27 Aug 2026 14:51:52 -0400 Subject: [PATCH] fix(value): [OBE-10735] consolidate and raise the array-index cap `MAX_ARRAY_INDEX` (32_768) and `MAX_ARRAY_CAPACITY` (32_769) were declared separately in `crud/mod.rs` and `crud/insert.rs`, so the enforcement bound and the preallocation bound could drift apart. Collapse them into one `pub(super)` constant and raise it to 2^20, per review feedback that 32_768 could reject legitimate large-array use. The cap stays because the amplification is real and cannot be optimised away: assigning to index N materialises N + 1 elements, and that null padding is observable VRL semantics (`length` sees it, `test_insert_array` asserts it), not merely a `with_capacity` hint. Dropping the preallocation would only trade one large allocation for amortised doubling. 2^20 bounds a single indexed write to ~42 MB at today's 40-byte `Value`; `test_value_size_is_pinned` fails if that size changes so the budget gets re-reviewed rather than silently drifting. Also replaces `(-index) as usize` with `index.unsigned_abs()` in the capacity calculation, which overflowed on `isize::MIN` (no positive `isize` counterpart). `insert_value` already used `unsigned_abs`; this was the remaining call site. Co-Authored-By: Claude Opus 5 (1M context) --- .../issues/obe_10735_array_index_cap.vrl | 14 ++++++ src/value/value/crud/insert.rs | 45 +++++++++++++++---- src/value/value/crud/mod.rs | 8 +++- 3 files changed, 56 insertions(+), 11 deletions(-) create mode 100644 lib/tests/tests/issues/obe_10735_array_index_cap.vrl diff --git a/lib/tests/tests/issues/obe_10735_array_index_cap.vrl b/lib/tests/tests/issues/obe_10735_array_index_cap.vrl new file mode 100644 index 000000000..3579b1e82 --- /dev/null +++ b/lib/tests/tests/issues/obe_10735_array_index_cap.vrl @@ -0,0 +1,14 @@ +# issue: OBE-10735 +# Assigning to a large array index pads the array with `Value::Null` up to that index. The padding +# is observable semantics (`length` sees it), so the write genuinely commits `index + 1` elements — +# an event-controlled index was enough to exhaust memory. Indices beyond +/-1048576 (2^20) are now +# dropped. 2^20 bounds one indexed write to ~42 MB at today's 40-byte `Value`. +# result: [0, 1048577] + +capped = [] +capped[2000000] = 1 + +allowed = [] +allowed[1048576] = 1 + +[length(capped), length(allowed)] diff --git a/src/value/value/crud/insert.rs b/src/value/value/crud/insert.rs index 23ff45113..0a1e445ad 100644 --- a/src/value/value/crud/insert.rs +++ b/src/value/value/crud/insert.rs @@ -1,4 +1,4 @@ -use super::ValueCollection; +use super::{ValueCollection, MAX_ARRAY_INDEX}; use crate::path::BorrowedSegment; use crate::value::Value; use std::borrow::Borrow; @@ -26,11 +26,14 @@ pub fn insert<'a, T: ValueCollection>( if let Some(Value::Array(array)) = value.get_mut_value(key.borrow()) { insert(array, index, path_iter, insert_value) } else { - const MAX_ARRAY_CAPACITY: usize = 32_769; + // Bounded by the same cap `insert_value` enforces, so an out-of-range index + // cannot reserve memory here before being rejected there. + let max_capacity = MAX_ARRAY_INDEX + 1; let capacity = if index >= 0 { - ((index as usize) + 1).min(MAX_ARRAY_CAPACITY) + ((index as usize) + 1).min(max_capacity) } else { - ((-index) as usize).min(MAX_ARRAY_CAPACITY) + // `unsigned_abs` rather than `-index`, which overflows on `isize::MIN`. + index.unsigned_abs().min(max_capacity) }; let mut array = Vec::with_capacity(capacity); let prev_value = insert(&mut array, index, path_iter, insert_value); @@ -84,24 +87,48 @@ mod test { #[test] fn test_insert_beyond_max_array_index_is_rejected() { let mut value = Value::Null; - assert_eq!(value.insert("[40000]", 1), None); + assert_eq!(value.insert("[1048577]", 1), None); assert_eq!(value, Value::from(json!([]))); } #[test] fn test_insert_beyond_max_negative_array_index_is_rejected() { let mut value = Value::Null; - assert_eq!(value.insert("[-40000]", 1), None); + assert_eq!(value.insert("[-1048577]", 1), None); assert_eq!(value, Value::from(json!([]))); } #[test] fn test_insert_at_max_array_index_is_allowed() { let mut value = Value::Null; - assert_eq!(value.insert("[32768]", 1), None); + assert_eq!(value.insert("[1048576]", 1), None); let array = value.as_array().expect("expected an array"); - assert_eq!(array.len(), 32769); - assert_eq!(array[32768], Value::Integer(1)); + assert_eq!(array.len(), 1_048_577); + assert_eq!(array[1_048_576], Value::Integer(1)); + } + + // OBE-10735: the capacity calculation negated the index with `(-index) as usize`, which + // overflows on `isize::MIN` (there is no positive `isize` counterpart). `unsigned_abs` is + // the total operation. + #[test] + fn test_insert_at_isize_min_does_not_panic() { + let mut value = Value::Null; + let path = vec![BorrowedSegment::Index(isize::MIN)].into_iter(); + assert_eq!(insert(&mut value, (), path, Value::Integer(1)), None); + assert_eq!(value, Value::from(json!([]))); + } + + // Drift detector, not a correctness assertion: the cap is justified in terms of the memory a + // single indexed write may commit (`MAX_ARRAY_INDEX + 1` elements of this size, ~42 MB today). + // If `Value` grows a variant, that budget changes and the cap deserves a fresh look. + #[test] + fn test_value_size_is_pinned() { + assert_eq!( + std::mem::size_of::(), + 40, + "size_of::() changed; re-check the MAX_ARRAY_INDEX memory budget \ + (cap x size = worst-case allocation for one indexed write)" + ); } #[test] diff --git a/src/value/value/crud/mod.rs b/src/value/value/crud/mod.rs index 4d257c721..a9fda7160 100644 --- a/src/value/value/crud/mod.rs +++ b/src/value/value/crud/mod.rs @@ -1,9 +1,13 @@ use crate::value::{KeyString, ObjectMap, Value}; use std::borrow::Borrow; -/// Largest array index `insert_value` will grow an array to, in either direction. +/// Largest array index an indexed write will grow an array to, in either direction. /// Prevents an event-controlled index (e.g. `.foo[40000000] = 1`) from exhausting memory. -const MAX_ARRAY_INDEX: usize = 32_768; +/// +/// Assigning to index `N` materialises `N + 1` elements — the null padding is observable VRL +/// semantics, not just a preallocation — so this cap is what bounds the memory a single write may +/// commit: ~42 MB at today's 40-byte `Value` (see `test_value_size_is_pinned`). +pub(super) const MAX_ARRAY_INDEX: usize = 1_048_576; mod get; mod get_mut;