From f2153e22ffbdf4ba97517408d8db254446f43ca6 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Thu, 3 Sep 2026 04:56:17 -0400 Subject: [PATCH] Carry the parse depth through proc and UDF descent #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 Claude-Session: https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n --- .../Services/ShowPlanParser.cs | 29 ++++- .../ShowPlanParserLimitsTests.cs | 111 ++++++++++++++++++ 2 files changed, 134 insertions(+), 6 deletions(-) create mode 100644 tests/PlanViewer.Core.Tests/ShowPlanParserLimitsTests.cs diff --git a/src/PlanViewer.Core/Services/ShowPlanParser.cs b/src/PlanViewer.Core/Services/ShowPlanParser.cs index 453d209..b01c7ee 100644 --- a/src/PlanViewer.Core/Services/ShowPlanParser.cs +++ b/src/PlanViewer.Core/Services/ShowPlanParser.cs @@ -15,14 +15,23 @@ public static partial class ShowPlanParser // Plan XML is untrusted input (opened/pasted/downloaded). Cap recursion depth so a // maliciously deep tree throws a catchable exception instead of an uncatchable // StackOverflowException that takes the whole process down. - private const int MaxParseDepth = 1000; - private const int MaxParseCharacters = 16 * 1024 * 1024; + // Internal so tests can pin behavior just past each limit without hardcoding the values. + internal const int MaxParseDepth = 1000; + internal const int MaxParseCharacters = 16 * 1024 * 1024; public static ParsedPlan Parse(string xml) { var plan = new ParsedPlan { RawXml = xml }; try { + /* Same ceiling ParseAsync enforces through XmlReaderSettings.MaxCharactersInDocument, + which this synchronous path (PlanViewerControl, the web viewer, the analysis + pipeline) never had - it went straight to XDocument.Parse with no limit at all. + The input is already an in-memory string here, so a length check is the equivalent + guard; like the reader setting, the limit is in characters, not bytes. */ + if (xml.Length > MaxParseCharacters) + throw new InvalidOperationException( + $"Plan XML exceeds the supported size limit of {MaxParseCharacters.ToString("N0", CultureInfo.InvariantCulture)} characters."); return ParseDocument(XDocument.Parse(xml), plan, CancellationToken.None); } catch (Exception exception) @@ -109,7 +118,7 @@ private static ParsedPlan ParseDocument( foreach (var statementElement in root.Descendants(Ns + "StmtSimple")) { cancellationToken.ThrowIfCancellationRequested(); - var statement = ParseStatement(statementElement, cancellationToken); + var statement = ParseStatement(statementElement, depth: 0, cancellationToken); if (statement is not null) batch.Statements.Add(statement); } @@ -213,7 +222,7 @@ private static List ParseStatementAndChildren( else { // StmtSimple or any other statement type - var stmt = ParseStatement(stmtEl, cancellationToken); + var stmt = ParseStatement(stmtEl, depth, cancellationToken); if (stmt != null) results.Add(stmt); } @@ -221,8 +230,16 @@ private static List ParseStatementAndChildren( return results; } + /* The depth parameter exists for the StoredProc/UDF descents below. #456 added those + descents calling ParseStatementAndChildren without a depth argument, so the recursion + depth silently reset to zero at every procedure boundary and the MaxParseDepth guard in + ParseStatementAndChildren could never fire across StoredProc/UDF nesting - a crafted plan + alternating StmtSimple > StoredProc > Statements a few thousand levels deep (about sixty + bytes each) still reached the uncatchable StackOverflowException the guard exists to + prevent. Carrying the caller's depth through this method closes that reset. */ private static PlanStatement? ParseStatement( XElement stmtEl, + int depth = 0, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); @@ -296,7 +313,7 @@ did. The same was true of a UDF call whose statement carries no plan of its own. { foreach (var childStmt in udfStmts.Elements()) { - var parsed = ParseStatementAndChildren(childStmt, cancellationToken: cancellationToken); + var parsed = ParseStatementAndChildren(childStmt, depth + 1, cancellationToken); udfInfo.Statements.AddRange(parsed); } } @@ -317,7 +334,7 @@ did. The same was true of a UDF call whose statement carries no plan of its own. { foreach (var childStmt in spStmts.Elements()) { - var parsed = ParseStatementAndChildren(childStmt, cancellationToken: cancellationToken); + var parsed = ParseStatementAndChildren(childStmt, depth + 1, cancellationToken); spInfo.Statements.AddRange(parsed); } } diff --git a/tests/PlanViewer.Core.Tests/ShowPlanParserLimitsTests.cs b/tests/PlanViewer.Core.Tests/ShowPlanParserLimitsTests.cs new file mode 100644 index 0000000..2097c65 --- /dev/null +++ b/tests/PlanViewer.Core.Tests/ShowPlanParserLimitsTests.cs @@ -0,0 +1,111 @@ +using System.Text; +using PlanViewer.Core.Models; +using PlanViewer.Core.Services; + +namespace PlanViewer.Core.Tests; + +/// +/// #456's stored-procedure descent called ParseStatementAndChildren without passing the depth +/// argument, so the parser's recursion depth silently reset to zero at every StoredProc/UDF +/// boundary. The MaxParseDepth guard — which exists precisely so a maliciously deep plan throws +/// a catchable error instead of an uncatchable StackOverflowException — could therefore never +/// fire across procedure nesting, and roughly sixty bytes of XML per level bought an attacker +/// one more stack frame pair on every plan-open route that feeds the parser. +/// +/// These tests pin both parser input ceilings: recursion depth across procedure bodies, +/// and the synchronous Parse path's document-size cap. ParseAsync always had the cap through +/// XmlReaderSettings.MaxCharactersInDocument; Parse — the path used by the app's +/// PlanViewerControl, the web viewer, and the analysis pipeline — had none. +/// +/// The plan XML here is generated rather than loaded from a fixture: a depth bomb is the +/// same three elements repeated a thousand-odd times, and a loop states that more honestly than +/// a 70KB fixture file could. +/// +public sealed class ShowPlanParserLimitsTests +{ + /// + /// Nesting chosen to fail safely in both directions. With the guard carrying through, it + /// fires at depth 1,001 — about two thousand frames, nowhere near exhaustion. If the depth + /// reset ever regresses, this nesting is shallow enough to parse to completion on the + /// big-stack thread below, so the test fails on the ParseError assert (it stays null) + /// instead of killing the test host — verified by running it against the unfixed parser. + /// + private const int BombDepth = ShowPlanParser.MaxParseDepth + 100; + + [Fact] + public void ProcedureNestingPastTheDepthLimitFailsWithACatchableError() + { + var xml = NestedProcedurePlan(BombDepth); + + /* A dedicated thread with a deliberately generous stack, so the test's two outcomes + stay deterministic regardless of build config or future frame-size drift: guard + working -> depth error long before the stack matters; guard regressed -> the parse + COMPLETES in the headroom (instead of gambling on where overflow lands) and the + assert below reports the miss. */ + ParsedPlan? plan = null; + var thread = new Thread(() => plan = ShowPlanParser.Parse(xml), 8 * 1024 * 1024); + thread.Start(); + thread.Join(); + + Assert.NotNull(plan); + Assert.NotNull(plan!.ParseError); + Assert.Contains("depth limit", plan.ParseError); + } + + /// + /// The other direction: carrying depth through procedure bodies must not reject legitimate + /// nesting. Every level below the limit still parses, in order, all the way down. + /// + [Fact] + public void ProcedureNestingBelowTheDepthLimitStillParsesEveryLevel() + { + const int depth = 50; + + var plan = ShowPlanParser.Parse(NestedProcedurePlan(depth)); + + Assert.Null(plan.ParseError); + var stmt = Assert.Single(Assert.Single(plan.Batches).Statements); + var levels = 0; + while (stmt.StoredProcPlan is not null) + { + stmt = Assert.Single(stmt.StoredProcPlan.Statements); + levels++; + } + Assert.Equal(depth, levels); + } + + [Fact] + public void SynchronousParseRejectsOversizedInput() + { + /* Well-formed XML on purpose: if the size cap regressed, this input would parse + cleanly and ParseError would stay null, so the test fails on the assert instead of + passing by accident on a syntax error. Whitespace inside the root pads it just past + the limit; the cap is in characters, matching MaxCharactersInDocument's unit on the + async path. */ + var xml = "" + + new string(' ', ShowPlanParser.MaxParseCharacters) + + ""; + + var plan = ShowPlanParser.Parse(xml); + + Assert.NotNull(plan.ParseError); + Assert.Contains("size limit", plan.ParseError); + } + + /// + /// StmtSimple > StoredProc > Statements repeated times around one + /// innermost bare statement — the exact shape whose depth #456's descent stopped counting. + /// + private static string NestedProcedurePlan(int levels) + { + var xml = new StringBuilder( + ""); + for (var level = 0; level < levels; level++) + xml.Append(""); + xml.Append(""); + for (var level = 0; level < levels; level++) + xml.Append(""); + xml.Append(""); + return xml.ToString(); + } +}