Skip to content

fix(value): [OBE-10735] consolidate and raise the array-index cap - #14

Open
JuanMantica45 wants to merge 2 commits into
Sentinel-One:mainfrom
JuanMantica45:obe-10735-array-index-cap
Open

fix(value): [OBE-10735] consolidate and raise the array-index cap#14
JuanMantica45 wants to merge 2 commits into
Sentinel-One:mainfrom
JuanMantica45:obe-10735-array-index-cap

Conversation

@JuanMantica45

Copy link
Copy Markdown
Contributor

Why

Supersedes #12, which is conflicting with main. Same substance, rebased and self-contained.

main declares the bound twice — MAX_ARRAY_INDEX = 32_768 (crud/mod.rs) enforces it, MAX_ARRAY_CAPACITY = 32_769 (crud/insert.rs) preallocates for it. Two constants for one invariant means they can drift. This collapses them into one pub(super) constant and raises it to 2^20, per @ajayshekar-s1's feedback on #7 that 32_768 could reject legitimate large-array use.

The cap stays, and it cannot be optimised away

The review question on #7 was whether the cap should exist at all. It has to, and not for the reason the original fix implied.

Assigning to index N materialises N + 1 elements. The null padding is observable VRL semantics, not a preallocation hint — length() sees it, and test_insert_array already asserts c[2] = 10 yields [5, null, 10]. So removing Vec::with_capacity would only trade one large allocation for amortised doubling; the array still ends up holding N + 1 elements. There is no version of this where an event-controlled index does not commit memory proportional to the index. Bounding it is the only fix that does not change the language.

2^20 bounds a single indexed write to ~42 MB at today's 40-byte Value — 32x more headroom than the original cap, while staying bounded rather than unbounded.

What changed

  • One pub(super) const MAX_ARRAY_INDEX = 1_048_576 in crud/mod.rs; insert.rs derives its capacity bound as MAX_ARRAY_INDEX + 1 instead of redeclaring it. grep MAX_ARRAY_CAPACITY src/ now returns nothing.
  • index.unsigned_abs() replaces (-index) as usize in the capacity calculation, which overflows on isize::MIN (no positive isize counterpart). insert_value already used unsigned_abs; this was the remaining call site. Found by @jsbalis1 in review of fix(security): prevent 9 panic/OOM vectors in VRL runtime (batch J) #7.
  • test_value_size_is_pinned asserts size_of::<Value>() is 40. It is a drift detector, not a correctness assertion: the cap is justified in terms of memory (cap x size), so a new Value variant should force a human to re-check the budget rather than silently changing it.

Test plan

  • cargo test --lib: 1761 passed, 0 failed.
  • New/updated in crud::insert: paired accept-at-2^20 / reject-at-2^20+1 (both signs), isize::MIN no-panic, and the size_of pin. The accept-at-cap and isize::MIN tests both fail against main — the latter with attempt to negate with overflow at insert.rs:33.
  • lib/tests fixture runner: new obe 10735 array index cap passes. Suite goes 762 -> 763 passed; the 2 parse_etld custom-PSL failures and 3 emit_metric clippy errors are present on unmodified main and are untouched here.

🤖 Generated with Claude Code

`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) <noreply@anthropic.com>
Comment thread src/value/value/crud/insert.rs Outdated
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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even for a rejection we're still creating a 42MB array here. so an out-of-range index still reserves the full capacity.
Same thing as above but for negative index, and worse since this isn't even a rejected case. Lets fix this

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, fixed in 4e502bd.

Root cause: insert_value (crud/mod.rs) already validates the index against MAX_ARRAY_INDEX before allocating anything, and already does its own correctly-sized allocation once the range check passes (a push growth loop for positive indices, Self::with_capacity(len_required) for negative ones — which fully replaces *self). So the Vec::with_capacity(capacity) here was redundant at best and wasteful at worst:

  • Out-of-range writes (either sign) still paid for the full ~42MB with_capacity before insert_value rejected them and returned None.
  • In-range negative-index writes paid for it twice: this allocation was immediately discarded by insert_value's *self = extended.

Replaced the capacity computation with plain Vec::new() and let insert_value own the one allocation it actually needs. Added test_insert_beyond_max_array_index_does_not_preallocate, which asserts a rejected out-of-range write leaves array.capacity() == 0.

`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 <noreply@anthropic.com>
// `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);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is already validation for max capacity here

fn insert_value(&mut self, key: isize, value: Value) -> Option<Value> {
115:     let max_index = MAX_ARRAY_INDEX as isize;
116:     if !(-max_index..=max_index).contains(&key) {
117:         // TODO: VRL-side array-index assignment is currently infallible (see
118:         // compiler::expression::assignment::Target::insert), so we can't surface this as a
119:         // proper VRL runtime error without a larger change. Log it so it's at least
120:         // observable instead of a silent no-op.
121:         tracing::warn!(
122:             index = key,
123:             max_index = MAX_ARRAY_INDEX,
124:             "array index assignment out of range, write dropped"
125:         );
126:         return None;
127:     }

/// 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;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incremented to give more headroom

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants