Apply recursive-protection to the Spanned::span walk#2400
Open
fmcmac wants to merge 1 commit into
Open
Conversation
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 <francis@zuru.com>
ac9702d to
b7cbd09
Compare
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.
What
The
Spanned::spanimplementations 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) recurses as deep as the AST and can overflow the thread stack, aborting the process.Notably, a query can parse successfully — the parser is already guarded by the
recursive-protectionfeature — and then overflow later, when its span is computed by a downstream consumer (e.g. during query planning). We hit this in production via a downstream planner that calls.span()on user-supplied ASTs.Fix
The parser already opts into stack-overflow protection behind the
recursive-protectionfeature (recursive::recursive, backed bystacker::maybe_grow); the span walk was simply never given the same treatment. This annotates the recursivespan()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.recursivecrate and feature already exist).recursive-protectionis disabled (same#[cfg_attr(...)]gating the parser uses).stacker::maybe_growto fire; these seven cover the deep axes (expression nesting, set-op/subquery/join nesting, statement nesting).Test
Adds a regression test that builds a deeply nested AST and walks its span on a deliberately small (512 KiB) thread stack: it overflows without the annotations and completes with them.