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 0000000000..c36b87b5d8 --- /dev/null +++ b/lib/tests/tests/issues/obe_10735_array_index_cap.vrl @@ -0,0 +1,15 @@ +# issue: OBE-10735 +# Assigning to a large array index padded the array with `Value::Null` up to that index with no +# cap, so an event-controlled index could exhaust memory. Indices beyond ±1048576 (2^20) are now +# ignored. The limit was raised from the original ±32768 after review feedback that the smaller +# cap could reject legitimate large-array use cases; 2^20 still bounds a single indexed write's +# preallocation to ~42MB (Value is 40 bytes) instead of being unbounded. +# 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 499905081b..34dce4f874 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,10 +26,11 @@ 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 { + let max_capacity = MAX_ARRAY_INDEX + 1; let capacity = if index >= 0 { - (index as usize) + 1 + ((index as usize) + 1).min(max_capacity) } else { - (-index) as usize + index.unsigned_abs().min(max_capacity) }; let mut array = Vec::with_capacity(capacity); let prev_value = insert(&mut array, index, path_iter, insert_value); @@ -77,6 +78,42 @@ mod test { assert_eq!(value, expected); } + // OBE-10735: `insert_value` padded the array with `Value::Null` up to an arbitrary index, + // and `Vec::with_capacity(index + 1)` allocated for it up front — an event-controlled path + // index was enough to exhaust memory. + #[test] + fn test_insert_beyond_max_array_index_is_rejected() { + let mut value = Value::Null; + assert_eq!(value.insert("[2000000]", 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("[-2000000]", 1), None); + assert_eq!(value, Value::from(json!([]))); + } + + // `-index` on `isize::MIN` overflows; `unsigned_abs()` must be used instead so this doesn't + // panic on the exact class of input the array-index cap is meant to guard against. + #[test] + fn test_insert_at_isize_min_index_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!([]))); + } + + #[test] + fn test_insert_at_max_array_index_is_allowed() { + let mut value = Value::Null; + assert_eq!(value.insert("[1048576]", 1), None); + let array = value.as_array().expect("expected an array"); + assert_eq!(array.len(), 1_048_577); + assert_eq!(array[1_048_576], Value::Integer(1)); + } + #[test] fn test_insert_negative_index() { let mut value = Value::Null; diff --git a/src/value/value/crud/mod.rs b/src/value/value/crud/mod.rs index 9883adc515..9cc7dc82f0 100644 --- a/src/value/value/crud/mod.rs +++ b/src/value/value/crud/mod.rs @@ -1,6 +1,12 @@ use crate::value::{KeyString, ObjectMap, Value}; use std::borrow::Borrow; +/// Largest array index `insert_value` will grow an array to, in either direction. +/// Prevents an event-controlled index (e.g. `.foo[40000000] = 1`) from exhausting memory. +/// `Value` is 40 bytes, so this bounds a single indexed write's preallocation to ~42MB +/// (1_048_576 * 40 bytes) instead of being unbounded. See OBE-10735. +const MAX_ARRAY_INDEX: usize = 1_048_576; // 2^20 + mod get; mod get_mut; mod insert; @@ -104,6 +110,19 @@ impl ValueCollection for Vec { } fn insert_value(&mut self, key: isize, value: Value) -> Option { + let max_index = MAX_ARRAY_INDEX as isize; + if !(-max_index..=max_index).contains(&key) { + // TODO: VRL-side array-index assignment is currently infallible (see + // compiler::expression::assignment::Target::insert), so we can't surface this as a + // proper VRL runtime error without a larger change. Log it so it's at least + // observable instead of a silent no-op. + tracing::warn!( + index = key, + max_index = MAX_ARRAY_INDEX, + "array index assignment out of range, write dropped" + ); + return None; + } if key >= 0 { if self.len() <= (key as usize) { while self.len() <= (key as usize) {