Close the remaining gaps in the parser recursion counter - #2440
Open
fmcmac wants to merge 1 commit into
Open
Conversation
`RecursionCounter` is charged at five sites today (`parse_statement`, `parse_subexpr`, `parse_query`, `parse_table_factor`, and `parse_interval` since apache#2422). Those sites were each added in response to a specific report, and nothing asserts they cover every recursive path. Where they do not, `with_recursion_limit` is silently inert and input recurses until the stack runs out. Audited rather than patched by example. Built the call graph over every `Parser` method, deleted the counted functions, and recomputed the strongly connected components: anything still recursive is a cycle that can spin without ever reaching a guard. On current main that finds eight such cycle groups. Each is now either counted or carries a comment recording why it cannot recur on input. Newly counted, with the shape that drives each: parse_data_type_helper SELECT CAST(1 AS Nullable(Nullable(..))) also LowCardinality, Map, Tuple, Nested, and ARRAY<..> / STRUCT<..> parse_pattern MATCH_RECOGNIZE(PATTERN ((((..)))) parse_json_table_column_def JSON_TABLE(.. NESTED PATH .. COLUMNS(..)) parse_key_value_options Snowflake COPY_OPTIONS=(a=(b=(..))) parse_joins SELECT * FROM t JOIN t JOIN t .. (on dialects where supports_left_associative_joins_without_ parens is false, i.e. Snowflake) The guard for data types goes on `parse_data_type_helper`, not on the public `parse_data_type`: `ARRAY<..>` and `STRUCT<..>` recurse into the helper directly in order to thread `MatchedTrailingBracket`, so a guard on `parse_data_type` alone leaves the angle-bracket forms uncounted. `parse_joins` is worth calling out because the obvious reasoning is wrong. It is tempting to say its self-call is already charged, since reaching it requires a `parse_table_factor` and that takes a guard. But `DepthGuard` releases on drop and `parse_table_factor` has already returned by the time `parse_joins` recurses, so nothing accumulates. Deliberately not counted, with the reason recorded in a comment: `parse_remaining_set_exprs`. It forms a cycle with `parse_query_body` but cannot recur on input -- the left side is consumed by a loop, so long same-precedence chains are iterative, and the right side only recurses on increasing precedence, of which there are two levels. Separately, stop discarding the guard's own error. `parse_prefix` matched only the `Ok` arm of a `maybe_parse` fallback, dropping `RecursionLimitExceeded` and retrying the same span under a second interpretation. `maybe_parse` re-raises that error specifically so callers can propagate it. Retrying re-descends the same input once per level, which makes a bounded denial exponential, and, when the retry happens to succeed, silently changes the AST -- so identical SQL parsed to different trees at different recursion limits, returned as `Ok`. Measured on a 2 MB stack, release build, parse only: each shape above previously did not return within 10 s and now denies in under 2 ms. `INT[][]..[]` and long `UNION` chains are assembled iteratively and still parse, so the guard has not become too eager. Tests assert the clean `RecursionLimitExceeded` rather than merely that parsing finished, and run each parse on a thread with a timeout so a regression fails loudly instead of hanging CI.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
RecursionCounteris charged at five sites today —parse_statement,parse_subexpr,parse_query,parse_table_factor, andparse_interval(added in #2422). Each was added in response to a specific report, and nothing asserts they cover every recursive path. Where they don't,with_recursion_limitis silently inert and input recurses until the stack runs out.Demonstrated directly on current
main, 2 MB stack, release build,Parser::parse_sqlonly. Each of these fails to return within 10 s:SELECT CAST(1 AS Nullable(Nullable(…)))— alsoLowCardinality,Map,Tuple,Nested,ARRAY<…>,STRUCT<…>MATCH_RECOGNIZE(PATTERN ((((…))))JSON_TABLE(… NESTED PATH … COLUMNS(…))COPY_OPTIONS=(a=(b=(…)))SELECT * FROM t JOIN t JOIN t …(Snowflake) — this one stack-overflows2000 nested
Nullable(is about 2 KB of SQL.with_recursion_limit(5)does not stop any of them.Method
Audited rather than patched by example — fixing the shapes someone happened to report is what produced the current state. I built the call graph over every
Parsermethod, deleted the counted functions, and recomputed the strongly connected components. Anything still recursive is a cycle that can spin without ever reaching a guard. On currentmainthat finds eight such cycle groups.Each is now either counted or carries a comment recording why it cannot recur on input.
Newly counted
parse_data_type_helper,parse_pattern,parse_json_table_column_def,parse_key_value_options,parse_joins.Two of these are worth calling out:
parse_data_type_helper, notparse_data_type. The obvious place is the publicparse_data_type, butARRAY<…>andSTRUCT<…>recurse into the helper directly in order to threadMatchedTrailingBracketthrough. A guard onparse_data_typealone leaves both angle-bracket forms uncounted.parse_joins. It's tempting to argue the self-call is already charged, since reaching it requires aparse_table_factorand that takes a guard. That reasoning is wrong:DepthGuardreleases on drop, andparse_table_factorhas already returned by the timeparse_joinsrecurses, so nothing accumulates.Deliberately not counted, with the reason in a comment:
parse_remaining_set_exprs. It cycles withparse_query_bodybut cannot recur on input — the left side is consumed by a loop (so long same-precedence chains stay iterative) and the right side only recurses on increasing precedence, of which there are two levels.Second defect: the guard's own error was discarded
parse_prefixmatched only theOkarm of amaybe_parsefallback, droppingRecursionLimitExceededand retrying the same span under a second interpretation.maybe_parsere-raises that error precisely so callers can propagate it.Two consequences, both on an already-counted path:
CAST(…)did not return at ~650 bytes of SQL.Ok. Six nestedCASErendered with 2, 5 or 6ENDs at limits 5, 8 and 12.(2) is a correctness bug independent of any resource concern, and is what
recursion_limit_does_not_change_the_parsed_astpins.Results
Every shape above now denies with
RecursionLimitExceededin under 2 ms.Not made too eager:
INT[][]…[]×5000 and a 20,000-operandUNIONchain are assembled iteratively and still parse.The cliff is remaining stack, not input size — 5000-deep
Nullable(completed in 1.8 ms at an 8 MB stack while wedging at 2 MB. None of the numbers above should be treated as thresholds.Tests
Tests assert the clean
RecursionLimitExceededrather than merely that parsing finished, and run each parse on its own thread with a timeout, so a regression fails loudly instead of hanging CI. Successful parses aremem::forget-ed, because the derivedDropon the AST is itself recursive and dropping a deep tree performs the very descent under test.Full suite passes (1592 tests).
cargo fmtclean; no new clippy warnings.Note
Reaching the default limit of 50 itself costs stack: the
parse_prefix → parse_cast_expr → parse_expr → parse_subexprchain runs ~85 KB per level unoptimised, so a debug build wants ~5 MB of headroom before the guard can fire. Documented onwith_recursion_limit. I have not changedDEFAULT_REMAINING_DEPTH— that's a judgement call for maintainers.One further finding I did not act on: the audit reports a cycle
parse_object_name → parse_object_name_inner → parse_function_args → function_arg_expr_from_wildcard → parse_wildcard_additional_options → parse_optional_select_item_exclude → parse_object_namethat exists onmainbut not in v0.61. I could not construct an input that drives it, so I have left it alone rather than guess at a guard. Flagging it in case someone recognises a shape that reaches it.