Skip to content

Carry the parse depth through proc and UDF descent - #484

Merged
erikdarlingdata merged 1 commit into
devfrom
fix/parser-depth-carry
Sep 3, 2026
Merged

Carry the parse depth through proc and UDF descent#484
erikdarlingdata merged 1 commit into
devfrom
fix/parser-depth-carry

Conversation

@erikdarlingdata

Copy link
Copy Markdown
Owner

What does this PR do?

Fixes the highest-severity finding from the 2026-09-02 adversarial review of the Aug 12 - Sep 2 range.

#456's StoredProc/UDF descents called ParseStatementAndChildren without the depth argument, so recursion depth silently reset to zero at every procedure boundary and the MaxParseDepth guard could never fire across procedure nesting. A crafted .sqlplan alternating StmtSimple > StoredProc > Statements a few thousand levels deep (~60 bytes per level) killed the process with an uncatchable StackOverflowException, from any plan-open route. Shipped in v1.22.0+.

  • Depth is now threaded through ParseStatement (where the descent sites live) and both descents pass depth + 1.
  • The synchronous Parse path (app UI, web viewer, analysis pipeline) gains the same 16MB ceiling ParseAsync always had, via the shared const.
  • Three new tests: a generated ~1,100-level depth bomb on a dedicated big-stack thread (fails cleanly in both directions — verified red against the unfixed parser), a 50-level positive test walking every parsed level, and a sync size-cap pin.

How was this tested?

Full suite at dev tip + fix: 408 tests, 407 passed, 1 platform skip, 0 failed, on Windows. Red-green verified against the unfixed parser. Web and App projects build clean.

🤖 Generated with Claude Code

https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n

#456's StoredProc/UDF descents called ParseStatementAndChildren without
the depth argument, so recursion depth silently reset to zero at every
procedure boundary. The MaxParseDepth guard - which exists so a
maliciously deep plan throws a catchable error instead of an uncatchable
StackOverflowException - could then never fire across StoredProc/UDF
nesting: a crafted .sqlplan alternating StmtSimple > StoredProc >
Statements a few thousand levels deep (about sixty bytes per level)
killed the whole process, from any plan-open route that reaches the
parser. ParseStatement now takes the caller's depth and both descents
pass depth + 1, so the guard sees the true nesting.

The same pass closes the sibling gap: synchronous Parse had no
document-size ceiling at all, while ParseAsync capped at 16MB via
XmlReaderSettings.MaxCharactersInDocument. Parse is the path the app's
PlanViewerControl, the web viewer, and the analysis pipeline actually
use, so it now enforces the same MaxParseCharacters limit with a length
check (the input is already a string; the limit is characters, not
bytes) thrown as the parser's usual catchable InvalidOperationException.

Tests generate the plan XML instead of shipping a fixture - a depth bomb
is three elements repeated 1,100 times. The bomb parses on a
deliberately large-stack thread with nesting just past the guard, so
both outcomes are deterministic: fixed, the guard fires at depth 1,001;
regressed, the parse completes in the headroom and the test fails on a
null ParseError instead of killing the test host (verified against the
unfixed parser). A 50-level companion pins that legitimate nesting still
parses every level, and an oversized well-formed document pins the sync
size cap.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n
@claude

claude Bot commented Sep 3, 2026

Copy link
Copy Markdown

Reviewed. The depth-threading fix is correct: ParseStatementAndChildren now passes depth (same level) into ParseStatement, which forwards depth + 1 into both the UDF and StoredProc descents — this is the exact reset #456 introduced, and it's closed on both paths. The StmtCond branches already incremented correctly and are untouched. The sync Parse size cap correctly mirrors ParseAsync's existing MaxCharactersInDocument (char-count semantics, same constant), and sits inside the existing try/catch so it degrades to ParseError like every other failure mode here — good, no untrusted-input path throws uncaught.

Tests are well-targeted: the depth-bomb test isolates the parse on a dedicated 8MB-stack thread so it fails on the assertion rather than the test host if the guard regresses, and the 50-level positive test walks the actual StoredProcPlan chain rather than just checking for the absence of an error. The oversized-input test uses well-formed XML so it can't pass by accident on a syntax error.

One thing worth a sentence of awareness rather than a fix: ParseRelOp's operator-tree depth counter (RelOp.cs) is independent of this statement-depth counter and resets to 0 at each statement boundary (both call sites at lines 373/423 omit the depth argument). A pathological plan could in principle stack ~1000 StoredProc levels on top of a ~1000-deep operator tree in the deepest statement, compounding to ~2000 real frames on one call stack instead of ~1000. That's pre-existing (not introduced here) and the two guards were clearly designed independently, so not blocking — just flagging in case a future hardening pass wants to thread a combined budget through both.

No version bump needed here (this PR targets dev, and check-version-bump.yml only gates devmain). No T-SQL, no PlanViewer.Web linked-file surface touched. Looks good to merge.

@erikdarlingdata
erikdarlingdata merged commit ca5fb36 into dev Sep 3, 2026
3 checks passed
@erikdarlingdata
erikdarlingdata deleted the fix/parser-depth-carry branch September 3, 2026 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant