From cddd76405f1e90de47b9c0093858841d3aaacc60 Mon Sep 17 00:00:00 2001 From: Harshvardhan Shrivastava Date: Mon, 10 Aug 2026 21:53:27 +0530 Subject: [PATCH] panic/OOM hardening for 6 tickets OBE-10736 (string repeat OOM) OBE-10737 (decompression bomb) OBE-10726 (Datadog mixed-bracket range panic) OBE-10721 (Kind::insert OOM on large negative index) OBE-10742 (parse_xml stack overflow on deep XML) --- src/compiler/expression/op.rs | 10 ++- src/compiler/value/arithmetic.rs | 73 ++++++++++++++++++- src/datadog/search/grammar.rs | 7 +- src/datadog/search/parser.rs | 33 +++++++++ src/parsing/xml.rs | 34 +++++---- src/stdlib/decode_gzip.rs | 31 ++++++-- src/stdlib/decode_snappy.rs | 60 ++++++++++++++- src/stdlib/decode_zlib.rs | 30 ++++++-- src/stdlib/decode_zstd.rs | 31 ++++++-- src/stdlib/parse_xml.rs | 25 +++++++ src/stdlib/util.rs | 11 +++ src/value/kind/crud/insert.rs | 121 ++++++++++++++++++++++++++++++- src/value/value/crud/mod.rs | 17 +++-- 13 files changed, 437 insertions(+), 46 deletions(-) diff --git a/src/compiler/expression/op.rs b/src/compiler/expression/op.rs index 2d479d110c..50a31173ae 100644 --- a/src/compiler/expression/op.rs +++ b/src/compiler/expression/op.rs @@ -278,11 +278,19 @@ impl Expression for Op { } // "bar" * 1 + // + // Deliberately stays infallible (OBE-10736). `try_mul` enforces a + // MAX_REPEAT_BYTES cap and returns an error above it, but marking this + // op fallible would be a breaking language change: every existing + // program doing `"x" * n` would stop compiling with E100. Integer + // `Add`/`Sub`/`Mul` make the same trade-off by wrapping rather than + // erroring. The over-limit error still surfaces as a graceful runtime + // error rather than an OOM, which is what the ticket required. Mul if lhs_def.is_bytes() && rhs_def.is_integer() => { lhs_def.union(rhs_def).with_kind(K::bytes()) } - // 1 * "bar" + // 1 * "bar" — see note above. Mul if lhs_def.is_integer() && rhs_def.is_bytes() => { lhs_def.union(rhs_def).with_kind(K::bytes()) } diff --git a/src/compiler/value/arithmetic.rs b/src/compiler/value/arithmetic.rs index 80b63b30bc..57c891dc71 100644 --- a/src/compiler/value/arithmetic.rs +++ b/src/compiler/value/arithmetic.rs @@ -11,6 +11,10 @@ use bytes::{BufMut, Bytes, BytesMut}; use super::ValueError; +/// Maximum byte length of a string produced by the `*` (repeat) operator. +/// Prevents OOM when an attacker supplies a large integer multiplier (OBE-10736). +const MAX_REPEAT_BYTES: usize = 64 * 1024 * 1024; // 64 MiB + pub trait VrlValueArithmetic: Sized { /// Similar to [`std::ops::Mul`], but fallible (e.g. `TryMul`). fn try_mul(self, rhs: Self) -> Result; @@ -75,11 +79,23 @@ impl VrlValueArithmetic for Value { // When multiplying a string by an integer, if the number is negative we set it to zero to // return an empty string. - let as_usize = |num| if num < 0 { 0 } else { num as usize }; + let as_usize = |num: i64| if num < 0 { 0 } else { num as usize }; let value = match self { Value::Integer(lhv) if rhs.is_bytes() => { - Bytes::from(rhs.try_bytes()?.repeat(as_usize(lhv))).into() + // `try_bytes` consumes `rhs`, so the `err` closure above (which borrows + // it) cannot be used past this point. Both operand kinds are known + // exactly in this arm, so build the error from them directly rather than + // deriving `Kind` from the values — `Kind::from(&Value)` deep-walks + // containers, and doing that eagerly would cost every multiplication. + let repeat_err = || ValueError::Mul(Kind::integer(), Kind::bytes()); + let bytes = rhs.try_bytes()?; + let n = as_usize(lhv); + let out_len = bytes.len().checked_mul(n).ok_or_else(repeat_err)?; + if out_len > MAX_REPEAT_BYTES { + return Err(repeat_err()); + } + Bytes::from(bytes.repeat(n)).into() } Value::Integer(lhv) if rhs.is_float() => { Value::from_f64_or_zero(lhv as f64 * rhs.try_float()?) @@ -93,7 +109,14 @@ impl VrlValueArithmetic for Value { lhv.mul(rhs).into() } Value::Bytes(lhv) if rhs.is_integer() => { - Bytes::from(lhv.repeat(as_usize(rhs.try_integer()?))).into() + // See the note in the `Integer * Bytes` arm above. + let repeat_err = || ValueError::Mul(Kind::bytes(), Kind::integer()); + let n = as_usize(rhs.try_integer()?); + let out_len = lhv.len().checked_mul(n).ok_or_else(repeat_err)?; + if out_len > MAX_REPEAT_BYTES { + return Err(repeat_err()); + } + Bytes::from(lhv.repeat(n)).into() } _ => return Err(err()), }; @@ -342,3 +365,47 @@ impl VrlValueArithmetic for Value { } } } + +#[cfg(test)] +mod tests { + use super::*; + + // OBE-10736: string * int must not OOM or capacity-overflow abort. + + #[test] + fn try_mul_bytes_large_count_returns_error() { + let s = Value::Bytes(bytes::Bytes::from("a")); + let n = Value::Integer(64 * 1024 * 1024 + 1); + assert!(s.try_mul(n).is_err(), "expected error for repeat count exceeding MAX_REPEAT_BYTES"); + } + + #[test] + fn try_mul_bytes_overflow_count_returns_error() { + let s = Value::Bytes(bytes::Bytes::from_static(b"aaaa")); // len = 4 + let n = Value::Integer(i64::MAX); // 4 * i64::MAX overflows usize on 64-bit + assert!(s.try_mul(n).is_err(), "expected error on checked_mul overflow"); + } + + #[test] + fn try_mul_int_bytes_large_count_returns_error() { + let n = Value::Integer(64 * 1024 * 1024 + 1); + let s = Value::Bytes(bytes::Bytes::from("b")); + assert!(n.try_mul(s).is_err(), "expected error for int * bytes exceeding MAX_REPEAT_BYTES"); + } + + #[test] + fn try_mul_bytes_small_count_succeeds() { + let s = Value::Bytes(bytes::Bytes::from("ab")); + let n = Value::Integer(3); + let result = s.try_mul(n).expect("expected success for small repeat"); + assert_eq!(result, Value::Bytes(bytes::Bytes::from("ababab"))); + } + + #[test] + fn try_mul_bytes_zero_count_returns_empty() { + let s = Value::Bytes(bytes::Bytes::from("hello")); + let n = Value::Integer(0); + let result = s.try_mul(n).expect("expected success for zero repeat"); + assert_eq!(result, Value::Bytes(bytes::Bytes::new())); + } +} diff --git a/src/datadog/search/grammar.rs b/src/datadog/search/grammar.rs index 3d9725e8a1..eee6e0dbac 100644 --- a/src/datadog/search/grammar.rs +++ b/src/datadog/search/grammar.rs @@ -242,9 +242,12 @@ impl QueryVisitor { ) => match (lc, rc) { (Comparison::Gte, Comparison::Lte) => (true, lv, rv, true), (Comparison::Gt, Comparison::Lt) => (false, lv, rv, false), - _ => panic!("invalid range comparison"), + // Mixed-bracket ranges: [lo TO hi} or {lo TO hi] + (Comparison::Gte, Comparison::Lt) => (true, lv, rv, false), + (Comparison::Gt, Comparison::Lte) => (false, lv, rv, true), + _ => unreachable!("grammar only produces Gt/Gte left and Lt/Lte right"), }, - _ => panic!("invalid range value"), + _ => unreachable!("grammar always emits bracket, value, value, bracket"), }; return QueryNode::AttributeRange { diff --git a/src/datadog/search/parser.rs b/src/datadog/search/parser.rs index ccf29e7e48..4fb5c37a17 100644 --- a/src/datadog/search/parser.rs +++ b/src/datadog/search/parser.rs @@ -505,6 +505,39 @@ mod tests { } } + // OBE-10726: mixed-bracket ranges ([lo TO hi} and {lo TO hi]) must not panic. + #[test] + fn parses_mixed_bracket_range_inclusive_lower() { + let res = parse("foo:[10 TO 20}"); + assert!( + matches!(res, + QueryNode::AttributeRange { + ref attr, + lower_inclusive: true, + upper_inclusive: false, + .. + } if attr == "foo"), + "expected inclusive lower, exclusive upper; got {:?}", + res + ); + } + + #[test] + fn parses_mixed_bracket_range_exclusive_lower() { + let res = parse("foo:{10 TO 20]"); + assert!( + matches!(res, + QueryNode::AttributeRange { + ref attr, + lower_inclusive: false, + upper_inclusive: true, + .. + } if attr == "foo"), + "expected exclusive lower, inclusive upper; got {:?}", + res + ); + } + #[test] fn parses_match_no_docs_query() { let cases = [ diff --git a/src/parsing/xml.rs b/src/parsing/xml.rs index 1bbfd1dc15..bd774be45c 100644 --- a/src/parsing/xml.rs +++ b/src/parsing/xml.rs @@ -1,5 +1,8 @@ use crate::compiler::prelude::*; use once_cell::sync::Lazy; + +// OBE-10742: bound recursion depth to prevent stack overflow on deeply-nested XML. +const MAX_XML_DEPTH: u32 = 128; use regex::{Regex, RegexBuilder}; use roxmltree::{Document, Node, NodeType}; use rust_decimal::prelude::Zero; @@ -91,14 +94,17 @@ pub(crate) fn parse_xml(value: Value, options: ParseOptions) -> Resolved { // Trim whitespace around XML elements, if applicable. let parse = if trim { trim_xml(&string) } else { string }; let doc = Document::parse(&parse).map_err(|e| format!("unable to parse xml: {e}"))?; - let value = process_node(doc.root(), &config); - Ok(value) + process_node(doc.root(), &config, 0) } /// Process an XML node, and return a VRL `Value`. -fn process_node(node: Node, config: &ParseXmlConfig) -> Value { +fn process_node(node: Node, config: &ParseXmlConfig, depth: u32) -> Resolved { + if depth > MAX_XML_DEPTH { + return Err(format!("xml nesting limit ({MAX_XML_DEPTH}) exceeded").into()); + } + // Helper to recurse over a `Node`s children, and build an object. - let recurse = |node: Node| -> ObjectMap { + let recurse = |node: Node| -> Result { let mut map = BTreeMap::new(); // Expand attributes, if required. @@ -119,7 +125,7 @@ fn process_node(node: Node, config: &ParseXmlConfig) -> Value { }; // Transform the node into a VRL `Value`. - let value = process_node(n, config); + let value = process_node(n, config, depth + 1)?; // If the key already exists, add it. Otherwise, insert. match map.entry(name) { @@ -143,11 +149,11 @@ fn process_node(node: Node, config: &ParseXmlConfig) -> Value { } } - map + Ok(map) }; match node.node_type() { - NodeType::Root => Value::Object(recurse(node)), + NodeType::Root => Ok(Value::Object(recurse(node)?)), NodeType::Element => { match ( @@ -155,9 +161,9 @@ fn process_node(node: Node, config: &ParseXmlConfig) -> Value { node.attributes().len().is_zero(), ) { // If the node has attributes, *always* recurse to expand default keys. - (_, false) if config.include_attr => Value::Object(recurse(node)), + (_, false) if config.include_attr => Ok(Value::Object(recurse(node)?)), // If a text key should be used, always recurse. - (true, true) => Value::Object(recurse(node)), + (true, true) => Ok(Value::Object(recurse(node)?)), // Otherwise, check the node count to determine what to do. _ => match node.children().count() { // For a single node, 'flatten' the object if necessary. @@ -171,21 +177,21 @@ fn process_node(node: Node, config: &ParseXmlConfig) -> Value { map.insert( node.tag_name().name().to_string().into(), - process_node(node, config), + process_node(node, config, depth + 1)?, ); - Value::Object(map) + Ok(Value::Object(map)) } else { // Otherwise, 'flatten' the object by continuing processing. - process_node(node, config) + process_node(node, config, depth + 1) } } // For 2+ nodes, expand. - _ => Value::Object(recurse(node)), + _ => Ok(Value::Object(recurse(node)?)), }, } } - NodeType::Text => process_text(node.text().expect("expected XML text node"), config), + NodeType::Text => Ok(process_text(node.text().expect("expected XML text node"), config)), _ => unreachable!("shouldn't be other XML nodes"), } } diff --git a/src/stdlib/decode_gzip.rs b/src/stdlib/decode_gzip.rs index 71fa223198..8a4ca1e811 100644 --- a/src/stdlib/decode_gzip.rs +++ b/src/stdlib/decode_gzip.rs @@ -1,16 +1,20 @@ use crate::compiler::prelude::*; +use crate::stdlib::util::{DECOMPRESS_LIMIT_ERROR, DEFAULT_DECOMPRESS_LIMIT}; use flate2::read::MultiGzDecoder; use std::io::Read; + fn decode_gzip(value: Value) -> Resolved { let value = value.try_bytes()?; let mut buf = Vec::new(); - let result = MultiGzDecoder::new(std::io::Cursor::new(value)).read_to_end(&mut buf); - - match result { - Ok(_) => Ok(Value::Bytes(buf.into())), - Err(_) => Err("unable to decode value with Gzip decoder".into()), + MultiGzDecoder::new(std::io::Cursor::new(value)) + .take(DEFAULT_DECOMPRESS_LIMIT + 1) + .read_to_end(&mut buf) + .map_err(|_| "unable to decode value with Gzip decoder")?; + if buf.len() as u64 > DEFAULT_DECOMPRESS_LIMIT { + return Err(DECOMPRESS_LIMIT_ERROR.into()); } + Ok(Value::Bytes(buf.into())) } #[derive(Clone, Copy, Debug)] @@ -97,4 +101,21 @@ mod tests { tdef: TypeDef::bytes().fallible(), } ]; + + // OBE-10737: a gzip bomb that decompresses to >64 MiB must return an error. + #[test] + fn gzip_bomb_exceeds_limit() { + // Compress 65 MiB of zeros — zeros compress to <1 KiB with gzip. + let zeros = vec![0u8; 65 * 1024 * 1024]; + let mut compressed = Vec::new(); + let mut enc = GzEncoder::new(zeros.as_slice(), flate2::Compression::best()); + enc.read_to_end(&mut compressed).unwrap(); + + let result = decode_gzip(Value::Bytes(compressed.into())); + assert!(result.is_err(), "expected error for gzip bomb exceeding size limit"); + assert!( + result.unwrap_err().to_string().contains("exceeds size limit"), + "error should mention size limit" + ); + } } diff --git a/src/stdlib/decode_snappy.rs b/src/stdlib/decode_snappy.rs index 5d4895698a..d242ca13b3 100644 --- a/src/stdlib/decode_snappy.rs +++ b/src/stdlib/decode_snappy.rs @@ -1,12 +1,22 @@ use crate::compiler::prelude::*; -use snap::raw::Decoder; +use crate::stdlib::util::{DECOMPRESS_LIMIT_ERROR, DEFAULT_DECOMPRESS_LIMIT}; +use snap::raw::{decompress_len, Decoder}; fn decode_snappy(value: Value) -> Resolved { let value = value.try_bytes()?; - let mut decoder = Decoder::new(); - let result = decoder.decompress_vec(&value); - match result { + // A snappy frame declares its uncompressed length up front, and + // `decompress_vec` sizes its output buffer from that value before doing any + // work. A few bytes of input can therefore claim gigabytes, so reject an + // over-limit claim before allocating anything (OBE-10737). + let claimed_len = + decompress_len(&value).map_err(|_| "unable to decode value with Snappy decoder")?; + if claimed_len as u64 > DEFAULT_DECOMPRESS_LIMIT { + return Err(DECOMPRESS_LIMIT_ERROR.into()); + } + + let mut decoder = Decoder::new(); + match decoder.decompress_vec(&value) { Ok(buf) => Ok(Value::Bytes(buf.into())), Err(_) => Err("unable to decode value with Snappy decoder".into()), } @@ -97,4 +107,46 @@ mod tests { tdef: TypeDef::bytes().fallible(), } ]; + + // OBE-10737: a snappy frame that *claims* a huge uncompressed length must be + // rejected before the output buffer is allocated. Unfixed, `decompress_vec` + // eagerly allocates the claimed size from this handful of input bytes. + // + // The claim is 1 GiB: comfortably above DEFAULT_DECOMPRESS_LIMIT but below + // snap's own `u32::MAX` ceiling, so snap itself will not reject it — our + // check is the only thing standing between this input and a 1 GiB alloc. + #[test] + fn snappy_oversized_claimed_length_rejected() { + // A snappy stream begins with the uncompressed length as a varint. + let mut payload = Vec::new(); + let mut claim: u64 = 1024 * 1024 * 1024; + while claim >= 0x80 { + payload.push((claim as u8) | 0x80); + claim >>= 7; + } + payload.push(claim as u8); + // Body is deliberately truncated — we must fail on the size claim, not + // by decoding to completion. + payload.extend_from_slice(&[0x00, 0x00, 0x00]); + + let result = decode_snappy(Value::Bytes(payload.into())); + assert!(result.is_err(), "oversized claimed length must be rejected"); + assert_eq!( + result.unwrap_err().to_string(), + DECOMPRESS_LIMIT_ERROR, + "must be rejected for exceeding the size limit, not as a decode error" + ); + } + + // A real snappy payload well under the limit must still round-trip. + #[test] + fn snappy_under_limit_still_decodes() { + let original = vec![b'a'; 1024 * 1024]; + let compressed = snap::raw::Encoder::new() + .compress_vec(&original) + .expect("snappy encode failed"); + + let result = decode_snappy(Value::Bytes(compressed.into())).expect("must decode"); + assert_eq!(result, Value::Bytes(original.into())); + } } diff --git a/src/stdlib/decode_zlib.rs b/src/stdlib/decode_zlib.rs index 9e037d1cdf..f78c27a329 100644 --- a/src/stdlib/decode_zlib.rs +++ b/src/stdlib/decode_zlib.rs @@ -1,16 +1,20 @@ use crate::compiler::prelude::*; +use crate::stdlib::util::{DECOMPRESS_LIMIT_ERROR, DEFAULT_DECOMPRESS_LIMIT}; use flate2::read::ZlibDecoder; use std::io::Read; + fn decode_zlib(value: Value) -> Resolved { let value = value.try_bytes()?; let mut buf = Vec::new(); - let result = ZlibDecoder::new(std::io::Cursor::new(value)).read_to_end(&mut buf); - - match result { - Ok(_) => Ok(Value::Bytes(buf.into())), - Err(_) => Err("unable to decode value with Zlib decoder".into()), + ZlibDecoder::new(std::io::Cursor::new(value)) + .take(DEFAULT_DECOMPRESS_LIMIT + 1) + .read_to_end(&mut buf) + .map_err(|_| "unable to decode value with Zlib decoder")?; + if buf.len() as u64 > DEFAULT_DECOMPRESS_LIMIT { + return Err(DECOMPRESS_LIMIT_ERROR.into()); } + Ok(Value::Bytes(buf.into())) } #[derive(Clone, Copy, Debug)] @@ -97,4 +101,20 @@ mod tests { tdef: TypeDef::bytes().fallible(), } ]; + + // OBE-10737: a zlib bomb that decompresses to >64 MiB must return an error. + #[test] + fn zlib_bomb_exceeds_limit() { + let zeros = vec![0u8; 65 * 1024 * 1024]; + let mut compressed = Vec::new(); + let mut enc = ZlibEncoder::new(zeros.as_slice(), flate2::Compression::best()); + enc.read_to_end(&mut compressed).unwrap(); + + let result = decode_zlib(Value::Bytes(compressed.into())); + assert!(result.is_err(), "expected error for zlib bomb exceeding size limit"); + assert!( + result.unwrap_err().to_string().contains("exceeds size limit"), + "error should mention size limit" + ); + } } diff --git a/src/stdlib/decode_zstd.rs b/src/stdlib/decode_zstd.rs index 008fd20363..de9078dc00 100644 --- a/src/stdlib/decode_zstd.rs +++ b/src/stdlib/decode_zstd.rs @@ -1,14 +1,21 @@ use crate::compiler::prelude::*; +use crate::stdlib::util::{DECOMPRESS_LIMIT_ERROR, DEFAULT_DECOMPRESS_LIMIT}; use nom::AsBytes; +use std::io::Read; + fn decode_zstd(value: Value) -> Resolved { let value = value.try_bytes()?; - let result = zstd::decode_all(value.as_bytes()); - - match result { - Ok(decoded_bytes) => Ok(Value::Bytes(decoded_bytes.into())), - Err(_) => Err("unable to decode value with Zstd decoder".into()), + let mut buf = Vec::new(); + zstd::Decoder::new(std::io::Cursor::new(value.as_bytes())) + .map_err(|_| "unable to decode value with Zstd decoder")? + .take(DEFAULT_DECOMPRESS_LIMIT + 1) + .read_to_end(&mut buf) + .map_err(|_| "unable to decode value with Zstd decoder")?; + if buf.len() as u64 > DEFAULT_DECOMPRESS_LIMIT { + return Err(DECOMPRESS_LIMIT_ERROR.into()); } + Ok(Value::Bytes(buf.into())) } #[derive(Clone, Copy, Debug)] @@ -93,4 +100,18 @@ mod tests { tdef: TypeDef::bytes().fallible(), } ]; + + // OBE-10737: a zstd bomb that decompresses to >64 MiB must return an error. + #[test] + fn zstd_bomb_exceeds_limit() { + let zeros = vec![0u8; 65 * 1024 * 1024]; + let compressed = zstd::encode_all(zeros.as_slice(), 22).expect("zstd encode failed"); + + let result = decode_zstd(Value::Bytes(compressed.into())); + assert!(result.is_err(), "expected error for zstd bomb exceeding size limit"); + assert!( + result.unwrap_err().to_string().contains("exceeds size limit"), + "error should mention size limit" + ); + } } diff --git a/src/stdlib/parse_xml.rs b/src/stdlib/parse_xml.rs index 4fac4980ff..6a922f2f55 100644 --- a/src/stdlib/parse_xml.rs +++ b/src/stdlib/parse_xml.rs @@ -481,6 +481,31 @@ mod tests { } ]; + // OBE-10742: deeply-nested XML must return an error, not overflow the stack. + #[test] + fn deeply_nested_xml_returns_error() { + // Build ... with 200 levels — exceeds MAX_XML_DEPTH (128). + let open: String = "".repeat(200); + let close: String = "".repeat(200); + let xml = format!("{}{}", open, close); + let result = parse_xml( + Value::Bytes(xml.into()), + ParseOptions { + trim: None, + include_attr: None, + attr_prefix: None, + text_key: None, + always_use_text_key: None, + parse_bool: None, + parse_null: None, + parse_number: None, + }, + ); + assert!(result.is_err(), "expected error for XML exceeding depth limit"); + let msg = result.unwrap_err().to_string(); + assert!(msg.contains("nesting limit"), "error should mention nesting limit; got: {msg}"); + } + #[test] fn test_kind() { let state = state::TypeState::default(); diff --git a/src/stdlib/util.rs b/src/stdlib/util.rs index aee7107557..be351a8350 100644 --- a/src/stdlib/util.rs +++ b/src/stdlib/util.rs @@ -1,6 +1,17 @@ use crate::compiler::{Context, Expression, Resolved, TypeState}; use crate::value::{KeyString, ObjectMap, Value}; +/// Maximum number of bytes any `decode_*` decompression function will produce +/// (OBE-10737). +/// +/// Compressed formats let a tiny input expand enormously ("decompression +/// bombs"), so every decoder caps its output at this size and returns a +/// recoverable error rather than exhausting memory. +pub(crate) const DEFAULT_DECOMPRESS_LIMIT: u64 = 64 * 1024 * 1024; // 64 MiB + +/// Error returned when a decoder's output would exceed [`DEFAULT_DECOMPRESS_LIMIT`]. +pub(crate) const DECOMPRESS_LIMIT_ERROR: &str = "decompressed output exceeds size limit"; + /// Rounds the given number to the given precision. /// Takes a function parameter so the exact rounding function (ceil, floor or round) /// can be specified. diff --git a/src/value/kind/crud/insert.rs b/src/value/kind/crud/insert.rs index a44c55d362..d1224369fe 100644 --- a/src/value/kind/crud/insert.rs +++ b/src/value/kind/crud/insert.rs @@ -1,9 +1,51 @@ //! All types related to inserting one [`Kind`] into another. use crate::path::{BorrowedSegment, ValuePath}; -use crate::value::kind::Collection; +use crate::value::kind::{Collection, Index}; use crate::value::Kind; +/// Largest number of `known` array entries this module will materialize while +/// modelling an array index (OBE-10721). +/// +/// Array index insertion normally records one `known` entry per index between +/// the existing entries and the target index, so an index such as `-99999999` +/// would allocate ~100M `Kind`s and exhaust memory *at compile time*. Beyond +/// this threshold we stop enumerating and widen the collection instead — see +/// [`collapse_array_to_unknown`]. +const MAX_KNOWN_INDEX_ENTRIES: usize = 128; + +/// Replace a precise, per-index array type with an imprecise but sound one. +/// +/// Every `known` entry is folded into the `unknown` kind, together with `null` +/// (extending an array to reach the index creates null holes) and the kind +/// resulting from the insertion itself. Callers use this when enumerating the +/// indices individually would exhaust memory. +/// +/// This only ever *widens* the type: each index that previously had a precise +/// `known` kind is now described by an `unknown` that is a superset of it. That +/// makes later expressions more fallible, never less, so it cannot mask a +/// type error. +fn collapse_array_to_unknown<'b>( + collection: &mut Collection, + iter: impl Iterator> + Clone, + kind: Kind, +) { + let mut widened = collection.unknown_kind(); + for known_kind in collection.known().values() { + widened = widened.union(known_kind.clone()); + } + // Holes created by extending the array to reach the index. + widened = widened.union(Kind::null()); + + // The insertion may land on any index in the collapsed range. + let mut with_insertion = widened.clone(); + with_insertion.insert_recursive(iter, kind); + widened = widened.union(with_insertion); + + collection.known_mut().clear(); + collection.set_unknown(widened); +} + impl Kind { /// Insert the `Kind` at the given `path` within `self`. /// This has the same behavior as `Value::insert`. @@ -58,10 +100,25 @@ impl Kind { *self = Self::array(self.array.clone().unwrap_or_else(Collection::empty)); let collection = self.array.as_mut().expect("array was just inserted"); + // OBE-10721: every branch below records one `known` entry per index + // between the existing entries and `index`. For a far-away index that + // exhausts memory (and time) at compile time, so widen instead of + // enumerating. `unsigned_abs` also avoids the `-index` overflow that + // `isize::MIN` would otherwise cause. + let indices_required = if index < 0 { + index.unsigned_abs() + } else { + (index as usize).saturating_add(1) + }; + if indices_required > MAX_KNOWN_INDEX_ENTRIES { + collapse_array_to_unknown(collection, iter, kind); + return; + } + if index < 0 { let largest_known_index = collection.largest_known_index(); // The minimum size of the resulting array. - let len_required = -index as usize; + let len_required = index.unsigned_abs(); let unknown_kind = collection.unknown_kind(); if unknown_kind.contains_any_defined() { @@ -648,4 +705,64 @@ mod tests { assert_eq!(this, expected, "{title}"); } } + + // OBE-10721: a far-away index must not OOM/hang the type-checker, in any of the + // three branches that materialize one `known` entry per index. Each of these + // enumerated ~100M entries before the fix. The wall-clock bound is the real + // assertion — unfixed, each of these takes hours. + #[test] + fn far_away_index_does_not_oom() { + let cases: Vec<(&str, Kind, i64)> = vec![ + // Negative index, collection has a defined `unknown` (shift-simulation path). + ( + "negative with unknown", + Kind::array(Collection::empty().with_unknown(Kind::integer())), + -99_999_999, + ), + // Negative index, no `unknown` (exact-position hole-fill path). + ( + "negative without unknown", + Kind::array(Collection::from_parts( + [(0.into(), Kind::integer()), (1.into(), Kind::integer())].into(), + Kind::undefined(), + )), + -99_999_999, + ), + // Positive index (hole-fill path). + ("positive", Kind::array(Collection::empty()), 99_999_999), + ]; + + for (name, mut kind, index) in cases { + let start = std::time::Instant::now(); + kind.insert(&owned_value_path!(index as isize), Kind::bytes()); + let elapsed = start.elapsed(); + + assert!( + elapsed < std::time::Duration::from_secs(5), + "{name}: insert took {elapsed:?}, expected well under 5s" + ); + + let arr = kind.as_array().expect("result is an array"); + assert!( + arr.known().len() <= MAX_KNOWN_INDEX_ENTRIES, + "{}: expected at most {} known entries, got {}", + name, + MAX_KNOWN_INDEX_ENTRIES, + arr.known().len() + ); + + // Soundness: the collapsed `unknown` must still admit everything that + // could really be at those indices — the inserted kind, the null holes, + // and any kind that was previously known. + let unknown = arr.unknown_kind(); + assert!( + unknown.contains_bytes(), + "{name}: collapsed unknown must admit the inserted bytes kind" + ); + assert!( + unknown.contains_null(), + "{name}: collapsed unknown must admit null holes" + ); + } + } } diff --git a/src/value/value/crud/mod.rs b/src/value/value/crud/mod.rs index 9883adc515..2ea0d7657a 100644 --- a/src/value/value/crud/mod.rs +++ b/src/value/value/crud/mod.rs @@ -115,12 +115,19 @@ impl ValueCollection for Vec { Some(std::mem::replace(&mut self[key as usize], value)) } } else { - let len_required = -key as usize; + // `unsigned_abs` rather than `-key`, which overflows on `isize::MIN`. + let len_required = key.unsigned_abs(); if self.len() < len_required { - while self.len() < (len_required - 1) { - self.insert(0, Value::Null); - } - self.insert(0, value); + // Prepend the value followed by the null holes needed to reach + // `len_required`. Building this in one pass keeps the operation + // O(n); repeated `insert(0, ..)` calls are O(n²) and turn a large + // negative index into a hang (OBE-10721). + let holes = len_required - 1 - self.len(); + let mut extended = Self::with_capacity(len_required); + extended.push(value); + extended.extend(std::iter::repeat(Value::Null).take(holes)); + extended.append(self); + *self = extended; None } else { let index = (self.len() as isize + key) as usize;