Skip to content

Harden dynamic payload length parsing - #2468

Merged
Brian Robbins (brianrob) merged 3 commits into
microsoft:mainfrom
msutara:users/marius/fix-dynamic-payload-lengths
Sep 18, 2026
Merged

Brian Robbins (brianrob) merged 3 commits into
microsoft:mainfrom
msutara:users/marius/fix-dynamic-payload-lengths

Conversation

@msutara

Copy link
Copy Markdown
Contributor

Summary

Fix dynamic TraceEvent payload parsing for unsigned length prefixes, invalid offsets, and truncated fixed-count arrays. This prevents oversized counted strings and arrays from producing negative or out-of-bounds payload reads.

Changes

  • Preserve unsigned 16-bit and 32-bit string and array length prefixes.
  • Use widened arithmetic before scaling UTF-16 lengths or narrowing array counts.
  • Validate length prefixes, counted payloads, fixed-count arrays, and cached field offsets before unchecked reads.
  • Add regression coverage for boundary values, malformed payloads, repeated field walks, and fixed/variable-length arrays.

Testing

dotnet test src\TraceEvent\TraceEvent.Tests\TraceEvent.Tests.csproj -c Debug --filter FullyQualifiedName~DynamicTraceEventParserVariableLengthFieldTests --no-restore

Passed 143 tests on net462 and 143 tests on net8.0.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@msutara
Marius Sutara (msutara) requested a review from a team as a code owner September 15, 2026 21:40

@brianrob Brian Robbins (brianrob) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for contributing this. Overall, looks great. A couple of questions specific to the tests.

Preserve direct lookup coverage through public payload access and remove the synthetic private cache-state test.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

@brianrob Brian Robbins (brianrob) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. One follow-up and then I think we're good.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

@brianrob Brian Robbins (brianrob) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@brianrob
Brian Robbins (brianrob) merged commit 4aab318 into microsoft:main Sep 18, 2026
5 checks passed
@msutara
Marius Sutara (msutara) deleted the users/marius/fix-dynamic-payload-lengths branch September 18, 2026 01:58
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.

2 participants