Close the remaining gaps in the parser recursion counter

src/parser/mod.rs

@@ -442,6 +442,24 @@ impl<'a> Parser<'a> {
     /// # }
     /// ```
     ///
+    /// The limit is only meaningful where it is actually charged. Every cycle in
+    /// the parser's call graph must pass through a `try_decrease` site, or input
+    /// driving that cycle recurses with no accounting and the limit is inert for
+    /// it. The counted set is deliberately the minimum that cuts every cycle:
+    /// [`Self::parse_statement`], [`Self::parse_subexpr`], [`Self::parse_query`],
+    /// [`Self::parse_table_factor`], `parse_data_type_helper`, [`Self::parse_interval`],
+    /// `parse_pattern`, [`Self::parse_json_table_column_def`],
+    /// `parse_key_value_options` and `parse_joins`. One further cycle is
+    /// intentionally uncounted because it cannot recur on input — see the comment
+    /// on `parse_remaining_set_exprs`. Adding a recursive path that reaches none
+    /// of these reintroduces the gap.
+    ///
+    /// Note also that reaching the limit itself costs stack: the deepest chain
+    /// (`parse_prefix` -> `parse_cast_expr` -> `parse_expr` -> `parse_subexpr`) runs
+    /// tens of KB per level in an unoptimised build, so the default of 50 wants a
+    /// few MB of headroom to deny cleanly. Callers running on small stacks should
+    /// lower the limit rather than rely on the default.
+    ///
     /// Note: when "recursive-protection" feature is enabled, this crate uses additional stack overflow protection
     //  for some of its recursive methods. See [`recursive::recursive`] for more information.
     pub fn with_recursion_limit(mut self, recursion_limit: usize) -> Self {
@@ -1895,6 +1913,18 @@ impl<'a> Parser<'a> {
                     Err(e) => {
                         self.failed_reserved_word_prefix_positions
                             .insert(next_token_index, (&e).into());
+                        // A recursion-limit error is not "this word meant
+                        // something else" -- it is "we ran out of depth". Retrying
+                        // the same span under a second interpretation re-descends
+                        // the same input, once per level, which turns a bounded
+                        // denial into exponential backtracking. It also silently
+                        // changes the AST when the retry happens to succeed, so the
+                        // same SQL parses differently at different recursion limits.
+                        // `maybe_parse` re-raises this error precisely so callers
+                        // can propagate it; do not discard it here.
+                        if matches!(e, ParserError::RecursionLimitExceeded) {
+                            return Err(e);
+                        }
                         if !self.dialect.is_reserved_for_identifier(w.keyword) {
                             if let Ok(Some(expr)) = self.maybe_parse(|parser| {
                                 parser.parse_expr_prefix_by_unreserved_word(&w, span)
@@ -12766,6 +12796,12 @@ impl<'a> Parser<'a> {
     fn parse_data_type_helper(
         &mut self,
     ) -> Result<(DataType, MatchedTrailingBracket), ParserError> {
+        // The guard belongs HERE and not on the public `parse_data_type`: the
+        // recursive type constructors are not all routed through it. `ARRAY<..>`
+        // and `STRUCT<..>` recurse into `parse_data_type_helper` directly (to
+        // thread `MatchedTrailingBracket` through), so a guard on
+        // `parse_data_type` alone would leave the angle-bracket forms uncounted.
+        let _guard = self.recursion_counter.try_decrease()?;
         let dialect = self.dialect;
         self.advance_token();
         let next_token = self.get_current_token();
@@ -15093,6 +15129,15 @@ impl<'a> Parser<'a> {
     /// Parse any extra set expressions that may be present in a query body
     ///
     /// (this is its own function to reduce required stack size in debug builds)
+    ///
+    /// Not counted, deliberately. This forms a cycle with [`Self::parse_query_body`],
+    /// but it cannot recur without bound on attacker-shaped input: the left side is
+    /// consumed by the `loop` below (so `A UNION B UNION C ..` is iterative, at any
+    /// length), and the right side only recurses when the next operator binds more
+    /// tightly. There are exactly two set-operation precedences (10 for
+    /// UNION/EXCEPT/MINUS, 20 for INTERSECT) and the recursive call passes the
+    /// higher one, so `precedence >= next_precedence` breaks immediately at the
+    /// next level. Depth is bounded by the precedence ladder at 2, not by input.
     fn parse_remaining_set_exprs(
         &mut self,
         mut expr: SetExpr,
@@ -16226,6 +16271,16 @@ impl<'a> Parser<'a> {
     }
 
     fn parse_joins(&mut self) -> Result<Vec<Join>, ParserError> {
+        // Self-recursive through the parens-less nested-join branch below, on
+        // dialects where `supports_left_associative_joins_without_parens` is
+        // false (Snowflake). `SELECT * FROM t JOIN t JOIN t ..` drives it.
+        //
+        // It is tempting to argue this is already charged because reaching the
+        // recursive call requires a `parse_table_factor`, which takes a guard.
+        // That is wrong: `DepthGuard` releases on drop, and `parse_table_factor`
+        // has already returned by the time we recurse, so its guard is not live
+        // and nothing accumulates. Count it here.
+        let _guard = self.recursion_counter.try_decrease()?;
         let mut joins = vec![];
         loop {
             let global = self.parse_keyword(Keyword::GLOBAL);
@@ -17348,6 +17403,11 @@ impl<'a> Parser<'a> {
     }
 
     fn parse_pattern(&mut self) -> Result<MatchRecognizePattern, ParserError> {
+        // MATCH_RECOGNIZE patterns are their own recursive-descent grammar
+        // (pattern -> concat -> repetition -> base -> pattern) reached from
+        // `parse_match_recognize`. Nothing on that cycle is counted, and nested
+        // parentheses inside PATTERN(..) drive it.
+        let _guard = self.recursion_counter.try_decrease()?;
         let pattern = self.parse_concat_pattern()?;
         if self.consume_token(&Token::Pipe) {
             match self.parse_pattern()? {
@@ -17414,6 +17474,9 @@ impl<'a> Parser<'a> {
     /// Parses MySQL's JSON_TABLE column definition.
     /// For example: `id INT EXISTS PATH '$' DEFAULT '0' ON EMPTY ERROR ON ERROR`
     pub fn parse_json_table_column_def(&mut self) -> Result<JsonTableColumn, ParserError> {
+        // `NESTED PATH '$' COLUMNS(..)` nests column definitions inside column
+        // definitions, so this function is directly self-recursive on input.
+        let _guard = self.recursion_counter.try_decrease()?;
         if self.parse_keyword(Keyword::NESTED) {
             let _has_path_keyword = self.parse_keyword(Keyword::PATH);
             let path = self.parse_value()?;
@@ -21083,6 +21146,10 @@ impl<'a> Parser<'a> {
         parenthesized: bool,
         end_words: &[Keyword],
     ) -> Result<KeyValueOptions, ParserError> {
+        // Option values may themselves be parenthesised option lists
+        // (`parse_key_value_option` calls back into this function), so nesting
+        // in e.g. Snowflake's `COPY_OPTIONS=(a=(b=(..)))` recurses on input.
+        let _guard = self.recursion_counter.try_decrease()?;
         let mut options: Vec<KeyValueOption> = Vec::new();
         let mut delimiter = KeyValueOptionsDelimiter::Space;
         if parenthesized {

tests/sqlparser_common.rs

@@ -11419,6 +11419,274 @@ fn parse_deeply_nested_subquery_expr_hits_recursion_limits() {
     assert_eq!(res, Err(ParserError::RecursionLimitExceeded));
 }
 
+/// Parse `sql` on a thread with a bounded stack and a hard timeout.
+///
+/// These shapes are bounded by *remaining stack*, not by input size, so an
+/// unguarded recursion does not fail — it wedges. Running the parse on its own
+/// thread with a receive timeout means a regression in the depth guard fails the
+/// test loudly and in bounded time instead of hanging the test process.
+///
+/// A successful parse is `mem::forget`-ed rather than dropped: the derived `Drop`
+/// on the AST is itself recursive, so dropping a deep tree performs exactly the
+/// stack descent this test exists to prevent.
+///
+/// The stack is deliberately *generous* rather than tight. What is under test is
+/// the depth contract, and reaching `DEFAULT_REMAINING_DEPTH` at all costs real
+/// stack: the `parse_prefix -> parse_cast_expr -> parse_expr -> parse_subexpr`
+/// chain runs ~85 KB per level in an unoptimised build, so a full 50 levels needs
+/// roughly 5 MB before the guard can fire. A tighter stack would make this test
+/// fail on the debug frame budget instead of on the property it asserts. The
+/// loudness comes from the assertion and the timeout, not from starving the stack.
+fn parse_depth_bounded(name: &str, sql: String) -> Result<(), ParserError> {
+    let (tx, rx) = std::sync::mpsc::channel();
+    std::thread::Builder::new()
+        .stack_size(16 * 1024 * 1024)
+        .spawn(move || {
+            let dialect = GenericDialect {};
+            let result = Parser::new(&dialect)
+                .try_with_sql(&sql)
+                .expect("tokenize to work")
+                .parse_statements();
+            let _ = tx.send(match result {
+                Ok(ast) => {
+                    std::mem::forget(ast);
+                    Ok(())
+                }
+                Err(e) => Err(e),
+            });
+        })
+        .expect("spawn to work");
+
+    rx.recv_timeout(std::time::Duration::from_secs(20))
+        .unwrap_or_else(|_| {
+            panic!("{name}: parser did not return within 20s; the depth guard has regressed")
+        })
+}
+
+/// Every recursive-descent shape that reaches an uncounted cycle must deny with
+/// `RecursionLimitExceeded`, not run to stack exhaustion.
+///
+/// Regression test for the audit in zuru-federated-query#606. Each entry drives a
+/// distinct cycle; see the comments at the corresponding `try_decrease` sites.
+#[test]
+fn deeply_nested_recursive_shapes_hit_recursion_limits() {
+    // depth far past DEFAULT_REMAINING_DEPTH (50), but only a few KB of SQL
+    let n = 2000;
+    let cases: Vec<(&str, String)> = vec![
+        // --- recursive DataType constructors, via CAST ---
+        (
+            "Nullable",
+            format!(
+                "SELECT CAST(1 AS {}INT{})",
+                "Nullable(".repeat(n),
+                ")".repeat(n)
+            ),
+        ),
+        (
+            "LowCardinality",
+            format!(
+                "SELECT CAST(1 AS {}INT{})",
+                "LowCardinality(".repeat(n),
+                ")".repeat(n)
+            ),
+        ),
+        (
+            "Map",
+            format!(
+                "SELECT CAST(1 AS {}INT{})",
+                "Map(INT, ".repeat(n),
+                ")".repeat(n)
+            ),
+        ),
+        (
+            "Tuple",
+            format!(
+                "SELECT CAST(1 AS {}INT{})",
+                "Tuple(a ".repeat(n),
+                ")".repeat(n)
+            ),
+        ),
+        (
+            "Nested",
+            format!(
+                "SELECT CAST(1 AS {}INT{})",
+                "NESTED(a ".repeat(n),
+                ")".repeat(n)
+            ),
+        ),
+        // Angle-bracket forms recurse into `parse_data_type_helper` *directly*,
+        // bypassing `parse_data_type`. A guard placed on `parse_data_type` alone
+        // would not catch these two — which is why the guard is on the helper.
+        (
+            "ARRAY<>",
+            format!(
+                "SELECT CAST(1 AS {}INT{})",
+                "ARRAY<".repeat(n),
+                ">".repeat(n)
+            ),
+        ),
+        (
+            "STRUCT<>",
+            format!(
+                "SELECT CAST(1 AS {}INT{})",
+                "STRUCT<a ".repeat(n),
+                ">".repeat(n)
+            ),
+        ),
+        // --- the same DataType cycle reached from non-CAST positions ---
+        (
+            "TRY_CAST",
+            format!(
+                "SELECT TRY_CAST(1 AS {}INT{})",
+                "Nullable(".repeat(n),
+                ")".repeat(n)
+            ),
+        ),
+        (
+            "::cast",
+            format!("SELECT 1::{}INT{}", "Nullable(".repeat(n), ")".repeat(n)),
+        ),
+        (
+            "column def",
+            format!(
+                "CREATE TABLE t (c {}INT{})",
+                "Nullable(".repeat(n),
+                ")".repeat(n)
+            ),
+        ),
+        (
+            "RETURNS TABLE",
+            format!(
+                "CREATE FUNCTION f() RETURNS TABLE (c {}INT{}) AS 'x'",
+                "Nullable(".repeat(n),
+                ")".repeat(n)
+            ),
+        ),
+        // --- cycles outside the DataType family ---
+        (
+            "INTERVAL prefix chain",
+            format!("SELECT {}'1'", "INTERVAL ".repeat(n)),
+        ),
+        (
+            "MATCH_RECOGNIZE PATTERN",
+            format!(
+                "SELECT * FROM t MATCH_RECOGNIZE(PATTERN ({}a{}) DEFINE a AS true)",
+                "(".repeat(n),
+                ")".repeat(n)
+            ),
+        ),
+        (
+            "JSON_TABLE NESTED COLUMNS",
+            format!(
+                "SELECT * FROM JSON_TABLE('{{}}', '$' COLUMNS({}a INT PATH '$'{}))",
+                "NESTED PATH '$' COLUMNS(".repeat(n),
+                ")".repeat(n)
+            ),
+        ),
+        // --- shapes that reach a *counted* site, but whose recursion-limit error
+        // --- was previously discarded and retried in `parse_prefix`
+        (
+            "nested CAST",
+            format!("SELECT {}1{}", "CAST(".repeat(n), " AS INT)".repeat(n)),
+        ),
+        (
+            "nested CASE",
+            format!(
+                "SELECT {}1{}",
+                "CASE WHEN true THEN ".repeat(n),
+                " END".repeat(n)
+            ),
+        ),
+    ];
+
+    for (name, sql) in cases {
+        let res = parse_depth_bounded(name, sql);
+        assert_eq!(
+            Err(ParserError::RecursionLimitExceeded),
+            res,
+            "{name}: expected a clean recursion-limit denial"
+        );
+    }
+}
+
+/// The depth guard must not be so eager that it rejects ordinary SQL, and must
+/// not convert the *iterative* deep shapes into denials.
+#[test]
+fn depth_guard_does_not_deny_shapes_that_are_not_recursive() {
+    // `INT[][]..[]` is assembled by a loop in `parse_data_type_helper`, not by
+    // recursive descent, so it must still parse at a depth no recursion could reach.
+    let sql = format!("SELECT CAST(1 AS INT{})", "[]".repeat(5000));
+    assert_eq!(
+        Ok(()),
+        parse_depth_bounded("sq", sql),
+        "square-bracket array form"
+    );
+
+    // A long UNION chain is left-associative and parsed iteratively.
+    let sql = format!("SELECT 1{}", " UNION SELECT 1".repeat(5000));
+    assert_eq!(
+        Ok(()),
+        parse_depth_bounded("union", sql),
+        "set-operation chain"
+    );
+
+    // Ordinary nested types stay well inside the budget.
+    let sql = "SELECT CAST(1 AS ARRAY<STRUCT<a Nullable(INT)>>)".to_string();
+    assert_eq!(
+        Ok(()),
+        parse_depth_bounded("ordinary", sql),
+        "ordinary nested type"
+    );
+}
+
+/// The recursion limit must change *whether* a statement parses, never *how* it
+/// parses. Before the fix, `parse_prefix` discarded `RecursionLimitExceeded` and
+/// retried the same span under a different interpretation, so the same SQL
+/// produced structurally different ASTs at different limits — silently, as `Ok`.
+#[test]
+fn recursion_limit_does_not_change_the_parsed_ast() {
+    let dialect = GenericDialect {};
+    let sql = format!(
+        "SELECT {}1{}",
+        "CASE WHEN true THEN ".repeat(6),
+        " END".repeat(6)
+    );
+
+    let mut parsed = None;
+    for limit in [20usize, 40, 60, 100] {
+        let res = Parser::new(&dialect)
+            .with_recursion_limit(limit)
+            .try_with_sql(&sql)
+            .expect("tokenize to work")
+            .parse_statements();
+
+        match res {
+            // Denial is a legitimate outcome at a tight limit.
+            Err(ParserError::RecursionLimitExceeded) => {}
+            Err(e) => panic!("limit {limit}: unexpected error {e}"),
+            Ok(ast) => {
+                let rendered = ast[0].to_string();
+                if let Some(prev) = &parsed {
+                    assert_eq!(
+                        prev, &rendered,
+                        "limit {limit} produced a different AST for identical SQL"
+                    );
+                } else {
+                    parsed = Some(rendered);
+                }
+            }
+        }
+    }
+
+    // Sanity: at least one limit must have been generous enough to succeed, and
+    // the successful parse must round-trip to the input.
+    assert_eq!(
+        Some(sql),
+        parsed,
+        "no limit parsed the statement successfully"
+    );
+}
+
 #[test]
 fn parse_deeply_nested_interval_hits_recursion_limits() {
     let dialect = GenericDialect {};