From b7cbd094c5531f639cea459c4ad884e564c2089d Mon Sep 17 00:00:00 2001 From: fmcmac Date: Fri, 10 Jul 2026 14:02:30 +1200 Subject: [PATCH 1/3] Apply recursive-protection to the Spanned::span walk MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `Spanned::span` implementations are plain unbounded recursion — one stack frame per AST node. Computing the span of a deeply nested AST (a deep expression tree, nested subqueries, or a long left-associative boolean chain) therefore recurses as deep as the AST and can overflow the thread stack, aborting the process. A query can parse successfully — the parser is already guarded — and then overflow later when its span is computed (e.g. during query planning by a downstream consumer). The parser already opts into stack-overflow protection behind the `recursive-protection` feature (`recursive::recursive`, backed by `stacker::maybe_grow`); the span walk was simply never given the same treatment. Annotate the recursive `span()` implementations for the container nodes (`Expr`, `SetExpr`, `Query`, `Select`, `SelectItem`, `TableFactor`, `Statement`) so at least one node on every deep-recursion cycle grows the stack on demand. No new dependency and a no-op when the feature is disabled. Adds a regression test that walks a deeply nested AST on a small thread stack: it overflows without the annotations and completes with them. Signed-off-by: fmcmac --- src/ast/spans.rs | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/src/ast/spans.rs b/src/ast/spans.rs index 6505b209e..9cfffadce 100644 --- a/src/ast/spans.rs +++ b/src/ast/spans.rs @@ -113,6 +113,7 @@ impl Spanned for Parens { } impl Spanned for Query { + #[cfg_attr(feature = "recursive-protection", recursive::recursive)] fn span(&self) -> Span { let Query { with, @@ -217,6 +218,7 @@ impl Spanned for Cte { /// /// [SetExpr::Table] is not implemented. impl Spanned for SetExpr { + #[cfg_attr(feature = "recursive-protection", recursive::recursive)] fn span(&self) -> Span { match self { SetExpr::Select(select) => select.span(), @@ -320,6 +322,7 @@ impl Spanned for Values { /// - [Statement::Unload] /// - [Statement::OptimizeTable] impl Spanned for Statement { + #[cfg_attr(feature = "recursive-protection", recursive::recursive)] fn span(&self) -> Span { match self { Statement::Analyze(analyze) => analyze.span(), @@ -1480,6 +1483,7 @@ impl Spanned for AssignmentTarget { /// - [Expr::Map] # DuckDB specific /// - [Expr::Lambda] impl Spanned for Expr { + #[cfg_attr(feature = "recursive-protection", recursive::recursive)] fn span(&self) -> Span { match self { Expr::Identifier(ident) => ident.span, @@ -1852,6 +1856,7 @@ impl Spanned for SelectItemQualifiedWildcardKind { } impl Spanned for SelectItem { + #[cfg_attr(feature = "recursive-protection", recursive::recursive)] fn span(&self) -> Span { match self { SelectItem::UnnamedExpr(expr) => expr.span(), @@ -1958,6 +1963,7 @@ impl Spanned for ReplaceSelectElement { /// Missing spans: /// - [TableFactor::JsonTable] impl Spanned for TableFactor { + #[cfg_attr(feature = "recursive-protection", recursive::recursive)] fn span(&self) -> Span { match self { TableFactor::Table { @@ -2303,6 +2309,7 @@ impl Spanned for TableWithJoins { } impl Spanned for Select { + #[cfg_attr(feature = "recursive-protection", recursive::recursive)] fn span(&self) -> Span { let Select { select_token, @@ -3116,4 +3123,34 @@ WHERE id = 1 Span::new(Location::new(2, 8), Location::new(4, 52)) ); } + // Regression: the `Spanned::span` walk is otherwise unbounded recursion, so + // a deeply nested expression (the shape a machine-generated query or a broad + // access-control predicate can produce) overflows the stack and aborts the + // process. With the `recursive-protection` feature the walk grows the stack + // on demand and completes. + // + // The AST is built directly (the parser is not the subject here) and walked + // on a deliberately small (512 KiB) thread stack, so the chosen depth far + // exceeds what the stack holds unprotected — the walk overflows without the + // fix and completes with it. + #[test] + fn test_deeply_nested_expr_span_does_not_overflow() { + std::thread::Builder::new() + .stack_size(512 * 1024) + .spawn(|| { + let depth = 200_000; + let mut expr = Expr::Identifier(crate::ast::Ident::new("x")); + for _ in 0..depth { + expr = Expr::Nested(Box::new(expr)); + } + // Must not overflow the stack while walking the deep AST. + let _ = expr.span(); + // The nested `Box` chain has a recursive `Drop`; forget it + // to keep the test fast and isolate the span walk under test. + core::mem::forget(expr); + }) + .unwrap() + .join() + .expect("span walk overflowed the small-stack thread"); + } } From 21ee610626d440338f3f953f12dcb34b677c5388 Mon Sep 17 00:00:00 2001 From: fmcmac Date: Wed, 15 Jul 2026 09:05:13 +1200 Subject: [PATCH 2/3] Fix clippy lints for -D warnings Resolve 7 clippy errors that fail the lint CI job under a newer toolchain: - useless_borrows_in_formatting: drop redundant `&` in write!/format! args (ast/ddl.rs, ast/query.rs, ast/mod.rs, parser/mod.rs x2) - needless_return_with_question_mark: drop redundant `?` on returned Err/parser_err! in parser/mod.rs x2 Co-Authored-By: Claude Opus 4.8 (1M context) --- src/ast/ddl.rs | 2 +- src/ast/mod.rs | 2 +- src/ast/query.rs | 2 +- src/parser/mod.rs | 8 ++++---- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/ast/ddl.rs b/src/ast/ddl.rs index 346627c68..ece602373 100644 --- a/src/ast/ddl.rs +++ b/src/ast/ddl.rs @@ -4780,7 +4780,7 @@ impl fmt::Display for AlterTable { if self.only { write!(f, "ONLY ")?; } - write!(f, "{} ", &self.name)?; + write!(f, "{} ", self.name)?; if let Some(cluster) = &self.on_cluster { write!(f, "ON CLUSTER {cluster} ")?; } diff --git a/src/ast/mod.rs b/src/ast/mod.rs index 88883cfbb..5bc7070e8 100644 --- a/src/ast/mod.rs +++ b/src/ast/mod.rs @@ -11842,7 +11842,7 @@ impl fmt::Display for AlterUser { let has_props = !self.set_props.options.is_empty(); if has_props { write!(f, " SET")?; - write!(f, " {}", &self.set_props)?; + write!(f, " {}", self.set_props)?; } if !self.unset_props.is_empty() { write!(f, " UNSET {}", display_comma_separated(&self.unset_props))?; diff --git a/src/ast/query.rs b/src/ast/query.rs index eb209228e..fdcacddf7 100644 --- a/src/ast/query.rs +++ b/src/ast/query.rs @@ -3545,7 +3545,7 @@ pub struct LockClause { impl fmt::Display for LockClause { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - write!(f, "FOR {}", &self.lock_type)?; + write!(f, "FOR {}", self.lock_type)?; if let Some(ref of) = self.of { write!(f, " OF {of}")?; } diff --git a/src/parser/mod.rs b/src/parser/mod.rs index 8b998b021..5cbbc09f1 100644 --- a/src/parser/mod.rs +++ b/src/parser/mod.rs @@ -4845,7 +4845,7 @@ impl<'a> Parser<'a> { if self.parse_keyword(expected) { Ok(self.get_current_token().clone()) } else { - self.expected_ref(format!("{:?}", &expected).as_str(), self.peek_token_ref()) + self.expected_ref(format!("{:?}", expected).as_str(), self.peek_token_ref()) } } @@ -4858,7 +4858,7 @@ impl<'a> Parser<'a> { if self.parse_keyword(expected) { Ok(()) } else { - self.expected_ref(format!("{:?}", &expected).as_str(), self.peek_token_ref()) + self.expected_ref(format!("{:?}", expected).as_str(), self.peek_token_ref()) } } @@ -13726,7 +13726,7 @@ impl<'a> Parser<'a> { token => { return Err(ParserError::ParserError(format!( "Unexpected token in identifier: {token}" - )))?; + ))); } } } @@ -17018,7 +17018,7 @@ impl<'a> Parser<'a> { tok.token ), tok.span.start - )?; + ); } } From c6175fe15cc1cbba810853fc771108c8ee79545e Mon Sep 17 00:00:00 2001 From: fmcmac Date: Wed, 15 Jul 2026 09:13:32 +1200 Subject: [PATCH 3/3] Fix remaining clippy useless_borrows_in_formatting in examples/tests The lib-only fixes let clippy proceed past the library to --all-targets, surfacing the same lint in examples and tests under clippy 0.1.97: - examples/cli.rs: drop redundant `&` in println!/panic! args - tests/sqlparser_postgres.rs x2, tests/sqlparser_common.rs x3: drop redundant `&str_op` in format! args Co-Authored-By: Claude Opus 4.8 (1M context) --- examples/cli.rs | 4 ++-- tests/sqlparser_common.rs | 6 +++--- tests/sqlparser_postgres.rs | 4 ++-- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/examples/cli.rs b/examples/cli.rs index 3c4299b20..51a63a7ea 100644 --- a/examples/cli.rs +++ b/examples/cli.rs @@ -71,9 +71,9 @@ $ cargo run --example cli - [--dialectname] .expect("failed to read from stdin"); String::from_utf8(buf).expect("stdin content wasn't valid utf8") } else { - println!("Parsing from file '{}' using {:?}", &filename, dialect); + println!("Parsing from file '{}' using {:?}", filename, dialect); fs::read_to_string(&filename) - .unwrap_or_else(|_| panic!("Unable to read the file {}", &filename)) + .unwrap_or_else(|_| panic!("Unable to read the file {}", filename)) }; let without_bom = if contents.chars().next().unwrap() as u64 != 0xfeff { contents.as_str() diff --git a/tests/sqlparser_common.rs b/tests/sqlparser_common.rs index 634b9aea2..50832d062 100644 --- a/tests/sqlparser_common.rs +++ b/tests/sqlparser_common.rs @@ -1731,7 +1731,7 @@ fn parse_json_ops_without_colon() { ]; for (str_op, op, dialects) in binary_ops { - let select = dialects.verified_only_select(&format!("SELECT a {} b", &str_op)); + let select = dialects.verified_only_select(&format!("SELECT a {} b", str_op)); assert_eq!( SelectItem::UnnamedExpr(Expr::BinaryOp { left: Box::new(Expr::Identifier(Ident::new("a"))), @@ -2441,7 +2441,7 @@ fn parse_bitwise_ops() { ]; for (str_op, op, dialects) in bitwise_ops { - let select = dialects.verified_only_select(&format!("SELECT a {} b", &str_op)); + let select = dialects.verified_only_select(&format!("SELECT a {} b", str_op)); assert_eq!( SelectItem::UnnamedExpr(Expr::BinaryOp { left: Box::new(Expr::Identifier(Ident::new("a"))), @@ -19100,7 +19100,7 @@ fn parse_generic_unary_ops() { ("+", UnaryOperator::Plus), ]; for (str_op, op) in unary_ops { - let select = verified_only_select(&format!("SELECT {}expr", &str_op)); + let select = verified_only_select(&format!("SELECT {}expr", str_op)); assert_eq!( UnnamedExpr(UnaryOp { op: *op, diff --git a/tests/sqlparser_postgres.rs b/tests/sqlparser_postgres.rs index 47e60e07d..9ee6ab3d6 100644 --- a/tests/sqlparser_postgres.rs +++ b/tests/sqlparser_postgres.rs @@ -2574,7 +2574,7 @@ fn parse_pg_unary_ops() { ("@", UnaryOperator::PGAbs), ]; for (str_op, op) in pg_unary_ops { - let select = pg().verified_only_select(&format!("SELECT {}a", &str_op)); + let select = pg().verified_only_select(&format!("SELECT {}a", str_op)); assert_eq!( SelectItem::UnnamedExpr(Expr::UnaryOp { op: *op, @@ -2590,7 +2590,7 @@ fn parse_pg_postfix_factorial() { let postfix_factorial = &[("!", UnaryOperator::PGPostfixFactorial)]; for (str_op, op) in postfix_factorial { - let select = pg().verified_only_select(&format!("SELECT a{}", &str_op)); + let select = pg().verified_only_select(&format!("SELECT a{}", str_op)); assert_eq!( SelectItem::UnnamedExpr(Expr::UnaryOp { op: *op,