diff --git a/Cargo.lock b/Cargo.lock index 736746aec..f082bee01 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 c68106f95..a2bcbb213 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/ast_teardown.rs b/src/compiler/ast_teardown.rs new file mode 100644 index 000000000..688aabc17 --- /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 e14e6294d..b69322294 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,25 @@ 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)); + // 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; + } + } let original_state = state.clone(); let span = node.span(); @@ -852,3 +911,70 @@ 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()); + } + + // 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() { + assert!(compile_at(64, 2 * 1024 * 1024).is_ok()); + } +} diff --git a/src/compiler/mod.rs b/src/compiler/mod.rs index c39146cd5..600d9c8b7 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; @@ -109,17 +111,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); + // 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_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 - }); - } + 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 701888bbb..344090cab 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;