Skip to content

Refuse a start tag with too many attributes before XmlReader reads it - #600

Merged
erikdarlingdata merged 2 commits into
devfrom
fix/xml-attribute-prescan
Sep 28, 2026
Merged

erikdarlingdata merged 2 commits into
devfrom
fix/xml-attribute-prescan

Conversation

@erikdarlingdata

@erikdarlingdata erikdarlingdata commented Sep 28, 2026 •

Copy link
Copy Markdown
Owner

What does this PR do?

Plan XML with one start tag that has a very large number of attributes loaded slowly. XmlReader reads a whole start tag before it can say how many attributes the tag has. For a tag with one million attributes, that took about 25 seconds. Only then did the existing limit of 1,024 attributes refuse the plan. The desktop app and the web viewer both use this code, so both were slow on such a plan.

PlanXml now counts the attributes of each start tag in the text, before XmlReader reads it:

  • The count is one pass over the text. It stops at the first tag with more than 1,024 attributes and refuses the plan with the same message as before.
  • Each attribute has exactly one = outside its quoted value. So the count is the number of = signs between a start tag's < and > that are not inside quotes.
  • The count steps over comments, CDATA sections, processing instructions, declarations and end tags.
  • Text that is not well formed stops the count. XmlReader then reports the error, as before.

The attribute check in the XmlReader pass stays as a second check.

Which component(s) does this affect?

  • Desktop App (PlanViewer.App)
  • Core Library (PlanViewer.Core)
  • CLI Tool (PlanViewer.Cli)
  • SSMS Extension (PlanViewer.Ssms)
  • Tests
  • Documentation

The web viewer, PlanViewer.Web, compiles the same file, so it gets the change too.

How was this tested?

  • New tests in PlanXmlTests:
    • A start tag with 1,000,000 attributes is refused in less than 5 seconds. With this change it took 5 ms. Without the change, the same test took 24.9 seconds and failed.
    • An = inside a value is not counted. A > or a ' inside a double-quoted value does not end the count. Single-quoted values count the same as double-quoted ones.
    • A tag inside a comment, a CDATA section, a processing instruction or escaped text is not counted.
    • Text that ends inside a quote, a comment, a CDATA section, an instruction or a tag stops the count with no error.
    • A DOCTYPE with a quoted > in its internal subset is still refused. The count stops at that > and reads the rest as text and tags. So it can only count more, and XmlReader refuses the DTD anyway.
    • Every fixture plan passes the count.
  • Cost: on a plan at the size limit (16.7 million characters), the count took 18 ms. Parsing the same plan took 498 ms.
  • Full suite on Windows: 1,080 tests, 1,078 passed, 0 failed, 2 skipped. The Release build has 0 warnings.

Checklist

  • I have read the contributing guide
  • My code builds with zero warnings (Release build, --no-incremental)
  • All tests pass (dotnet test)
  • I have not introduced any hardcoded credentials or server names

🤖 Generated with Claude Code

https://claude.ai/code/session_019n3G844aTidqrD6A6iMgza

XmlReader reads a whole start tag before it can report AttributeCount,
and a tag with a million attributes took it about 25 seconds before the
existing 1,024 limit could refuse it. CheckAttributeCounts counts each
start tag's "=" signs outside quotes in one pass and throws at the first
tag past the limit; comments, CDATA, processing instructions,
declarations and end tags are stepped over, and malformed text is left
to XmlReader. The XmlReader-side check stays.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019n3G844aTidqrD6A6iMgza
@claude

claude Bot commented Sep 28, 2026

Copy link
Copy Markdown

Reviewed PlanXml.CheckAttributeCounts. I found nothing that needs changing.

  • Span bounds: the slices at the end of the text (< as the last character, SkipPast past the end, a quote as the last character) stay in range, and the tests cover them.
  • Cost: it is a single linear pass, since SkipStartTag never rescans. An attacker can't make it slower than the parse it guards.
  • Scanner and parser disagreeing: the only way I found is a DOCTYPE with a quoted > in its internal subset. There the scanner could see a later < as a tag start. That can't hide a real over-limit tag, and the second attribute check in the XmlReader pass is still there as a backstop.
  • Conventions: no new warnings or NoWarn, no version bump, no T-SQL. PlanXml.cs is already compiled by the Web project.

Optional: add a test with a DOCTYPE whose internal subset contains > inside quotes, to pin that behavior.

🤖 Generated with Claude Code

@claude

claude Bot commented Sep 28, 2026

Copy link
Copy Markdown

Reviewed the diff. I found no correctness, untrusted-input, or convention problems, and I didn't build or run the tests.

  • The pre-scan stays in step with XmlReader for well-formed input. It skips quoted values, comments, CDATA and PIs, and it stops on unterminated input. XmlReader then reports the error.
  • The DOCTYPE case can only over-count. The DTD is refused either way, and a test covers it.
  • The scan is linear, since it uses IndexOfAny with SearchValues, so it adds no new quadratic path.

One minor point: AStartTagWithAMillionAttributesIsRefusedQuickly asserts a wall-clock bound of 5s. Refusal should take milliseconds, so that leaves plenty of margin. It's still a timing assertion, so it could flake on a heavily loaded CI runner.

LGTM.

@erikdarlingdata
erikdarlingdata merged commit ddd2c4c into dev Sep 28, 2026
4 checks passed
@erikdarlingdata
erikdarlingdata deleted the fix/xml-attribute-prescan branch September 28, 2026 20:49
@erikdarlingdata erikdarlingdata mentioned this pull request Sep 29, 2026
2 of 8 tasks
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