fix(compiler): [OBE-10738] stop cloning the AST, and guard compilation on remaining stack by JuanMantica45 · Pull Request #15 · Sentinel-One/vrl

JuanMantica45 · 2026-08-27T19:21:11Z

Supersedes #13. Same ticket, different conclusion — measurement changed the diagnosis.

The bug

Compiling a nested VRL program recurses once per nesting level. Each level eats native stack. Nest deep enough and the thread walks into its guard page: SIGSEGV, which Rust cannot catch. The process dies, taking every other pipeline on that worker with it. One hostile program is enough.

Where the stack actually goes

I measured each phase in isolation on a 2 MiB thread. The result reframed the fix — the compiler's own recursion was not the first thing to break.

flowchart TD
    SRC["VRL source<br/>!!!!…!true"] --> P

    P["parse()<br/>255 B per level<br/>table-driven LR — cheap"]
    P --> C["ast.clone()<br/>~2,900 B per level<br/>DIES AT DEPTH ~700"]
    C --> K["Compiler::compile_expr<br/>5,030 B/level release<br/>10,850 B/level debug<br/>DIES AT DEPTH ~417"]
    K --> U["check_for_unused_results<br/>small per level"]

    style C fill:#ffd6d6,stroke:#c00,stroke-width:2px
    style K fill:#ffd6d6,stroke:#c00,stroke-width:2px

Two things follow immediately:

ast.clone() runs before the compiler. So the MAX_EXPR_DEPTH = 128 counter proposed in fix(security): reject excessive VRL expression nesting at compile time (OBE-10738, OBE-10740) #13 sits downstream of a walk that already overflowed. It could never have fired on the public compile() entry point.
The parser is not the problem. fix(security): reject excessive VRL expression nesting at compile time (OBE-10738, OBE-10740) #13 stated it needs 32 MB to reach 130 levels. It does 130 levels in about 8 KB. The compiler is ~80x more expensive per level and is what genuinely bounds nesting.

Fix, part 1 — remove the clone

The clone existed only so the AST survived for the unused-expression check afterwards. Running that check first lets the AST be moved into the compiler instead of copied.

flowchart LR
    subgraph AFTER["after"]
        direction TB
        B1["parse()"] --> B2["check_for_unused_results(&ast)"]
        B2 --> B3["Compiler::compile(fns, ast, ..)<br/>moved, not cloned"]
    end

    subgraph BEFORE["before"]
        direction TB
        A1["parse()"] --> A2["ast.clone()<br/>deep copy of the whole tree"]
        A2 --> A3["Compiler::compile(fns, ast.clone(), ..)"]
        A3 --> A4["check_for_unused_results(&ast)"]
    end

    style A2 fill:#ffd6d6,stroke:#c00,stroke-width:2px

That removes a recursive walk and stops deep-copying the entire AST on every single compile — a straight performance win, independent of the security fix.

Fix, part 2 — stop before the stack runs out

compile_expr and the unused-expression visitor now check stacker::remaining_stack() before descending, and emit a diagnostic instead of recursing into the guard page. None (platform cannot report) is treated as "proceed", so unsupported platforms keep today's behaviour.

Both reserves are a fraction of the stack available when the walk starts, not a byte count. This is the part worth reviewing. A fixed reserve is wrong at some stack size by construction, because the work still owed when the guard fires scales with the depth already reached, which scales with the stack you started with. A 64 KiB reserve tuned on release measurements overflowed in debug builds, whose frames are several times fatter.

A fixed depth is wrong the same way, in both directions:

thread stack	levels it can actually hold	`MAX_EXPR_DEPTH = 128`
512 KiB	~104	still overflows
2 MiB	~417	rejects at 3.25x margin
32 MiB	~6,600	rejects at 50x margin

Fix, part 3 — clean up without recursing

The guard alone was not enough, and this is the non-obvious part. With parts 1 and 2 in place the guard fired correctly and the process still died — just later, and in cleanup rather than compilation.

When the guard bails it still owns the entire un-compiled remainder of the program. Letting that go out of scope runs the derived drop glue, which reaches into each nesting level in turn:

flowchart TD
    S["2 MiB stack, guard but no iterative teardown"] --> D1
    D1["compile_expr descends ~92 levels<br/>≈ 1 MB of stack consumed"]
    D1 --> D2["guard fires: remaining &lt; floor<br/>returns None"]
    D2 --> D3["the un-compiled remainder — ~4,900 levels —<br/>is dropped by derived drop glue"]
    D3 --> D4["that drop needs 4,900 × 255 B ≈ 1.25 MB<br/>but only ~1 MB is left"]
    D4 --> D5["PROCESS ABORTS DURING CLEANUP"]

    style D5 fill:#ffd6d6,stroke:#c00,stroke-width:2px

No reserve could ever have been large enough. The cleanup cost depends on the program's depth, not on where compilation stopped — a 100,000-level program needs ~25 MB to drop its AST however early the guard trips.

So the new ast_teardown module unwinds it against an explicit heap worklist: pop a node, move its children onto the worklist, let the childless remainder drop. Stack usage is constant regardless of nesting depth.

flowchart LR
    subgraph ITER["ast_teardown::drop_expr — iterative"]
        direction TB
        I1["worklist: Vec&lt;Expr&gt; on the heap"]
        I2["pop a node"]
        I3["move its children onto the worklist"]
        I4["childless remainder drops"]
        I1 --> I2 --> I3 --> I4 --> I2
        I5["stack usage: CONSTANT"]
    end

    subgraph REC["derived Drop — recursive"]
        direction TB
        R1["drop level 1"] --> R2["drop level 2"]
        R2 --> R3["drop level 3"]
        R3 --> R4["… one stack frame per level …"]
        R4 --> R5["stack usage: O(depth)"]
    end

    style R5 fill:#ffd6d6,stroke:#c00,stroke-width:2px
    style I5 fill:#d6f5d6,stroke:#0a0,stroke-width:2px

Every match in that module is exhaustive, with no _ arm, across all 11 ast::Expr variants and 4 Container variants. Adding a grammar variant later fails to compile rather than silently reintroducing a recursive drop path.

This is the same shape as the iterative depth check used for Value in OBE-10732 (#16): bound the structure rather than trying to survive traversing it.

Result

Verified on both build profiles and both stack sizes:

depth	512 KiB	2 MiB
1,000	rejected cleanly	rejected cleanly
50,000	rejected cleanly	rejected cleanly
1,000,000	rejected cleanly	rejected cleanly

For contrast: main aborts at depth 417 (release). An earlier revision of this branch — parts 1 and 2 but no iterative teardown — still aborted past ~5,000, which is why part 3 is load-bearing rather than a tidy-up.

Dependency note — @jsbalis1

This adds stacker to a crate every downstream consumer (including Vector) depends on. Only remaining_stack() is used, never maybe_grow — #13 already established maybe_grow does not help here, since type_info recurses outside the wrapped frame.

It is pinned >=0.1, <0.1.22: later releases need cc >= 1.2.33, which requires edition2024 and does not build on the Rust 1.83 this repo pins in rust-toolchain.toml. psm is pinned in Cargo.lock for the same reason. Worth a look — this is the concern that split #13 out of #9.

Test plan

cargo test --lib: 1767 passed, 0 failed.
Differential pair — the same 1,000-level program is rejected with a diagnostic on a 2 MiB stack and compiles on a 32 MiB one. A MAX_EXPR_DEPTH = 128 implementation fails the 32 MiB half, which is the point.
adapts_to_a_small_stack: 200 levels rejected on 512 KiB, 20 accepted.
rejects_pathological_nesting_without_dying_in_cleanup: 50,000 levels on both 512 KiB and 2 MiB. This is the regression test for part 3 — it aborts the process if the iterative teardown is reverted, so it genuinely guards what it claims to.
Three ast_teardown tests parse on a large stack and tear down on a 512 KiB one, covering nested unary, nested arrays and nested groups.

Closes OBE-10738. Closes OBE-10740 transitively — a program that cannot compile never reaches the runtime resolver, whose recursion follows the same nesting.

🤖 Generated with Claude Code

janmejay-s1

After addressing the comments, please create a branch in vector to pull this dependency and trigger test and publish on dataplane-build.

janmejay-s1 · 2026-09-10T16:21:09Z

+    }
+}
+
+fn push_block(block: crate::parser::ast::Block, worklist: &mut Vec<Expr>) {


lets use (avoid fully qualified names, makes code verbose)

janmejay-s1 · 2026-09-10T16:44:27Z

+                    .map(|_| ())
+                    .map_err(|_| ())


don't suppress errors, let it propagate, retrieve DiagnosticList and make the assertions in the tests below stronger by checking the actual code and part of the message, say there is atleast 1 diagnostic such that message contains reduce the nesting depth etc.

janmejay-s1 · 2026-09-10T16:54:40Z

+/// Fraction of the stack available when the walk starts that it will not descend into. Like the
+/// compiler's guard this is a fraction rather than a byte count: a fixed reserve lets a cheap
+/// per-level walk descend until only that fixed amount remains, which is then too little for
+/// whatever it calls at the bottom.
+const VISITOR_STACK_RESERVE_FRACTION: usize = 2;


Why not have just one of these constants, say STACK_RESERVE_FRACTION and then re-use here?

janmejay-s1 · 2026-09-10T16:58:18Z

+    #[test]
+    fn rejects_nesting_that_would_exhaust_the_stack() {
+        assert!(
+            compile_at(1_000, 2 * 1024 * 1024).is_err(),


Let us add a comment here that says: "If you see a sigsegv here on any architecture, it is likely a result of stacker not supporting the architecture, which means VRL may not be safe to run on that architecture."

janmejay-s1 · 2026-09-10T18:09:54Z

+        // unused-expression warnings for its deepest nodes costs nothing.
+        if let (Some(remaining), Some(floor)) = (stacker::remaining_stack(), state.stack_floor) {
+            if remaining < floor {
+                return;


append a diagnostic identifying the condition, something like "unused check abandoned, stack bottomed out at {floor} bytes"

janmejay-s1 · 2026-09-10T18:11:57Z

        let mut unused_warnings = DiagnosticList::default();
-        let mut state = VisitorState::default();
+        let mut state = VisitorState {
+            stack_floor: stacker::remaining_stack().map(|r| r / VISITOR_STACK_RESERVE_FRACTION),


why not move this to default (instead of letting default be something that is unsafe?)

in general, try to avoid allowing types to be instantiated in bad state

janmejay-s1 · 2026-09-10T18:18:12Z

+# Pinned below 0.1.22: later releases require `cc` >= 1.2.33, which needs edition2024 and does
+# not build on the toolchain this crate pins (see rust-toolchain.toml, Rust 1.83).
+stacker = { version = ">=0.1, <0.1.22", optional = true }


please double-check this, according to quick analysis by Claude of the pinning here, psm needs to be pinned, details:

1. The dependency comment is misattributed. The Cargo.toml comment and PR body both say "later releases require cc >= 1.2.33, which needs edition2024 and does not build on … Rust 1.83." That's factually wrong — cc builds on 1.83 at every version tested. The actual chain is stacker → psm ≥ 0.1.32 (requires 1.88) → ar_archive_writer 0.5.3 (edition2024). The comment should name psm, not cc. 2. The pin is fragile — it only holds via the lockfile. The Cargo.toml constraint is on stacker (<0.1.22), but stacker 0.1.21's requirement on psm is loose, so psm is only held at 0.1.26 by Cargo.lock. As the last probe proves, a plain cargo update (or any lock regeneration) floats psm to 0.1.32 and silently breaks the 1.83 build again. The stacker < 0.1.22 bound does not protect you; the PR body even notes "psm is pinned in Cargo.lock for the same reason" — but a lock-only pin isn't enforced against cargo update. Durable fix: add an explicit psm bound to Cargo.toml (e.g. psm = ">=0.1, <0.1.32", or whatever the last 1.83-compatible version is — 0.1.31 is worth testing since it was "available"), so the constraint survives lock regeneration. Better still, since this is really about the repo pinning an old toolchain, the long-term answer is bumping rust-toolchain.toml past 1.85 — but that's a much larger call and out of scope for this security fix.

janmejay-s1 · 2026-09-10T18:20:43Z

            external_assignments: vec![],
            skip_missing_query_target: vec![],
            fallible_expression_error: None,
+            stack_floor: stacker::remaining_stack().map(|r| r / STACK_RESERVE_FRACTION),


Why not compute this and return a diagnostic-error when we are unable to determine stack-floor. Failing with a clear message would be superior to silent crash. This will eventually slip off everyone's mind, its better to ensure we see a failure when we can't determine remaining capacity.

janmejay-s1 · 2026-09-10T18:23:01Z

+        // the stack runs short the correct move is to stop descending rather than to fail — a
+        // deeply-nested program is about to be rejected by the compiler anyway, and losing
+        // unused-expression warnings for its deepest nodes costs nothing.
+        if let (Some(remaining), Some(floor)) = (stacker::remaining_stack(), state.stack_floor) {


ditto, if we build in default we can't really return error, but we can populate a diagnostic (may be Abort) and draw operators attention to the fact that we don't know stack size in the said environment

…n on remaining stack Compiling a deeply-nested VRL program drives native-stack recursion with no bound, ending in a guard-page SIGSEGV that Rust cannot catch — the process dies, not the transform. The dominant cause was not the compiler's own recursion. `compile_with_state` called `Compiler::compile(fns, ast.clone(), ..)` so that the AST survived for the unused-expression check afterwards. `Clone` on the AST is derived, so it recursed once per nesting level and overflowed at depth ~700 on a 2 MiB stack — before the compiler ran, and therefore before any guard inside the compiler could fire. Running the check first lets the AST be moved into the compiler instead of copied. That removes the recursion and also stops cloning the whole tree on every single compile. On top of that, `compile_expr` and the unused-expression visitor now stop descending when `stacker::remaining_stack()` reports the thread is running out, reporting a diagnostic rather than recursing into the guard page. Both reserves are a *fraction* of the stack available when the walk starts, not a byte count. A fixed reserve is wrong at some stack size by construction: the work still owed when the guard fires scales with the depth already reached, which itself scales with the stack. A 64 KiB reserve tuned against release measurements overflowed in debug builds, whose frames are several times fatter. Measured stack cost per nesting level, by phase (2 MiB thread): parser 255 B (LR, table-driven) ast.clone() 2,900 B removed by this commit compile_expr 10,850 B debug / 5,030 B release AST drop 255 B derived Drop, not guardable Together these take a 2 MiB thread from overflowing at depth 417 (release) to compiling or cleanly rejecting past 2,000. This does not fully close OBE-10738, and the PR says so. Beyond ~5,000 levels the process still dies dropping the un-compiled remainder of the AST: that drop is derived `Drop`, costs stack proportional to the *program's* depth rather than to where compilation stopped, and so cannot be bounded by any reserve. Closing it needs a depth bound taken before the AST is built, which is its own change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…when the guard fires The stack guard added in the previous commit fired correctly and the process still died — just later, and in cleanup rather than in compilation. When the guard bails it still owns the entire un-compiled remainder of the program. Letting that go out of scope runs the derived drop glue, which reaches into each nesting level in turn and costs stack proportional to the *program's* depth, not to the depth at which compilation stopped. So no reserve could ever have been large enough: a 100,000-level program needs ~25 MB to drop its AST however early the guard trips. Tear it down against an explicit heap worklist instead — pop a node, move its children onto the worklist, let the childless remainder drop. Nothing recurses, so nesting depth cannot exhaust the stack. This is the same shape as the iterative depth check used for `Value` in OBE-10732. Every match in the new module is exhaustive, with no `_` arm, so adding an AST variant fails to compile rather than silently reintroducing a recursive drop. This closes the gap the previous commit documented as still open. Verified on both build profiles and both stack sizes: depth 512 KiB 2 MiB 1,000 rejected rejected 50,000 rejected rejected 1,000,000 rejected rejected Previously anything past ~5,000 aborted the process on a 2 MiB stack. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…emaining stack Address review: on a platform where stacker::remaining_stack() returns None, Compiler::compile used to silently proceed without a stack guard instead of failing with a diagnostic - a decision that would eventually slip off everyone's mind. Compute the floor once up front and bail out with a new UnknownStackBoundsError if it can't be determined, rather than deferring to an unguarded compile_expr. Also use the imported Block alias instead of the fully-qualified path in ast_teardown.rs, and strengthen rejects_nesting_that_would_exhaust_the_stack to assert on the actual diagnostic (code 670, "reduce the nesting depth" note) instead of just Result::is_err. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

… stack floor Address review: VisitorState derived Default, which silently set stack_floor to None (disabling the visit_node stack guard) unless the one call site overrode it by hand. Give VisitorState a hand-written Default that always computes stack_floor from stacker::remaining_stack(), so it can't be constructed in that unsafe state again; when the platform can't report a bound, push a diagnostic instead (this walk is advisory-only and can't return an error). Also append a diagnostic when the guard actually fires, identifying that the unused-expression check was abandoned and at what stack floor, and reuse the compiler's STACK_RESERVE_FRACTION instead of a second copy of the same constant. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…go.lock chain Address review: the stacker < 0.1.22 bound in Cargo.toml doesn't actually hold psm down - stacker 0.1.21 accepts any 0.1.x psm, so only Cargo.lock was keeping it at 0.1.26. A plain `cargo update` floats psm to 0.1.29+, which pulls in ar_archive_writer 0.5.3 (requires edition2024 / Rust 1.88) and breaks the Rust 1.83 build this crate pins. Verified directly against rustc 1.83: psm 0.1.28 builds, 0.1.29 fails with "feature `edition2024` is required" - the original comment's "cc >= 1.2.33" attribution was incorrect, and 0.1.31 (suggested for testing) also pulls in the same edition2024 chain. Add psm as an explicit optional dependency, gated on the same `compiler` feature as stacker, with an upper bound of <0.1.29 so the constraint holds across a lockfile regeneration instead of only in Cargo.lock. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

janmejay-s1

LGTM, but please trigger test and publish workflows in dataplane-build for this branch (branch vector, pin it to this branch and trigger the workflows). Once they complete please link em here.

Address review: stack_floor was Option<usize> even though Compiler::compile always populates it - the platform-can't-report-a- bound case already bails out with a diagnostic before the Compiler is constructed, so the type can't actually be None at that point. Store it as usize instead of wrapping the always-Some value, dropping the now-unneeded Some destructuring in compile_expr's guard. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

JuanMantica45 · 2026-09-11T19:17:05Z

Addressed the "make types stricter" comment (stack_floor: Option<usize> → usize) — pushed.

Vector CI, pinned to this branch (via Sentinel-One/vector#160, test-only, not for merge):

Test Suite: https://ghe.eng.sentinelone.tech/sentinel-one/dataplane-build/actions/runs/12748727 — success
Publish: https://ghe.eng.sentinelone.tech/sentinel-one/dataplane-build/actions/runs/12747597 — success
- Image: 822434346939.dkr.ecr.us-east-1.amazonaws.com/vector:0.44.13.custom.5e69e68-debian

Note: the first Test Suite run failed on an unrelated pre-existing issue — format_number became fallible in an earlier PR (batch-J, vrl#7), but vector's vrl pin had never advanced far enough to expose it in vector's own doc example. Fixed that doc example in the same throwaway vector PR to unblock the run; it's unrelated to this fix and out of scope for this PR.

Will upgrade the dataplane on one of my test sites next and validate a sample-logs pipeline.

🤖 Generated with Claude Code

JuanMantica45 mentioned this pull request Aug 27, 2026

fix(security): reject excessive VRL expression nesting at compile time (OBE-10738, OBE-10740) #13

Closed

janmejay-s1 reviewed Sep 10, 2026

View reviewed changes

JuanMantica45 and others added 5 commits September 10, 2026 16:44

JuanMantica45 force-pushed the obe-10738-stack-headroom-guard branch from 5b31ea7 to c047803 Compare September 10, 2026 20:58

janmejay-s1 reviewed Sep 11, 2026

View reviewed changes

JuanMantica45 mentioned this pull request Sep 11, 2026

test(deps): [OBE-10738] pin vrl to stack-headroom-guard fix for CI validation (DO NOT MERGE) Sentinel-One/vector#160

Open

janmejay-s1 approved these changes Sep 15, 2026

View reviewed changes

janmejay-s1 merged commit f2566db into Sentinel-One:main Sep 15, 2026

Navigation Menu

fix(compiler): [OBE-10738] stop cloning the AST, and guard compilation on remaining stack - #15

fix(compiler): [OBE-10738] stop cloning the AST, and guard compilation on remaining stack#15
janmejay-s1 merged 6 commits into
Sentinel-One:mainfrom
JuanMantica45:obe-10738-stack-headroom-guard

JuanMantica45 commented Aug 27, 2026 •

edited

Loading

Uh oh!

janmejay-s1 left a comment

Uh oh!

janmejay-s1 Sep 10, 2026

Uh oh!

janmejay-s1 Sep 10, 2026

Uh oh!

janmejay-s1 Sep 10, 2026

Uh oh!

janmejay-s1 Sep 10, 2026

Uh oh!

janmejay-s1 Sep 10, 2026

Uh oh!

janmejay-s1 Sep 10, 2026

Uh oh!

janmejay-s1 Sep 10, 2026

Uh oh!

janmejay-s1 Sep 10, 2026

Uh oh!

janmejay-s1 Sep 10, 2026

Uh oh!

janmejay-s1 Sep 10, 2026

Uh oh!

janmejay-s1 left a comment

Uh oh!

JuanMantica45 commented Sep 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

JuanMantica45 commented Aug 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The bug

Where the stack actually goes

Fix, part 1 — remove the clone

Fix, part 2 — stop before the stack runs out

Fix, part 3 — clean up without recursing

Result

Dependency note — @jsbalis1

Test plan

Uh oh!

janmejay-s1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janmejay-s1 left a comment

Choose a reason for hiding this comment

Uh oh!

JuanMantica45 commented Sep 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

JuanMantica45 commented Aug 27, 2026 •

edited

Loading