From 3907b4df64b168713cd4f66e7d7ea276af2f5024 Mon Sep 17 00:00:00 2001 From: Juan Mantica Date: Thu, 27 Aug 2026 15:20:39 -0400 Subject: [PATCH 1/2] fix(compiler): [OBE-10738] stop cloning the AST, and guard compilation on remaining stack MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- Cargo.lock | 38 +++++++- Cargo.toml | 5 +- src/compiler/compiler.rs | 111 ++++++++++++++++++++++ src/compiler/mod.rs | 30 +++--- src/compiler/unused_expression_checker.rs | 26 ++++- 5 files changed, 193 insertions(+), 17 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 736746aec9..f082bee01c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -439,10 +439,11 @@ dependencies = [ [[package]] name = "cc" -version = "1.2.6" +version = "1.4.4" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8d6dbb628b8f8555f86d0323c2eb39e3ec81901f4b83e091db8a6a76d316a333" +checksum = "0ad534f4357a5264cce5019c989cf66a4f0dc4e0d1b1d15f8aacec0ff7360273" dependencies = [ + "find-msvc-tools", "jobserver", "libc", "shlex", @@ -1150,6 +1151,12 @@ version = "2.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "37909eebbb50d72f9059c3b6d82c0463f2ff062c9e95845c43a6c9c0355411be" +[[package]] +name = "find-msvc-tools" +version = "0.1.11" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d45db016d36b838f563236e9193d0ee6ce38f3f68b6c94e914b4929c96bbb890" + [[package]] name = "fixedbitset" version = "0.4.2" @@ -2531,6 +2538,15 @@ version = "2.0.11" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "33cb294fe86a74cbcf50d4445b37da762029549ebeea341421c7c70370f86cac" +[[package]] +name = "psm" +version = "0.1.26" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6e944464ec8536cd1beb0bbfd96987eb5e3b72f2ecdafdc5c769a37f1fa2ae1f" +dependencies = [ + "cc", +] + [[package]] name = "ptr_meta" version = "0.1.4" @@ -3046,9 +3062,9 @@ dependencies = [ [[package]] name = "shlex" -version = "1.3.0" +version = "2.0.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0fda2ff0d084019ba4d7c6f371c95d8fd75ce3524c3cb8fb653a3023f6323e64" +checksum = "f8fadd59c855ef2080decdef8ff161eb6661b86933c9d82e5ba29dc602a55aba" [[package]] name = "simdutf8" @@ -3148,6 +3164,19 @@ version = "1.2.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "a8f112729512f8e442d81f95a8a7ddf2b7c6b8a1a6f509a95864142b30cab2d3" +[[package]] +name = "stacker" +version = "0.1.21" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cddb07e32ddb770749da91081d8d0ac3a16f1a569a18b20348cd371f5dead06b" +dependencies = [ + "cc", + "cfg-if", + "libc", + "psm", + "windows-sys 0.52.0", +] + [[package]] name = "string_cache" version = "0.8.7" @@ -3789,6 +3818,7 @@ dependencies = [ "sha3", "snafu 0.8.5", "snap", + "stacker", "strip-ansi-escapes", "syslog_loose", "termcolor", diff --git a/Cargo.toml b/Cargo.toml index c68106f95b..a2bcbb2136 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -25,7 +25,7 @@ members = [ default = ["compiler", "value", "diagnostic", "path", "parser", "stdlib", "datadog", "core", "observo"] # Main features (on by default) -compiler = ["diagnostic", "path", "parser", "value", "dep:paste", "dep:chrono", "dep:serde", "dep:regex", "dep:bytes", "dep:ordered-float", "dep:chrono-tz", "dep:snafu", "dep:thiserror", "dep:dyn-clone", "dep:indoc", "dep:thiserror", "dep:lalrpop-util"] +compiler = ["diagnostic", "path", "parser", "value", "dep:paste", "dep:chrono", "dep:serde", "dep:regex", "dep:bytes", "dep:ordered-float", "dep:chrono-tz", "dep:snafu", "dep:thiserror", "dep:dyn-clone", "dep:indoc", "dep:thiserror", "dep:lalrpop-util", "dep:stacker"] value = ["path", "dep:bytes", "dep:regex", "dep:ordered-float", "dep:chrono", "dep:serde_json"] diagnostic = ["dep:codespan-reporting", "dep:termcolor"] path = ["value", "dep:once_cell", "dep:serde", "dep:snafu", "dep:regex"] @@ -163,6 +163,9 @@ ordered-float = { version = "4", default-features = false, optional = true } md-5 = { version = "0.10", optional = true } metrics = { version = "0.24.0", optional = true } metrics-util = { version = "0.19.0", optional = true } +# 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 } paste = { version = "1", default-features = false, optional = true } parse-size = { version = "1.1.0", optional = true } peeking_take_while = { version = "1", default-features = false, optional = true } diff --git a/src/compiler/compiler.rs b/src/compiler/compiler.rs index e14e6294d1..bbba8f3ea8 100644 --- a/src/compiler/compiler.rs +++ b/src/compiler/compiler.rs @@ -56,6 +56,10 @@ pub struct Compiler<'a> { // the error from the LHS) fallible_expression_error: Option, + /// Stack level below which `compile_expr` stops recursing, computed once from the stack + /// available when compilation started. `None` where the platform cannot report it. + stack_floor: Option, + config: CompileConfig, } @@ -67,6 +71,41 @@ pub(crate) enum CompilerError { ExpressionError(ExpressionError), } +/// Fraction of the stack available when compilation starts that is held back for the work still +/// owed once the guard fires: `Expr::type_info`'s walk back over the compiled subtree, and +/// unwinding. +/// +/// This is a *fraction*, not a byte count, because the reserve has to scale. `type_info` recurses +/// over the subtree built so far, so the deeper we let compilation go, the more stack the unwind +/// needs — and how deep we let it go is itself a function of the stack we started with. A fixed +/// byte reserve is therefore wrong at some stack size by construction: tuned for a 2 MiB release +/// build it overflows a debug build, whose frames are several times fatter. +/// +/// Reserving half is what measurement supports: compilation costs ~5,030 bytes per nesting level in +/// release and ~10,850 in debug (measured), and `type_info`'s walk back over the compiled subtree is a +/// large enough share of that per-level cost that a quarter proved insufficient in debug builds. +const STACK_RESERVE_FRACTION: usize = 2; + +/// A program nested deeply enough that compiling it would overflow the native stack. +/// +/// This is not a fixed nesting limit. How deeply a program may nest depends on the stack of the +/// thread compiling it, so the same program can be valid on one thread and rejected on another. +#[derive(Debug, thiserror::Error)] +#[error("recursion limit reached: not enough stack remaining to compile this level of expression nesting")] +pub(crate) struct StackExhaustionError; + +impl DiagnosticMessage for StackExhaustionError { + fn code(&self) -> usize { + 670 + } + + fn notes(&self) -> Vec { + vec![Note::Basic( + "reduce the nesting depth of this expression".to_owned(), + )] + } +} + impl CompilerError { fn to_diagnostic(&self) -> &dyn DiagnosticMessage { match self { @@ -102,6 +141,7 @@ impl<'a> Compiler<'a> { external_assignments: vec![], skip_missing_query_target: vec![], fallible_expression_error: None, + stack_floor: stacker::remaining_stack().map(|r| r / STACK_RESERVE_FRACTION), config, }; let expressions = compiler.compile_root_exprs(ast, &mut state); @@ -153,6 +193,21 @@ impl<'a> Compiler<'a> { Abort, Assignment, Container, FunctionCall, IfStatement, Literal, Op, Query, Return, Unary, Variable, }; + + // OBE-10738/OBE-10740: this function recurses once per expression-nesting level, so a + // crafted program can drive the native stack into its guard page — a SIGSEGV, not a + // catchable panic. Stop while there is still stack to fail gracefully in. + // + // `remaining_stack()` returns `None` where the platform cannot determine the bound; treat + // that as "proceed" so such platforms keep today's behaviour rather than rejecting + // everything. A program rejected here also never reaches `Expr::resolve`, whose recursion + // follows the same nesting (OBE-10740). + if let (Some(remaining), Some(floor)) = (stacker::remaining_stack(), self.stack_floor) { + if remaining < floor { + self.diagnostics.push(Box::new(StackExhaustionError)); + return None; + } + } let original_state = state.clone(); let span = node.span(); @@ -852,3 +907,59 @@ impl<'a> Compiler<'a> { self.skip_missing_query_target.push(query); } } + +#[cfg(test)] +mod tests { + /// Compiles `!!!…!true` at `depth` on a thread with exactly `stack` bytes. + /// Returns `Ok(())` if it compiled, `Err(())` if it was rejected with a diagnostic. + /// A stack overflow aborts the process instead of returning — which is the point. + fn compile_at(depth: usize, stack: usize) -> Result<(), ()> { + let src = "!".repeat(depth) + "true"; + std::thread::Builder::new() + .stack_size(stack) + .spawn(move || { + let fns = crate::stdlib::all(); + crate::compiler::compile(&src, &fns) + .map(|_| ()) + .map_err(|_| ()) + }) + .expect("spawn") + .join() + .expect("join") + } + + // OBE-10738: before the guard, this aborted the process — compilation costs ~5,030 bytes of + // stack per nesting level, so 1,000 levels needs ~5 MB and a 2 MiB thread cannot hold it. + #[test] + fn rejects_nesting_that_would_exhaust_the_stack() { + assert!( + compile_at(1_000, 2 * 1024 * 1024).is_err(), + "expected a diagnostic, not a compiled program" + ); + } + + // The other half of the pair, and the reason this is a headroom guard rather than a fixed + // depth cap: the *same* program is legitimate given a bigger stack. A `MAX_EXPR_DEPTH = 128` + // implementation fails this test. + #[test] + fn compiles_the_same_program_given_a_larger_stack() { + assert!( + compile_at(1_000, 32 * 1024 * 1024).is_ok(), + "expected the program to compile with ample stack" + ); + } + + // A fixed cap of 128 would also have been unsafe in the other direction: a 512 KiB thread + // holds only ~104 levels. The guard adapts instead of rejecting or overflowing. + #[test] + fn adapts_to_a_small_stack() { + assert!(compile_at(200, 512 * 1024).is_err()); + assert!(compile_at(20, 512 * 1024).is_ok()); + } + + // Ordinary programs must be unaffected on the 2 MiB stack tokio gives its workers. + #[test] + fn leaves_ordinary_nesting_alone() { + assert!(compile_at(64, 2 * 1024 * 1024).is_ok()); + } +} diff --git a/src/compiler/mod.rs b/src/compiler/mod.rs index c39146cd5e..99555eac8a 100644 --- a/src/compiler/mod.rs +++ b/src/compiler/mod.rs @@ -109,17 +109,25 @@ pub fn compile_with_state( let ast = parse(source) .map_err(|err| crate::diagnostic::DiagnosticList::from(vec![Box::new(err) as Box<_>]))?; - let unused_expression_check_enabled = config.unused_expression_check_enabled(); - let result = Compiler::compile(fns, ast.clone(), state, config); - - if unused_expression_check_enabled { - let unused_warnings = check_for_unused_results(&ast); - if !unused_warnings.is_empty() { - return result.map(|mut compilation_result| { - compilation_result.warnings.extend(unused_warnings); - compilation_result - }); - } + // OBE-10738: this used to be `Compiler::compile(fns, ast.clone(), ..)` so that `ast` survived + // for the unused-expression check below. `Clone` on the AST is derived, so it recursed once + // per expression-nesting level and overflowed the native stack on a deeply-nested program — + // before the compiler ran, and so before any guard inside it could fire. Running the check + // first lets the AST be moved into the compiler instead of copied, which removes that + // recursion entirely and saves cloning the whole tree on every single compile. + let unused_warnings = if config.unused_expression_check_enabled() { + check_for_unused_results(&ast) + } else { + DiagnosticList::default() + }; + + let result = Compiler::compile(fns, ast, state, config); + + if !unused_warnings.is_empty() { + return result.map(|mut compilation_result| { + compilation_result.warnings.extend(unused_warnings); + compilation_result + }); } result diff --git a/src/compiler/unused_expression_checker.rs b/src/compiler/unused_expression_checker.rs index 701888bbbc..344090cabd 100644 --- a/src/compiler/unused_expression_checker.rs +++ b/src/compiler/unused_expression_checker.rs @@ -33,6 +33,12 @@ use tracing::warn; const SIDE_EFFECT_FUNCTIONS: [&str; 5] = ["del", "log", "assert", "assert_eq", "set_semantic_meaning"]; +/// 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; + #[must_use] pub fn check_for_unused_results(ast: &Program) -> DiagnosticList { let expression_visitor = AstVisitor { ast }; @@ -58,6 +64,10 @@ struct VisitorState { ident_to_state: BTreeMap, visiting_closure: bool, diagnostics: DiagnosticList, + + /// Stack level below which `visit_node` stops descending. `None` where the platform cannot + /// report remaining stack, in which case the walk behaves as it always has. + stack_floor: Option, } impl VisitorState { @@ -190,6 +200,17 @@ fn scoped_visit(state: &mut VisitorState, f: impl FnOnce(&mut VisitorState)) { impl AstVisitor<'_> { fn visit_node(&self, node: &Node, state: &mut VisitorState) { + // OBE-10738: this walk recurses once per expression-nesting level, on the raw AST and + // before the compiler's own guard can apply. It only produces advisory warnings, so when + // 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) { + if remaining < floor { + return; + } + } + let expression = node.inner(); match expression { @@ -412,7 +433,10 @@ impl AstVisitor<'_> { /// * Unused Expressions: an expression without side-effects with an unused result fn check_for_unused_results(&self) -> DiagnosticList { 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), + ..VisitorState::default() + }; let root_expressions = &self.ast.0; for (i, root_node) in root_expressions.iter().enumerate() { let is_last = i == root_expressions.len() - 1; From 5b31ea7418b8211aa4640ce3eee39091b56b041f Mon Sep 17 00:00:00 2001 From: Juan Mantica Date: Thu, 27 Aug 2026 15:51:18 -0400 Subject: [PATCH 2/2] fix(compiler): [OBE-10738] tear the un-compiled AST down iteratively when the guard fires MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- src/compiler/ast_teardown.rs | 192 +++++++++++++++++++++++++++++++++++ src/compiler/compiler.rs | 15 +++ src/compiler/mod.rs | 2 + 3 files changed, 209 insertions(+) create mode 100644 src/compiler/ast_teardown.rs diff --git a/src/compiler/ast_teardown.rs b/src/compiler/ast_teardown.rs new file mode 100644 index 0000000000..688aabc17d --- /dev/null +++ b/src/compiler/ast_teardown.rs @@ -0,0 +1,192 @@ +//! Dropping an AST subtree without recursing. +//! +//! When the stack guard in `compile_expr` 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 actually stopped. So the guard would fire correctly and then the process +//! would die cleaning up, which is exactly what it was trying to prevent. +//! +//! This tears the subtree down against an explicit heap worklist instead: pop a node, move its +//! children onto the worklist, let the childless remainder drop. Nothing recurses, so no depth of +//! nesting can exhaust the stack. +//! +//! Every `match` here is exhaustive on purpose — no `_` arm. A new AST variant must fail to +//! compile rather than silently reintroduce a recursive drop path. + +use crate::parser::ast::Node; +use crate::parser::ast::{ + Abort, Assignment, Container, Expr, FunctionCall, FunctionClosure, IfStatement, Op, Predicate, + Query, QueryTarget, Return, Unary, +}; + +/// Drops `root` and everything below it iteratively. +pub(super) fn drop_expr(root: Node) { + let mut worklist = vec![root.into_inner()]; + while let Some(expr) = worklist.pop() { + push_children(expr, &mut worklist); + } +} + +/// Moves every `Expr` directly beneath `expr` onto `worklist`. Whatever is left of `expr` — spans, +/// identifiers, operators — is shallow and drops normally when this returns. +fn push_children(expr: Expr, worklist: &mut Vec) { + match expr { + // Leaves. A string literal can hold template segments, but those are identifiers, not + // expressions, so there is nothing nested to unwind here. + Expr::Literal(_) | Expr::Variable(_) => {} + + Expr::Container(node) => push_container(node.into_inner(), worklist), + + Expr::IfStatement(node) => { + let IfStatement { + predicate, + if_node, + else_node, + } = node.into_inner(); + push_predicate(predicate.into_inner(), worklist); + push_block(if_node.into_inner(), worklist); + if let Some(block) = else_node { + push_block(block.into_inner(), worklist); + } + } + + Expr::Op(node) => { + let Op(lhs, _opcode, rhs) = node.into_inner(); + push_boxed(lhs, worklist); + push_boxed(rhs, worklist); + } + + Expr::Assignment(node) => match node.into_inner() { + Assignment::Single { expr, .. } | Assignment::Infallible { expr, .. } => { + push_boxed(expr, worklist); + } + }, + + Expr::Query(node) => { + let Query { target, path: _ } = node.into_inner(); + match target.into_inner() { + QueryTarget::Internal(_) | QueryTarget::External(_) => {} + QueryTarget::FunctionCall(call) => push_function_call(call, worklist), + QueryTarget::Container(container) => push_container(container, worklist), + } + } + + Expr::FunctionCall(node) => push_function_call(node.into_inner(), worklist), + + Expr::Unary(node) => match node.into_inner() { + Unary::Not(not) => { + let (_span, expr) = not.into_inner().take(); + push_boxed(expr, worklist); + } + }, + + Expr::Abort(node) => { + let Abort { message } = node.into_inner(); + if let Some(expr) = message { + push_boxed(expr, worklist); + } + } + + Expr::Return(node) => { + let Return { expr } = node.into_inner(); + push_boxed(expr, worklist); + } + } +} + +fn push_container(container: Container, worklist: &mut Vec) { + match container { + Container::Group(group) => worklist.push((*group).into_inner().into_inner().into_inner()), + Container::Block(block) => push_block(block.into_inner(), worklist), + Container::Array(array) => { + worklist.extend(array.into_inner().0.into_iter().map(Node::into_inner)); + } + Container::Object(object) => { + worklist.extend(object.into_inner().0.into_values().map(Node::into_inner)); + } + } +} + +fn push_block(block: crate::parser::ast::Block, worklist: &mut Vec) { + worklist.extend(block.into_inner().into_iter().map(Node::into_inner)); +} + +fn push_predicate(predicate: Predicate, worklist: &mut Vec) { + match predicate { + Predicate::One(expr) => push_boxed(expr, worklist), + Predicate::Many(exprs) => worklist.extend(exprs.into_iter().map(Node::into_inner)), + } +} + +fn push_function_call(call: FunctionCall, worklist: &mut Vec) { + let FunctionCall { + ident: _, + abort_on_error: _, + arguments, + closure, + } = call; + + worklist.extend( + arguments + .into_iter() + .map(|argument| argument.into_inner().expr.into_inner()), + ); + + if let Some(closure) = closure { + let FunctionClosure { + variables: _, + block, + } = closure.into_inner(); + push_block(block.into_inner(), worklist); + } +} + +fn push_boxed(expr: Box>, worklist: &mut Vec) { + worklist.push((*expr).into_inner()); +} + +#[cfg(test)] +mod tests { + use super::*; + + /// Parses `source` on a generous stack, then tears the AST down on a deliberately small one. + /// Returns normally only if the teardown never recursed; a recursive drop aborts the process. + fn parse_then_drop_on_small_stack(source: String, stack: usize) { + let program = std::thread::Builder::new() + .stack_size(256 * 1024 * 1024) + .spawn(move || crate::parser::parse(&source).expect("parse")) + .expect("spawn") + .join() + .expect("join"); + + std::thread::Builder::new() + .stack_size(stack) + .spawn(move || { + for root in program.0 { + if let crate::parser::ast::RootExpr::Expr(expr) = root.into_inner() { + drop_expr(expr); + } + } + }) + .expect("spawn") + .join() + .expect("join"); + } + + #[test] + fn tears_down_deeply_nested_unary_without_recursing() { + parse_then_drop_on_small_stack("!".repeat(30_000) + "true", 512 * 1024); + } + + #[test] + fn tears_down_deeply_nested_containers_without_recursing() { + let depth = 10_000; + parse_then_drop_on_small_stack("[".repeat(depth) + &"]".repeat(depth), 512 * 1024); + } + + #[test] + fn tears_down_deeply_nested_groups_without_recursing() { + let depth = 5_000; + parse_then_drop_on_small_stack("(".repeat(depth) + "true" + &")".repeat(depth), 512 * 1024); + } +} diff --git a/src/compiler/compiler.rs b/src/compiler/compiler.rs index bbba8f3ea8..b69322294a 100644 --- a/src/compiler/compiler.rs +++ b/src/compiler/compiler.rs @@ -205,6 +205,10 @@ impl<'a> Compiler<'a> { if let (Some(remaining), Some(floor)) = (stacker::remaining_stack(), self.stack_floor) { if remaining < floor { self.diagnostics.push(Box::new(StackExhaustionError)); + // We still own everything below this point. Dropping it normally would run the + // derived drop glue, which recurses per nesting level and would overflow the very + // stack this guard just protected — so unwind it against a heap worklist instead. + super::ast_teardown::drop_expr(node); return None; } } @@ -957,6 +961,17 @@ mod tests { assert!(compile_at(20, 512 * 1024).is_ok()); } + // The guard bails while still holding the entire un-compiled remainder of the program. + // Dropping that with the derived glue is what used to kill the process *after* the guard had + // correctly fired, because it costs stack proportional to the program's depth rather than to + // where compilation stopped. Tearing it down iteratively is therefore load-bearing, and this + // must hold at any depth whatsoever. + #[test] + fn rejects_pathological_nesting_without_dying_in_cleanup() { + assert!(compile_at(50_000, 2 * 1024 * 1024).is_err()); + assert!(compile_at(50_000, 512 * 1024).is_err()); + } + // Ordinary programs must be unaffected on the 2 MiB stack tokio gives its workers. #[test] fn leaves_ordinary_nesting_alone() { diff --git a/src/compiler/mod.rs b/src/compiler/mod.rs index 99555eac8a..600d9c8b70 100644 --- a/src/compiler/mod.rs +++ b/src/compiler/mod.rs @@ -56,6 +56,8 @@ pub use self::deprecation_warning::DeprecationWarning; #[allow(clippy::module_inception)] mod compiler; +mod ast_teardown; + mod compile_config; mod context; mod datetime;