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;