From 6fb34c87b0b282e61075d7526bff6ffa803ada7b Mon Sep 17 00:00:00 2001 From: Juan Mantica Date: Thu, 27 Aug 2026 14:51:52 -0400 Subject: [PATCH 1/2] 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; From 4e502bd60838035abc5d6101b2304b557c03d1c4 Mon Sep 17 00:00:00 2001 From: Juan Mantica Date: Thu, 3 Sep 2026 14:36:39 -0400 Subject: [PATCH 2/2] fix(value): stop preallocating array capacity before range validation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `insert`'s array branch (`crud/insert.rs`) speculatively called `Vec::with_capacity` for the clamped index before recursing into `insert_value`, which is where the actual `MAX_ARRAY_INDEX` range check lives (`crud/mod.rs`). That defeated the point of the cap in two ways: - Every out-of-range indexed write (e.g. `arr[2000000] = 1`, which `insert_value` ultimately rejects and returns `None` for) still paid for a ~42 MB `Vec::with_capacity(MAX_ARRAY_INDEX + 1)` allocation first. A caller hitting this repeatedly (e.g. `for_each` over attacker-controlled data doing an out-of-range indexed write) still causes sustained large allocations even though every write is rejected. - For accepted negative-index writes, the preallocation was wasted even when in range: `insert_value`'s negative-index branch does its own independent `Self::with_capacity(len_required)` and replaces the array outright (`*self = extended`), discarding the array `insert` had just allocated. `insert_value` already validates the range before allocating anything, and already sizes its own allocations correctly for both the positive-index growth-loop path (amortized `push`, safe from an empty `Vec`) and the negative-index prepend path. So `insert` no longer needs to guess a capacity — starting from `Vec::new()` lets `insert_value` do the one allocation that's actually needed, sized correctly, only after the range check passes. Adds a test asserting a rejected out-of-range indexed write leaves the array unallocated (`capacity() == 0`), which is the property this cap is supposed to guarantee. Co-Authored-By: Claude Sonnet 5 --- src/value/value/crud/insert.rs | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/src/value/value/crud/insert.rs b/src/value/value/crud/insert.rs index 0a1e445ad..518d707d4 100644 --- a/src/value/value/crud/insert.rs +++ b/src/value/value/crud/insert.rs @@ -1,4 +1,4 @@ -use super::{ValueCollection, MAX_ARRAY_INDEX}; +use super::ValueCollection; use crate::path::BorrowedSegment; use crate::value::Value; use std::borrow::Borrow; @@ -26,16 +26,13 @@ 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 { - // 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_capacity) - } else { - // `unsigned_abs` rather than `-index`, which overflows on `isize::MIN`. - index.unsigned_abs().min(max_capacity) - }; - let mut array = Vec::with_capacity(capacity); + // No preallocation here: `insert_value` (for `Vec`) checks the index + // against `MAX_ARRAY_INDEX` before doing any allocation, so an out-of-range + // index is rejected with zero large allocation. For an in-range index it does + // its own correctly-sized allocation (a growth loop for positive indices, a + // `with_capacity` for negative ones) — preallocating here duplicated or wasted + // that work. + let mut array = Vec::new(); let prev_value = insert(&mut array, index, path_iter, insert_value); value.insert_value(key, Value::Array(array)); prev_value @@ -98,6 +95,17 @@ mod test { assert_eq!(value, Value::from(json!([]))); } + // OBE-10735: `insert` used to speculatively `Vec::with_capacity` the (clamped) index before + // `insert_value` had a chance to reject an out-of-range write, so a rejected huge index still + // committed a large (~42 MB) allocation. Assert the rejected array stays unallocated. + #[test] + fn test_insert_beyond_max_array_index_does_not_preallocate() { + let mut value = Value::Null; + assert_eq!(value.insert("[2000000]", 1), None); + let array = value.as_array_mut().expect("expected an array"); + assert_eq!(array.capacity(), 0); + } + #[test] fn test_insert_at_max_array_index_is_allowed() { let mut value = Value::Null;