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

[JuanMantica45]

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

Loading

Two things follow immediately:

1. 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.
2. 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

Loading

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

Loading

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<Expr> 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

Loading

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