Distinguish TraceLogging events that share a name in the schema cache - #282
Open
Chris Davis (chrdavis) wants to merge 3 commits into
Open
Chris Davis (chrdavis) wants to merge 3 commits into
Chris Davis (chrdavis) wants to merge 3 commits into
Conversation
Fixes microsoft#193. TraceLogging events do not get a meaningful event id, so the schema cache key could not tell two events from the same provider apart. Adding the event name (microsoft#242) covered the common case, but a provider may log the same event name from several call sites with different fields. Those variants agree on provider, name, id, version, opcode, level and keyword, so they still collided and the first schema retrieved was used to decode all of them - silently shifting every field, and potentially reading past the end of the user data buffer. The TraceLogging metadata is the schema, so hash it into the key. This keeps the cache effective for repeated events while separating variants, and costs no extra TdhGetEventInformation call. The hash is FNV-1a over a small blob; 0 is reserved to mean "no metadata" so manifest-based events, which the existing fields already identify uniquely, are unaffected. Also fixes two defects in the metadata parsing this relies on: * The name was read with an unbounded string_view constructor. A record whose name is not nul-terminated would read past the end of the extended data item. The search is now bounded by the metadata extent. * The parser required the metadata Size field to exactly equal the extended data item size. Size describes the pseudo-structure and the item may be larger, so valid events lost their name and fell back to the ambiguous key. Size is now range-checked against the item instead. And fixes schema_key::operator=, which recursed until the stack was exhausted: the user-declared copy constructor and copy assignment suppress the implicit move operations, so the std::swap it used resolved back to operator= itself. It now assigns members directly and re-internalizes the name to preserve the copy constructor's ownership invariant. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds examples/SchemaKeyCollisionRepro, a minimal program that demonstrates issue microsoft#193 without needing an external provider, an ETL file, or any third-party tooling. It registers its own TraceLogging provider and emits two variants of a single event name -- one with a leading PartA_PrivTags field and one without -- then consumes them with krabs and prints the field names decoded for each variant. Real providers do this; for example, Microsoft.Windows.AppLifeCycle.UI emits AppLaunch_UserClick both ways. Because the variants agree on provider, name, id, version, opcode, level and keyword, they collide in schema_locator's cache: the schema cached for the first variant is reused for the second, so every field of the second is shifted by the size of the missing field. The program exits 0 on PASS, 1 on FAIL and 2 if it could not start a trace session, so it doubles as a regression check. Verified against both the pre-fix and post-fix schema_locator: it fails on the former and passes on the latter. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
1 participant
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.
Fixes #193.
TraceLogging events do not get a meaningful event id, so the schema cache key could not tell two events from the same provider apart. Adding the event name (#242) covered the common case, but a provider may log the same event name from several call sites with different fields. Those variants agree on provider, name, id, version, opcode, level and keyword, so they still collided and the first schema retrieved was used to decode all of them - silently shifting every field, and potentially reading past the end of the user data buffer.
The TraceLogging metadata is the schema, so hash it into the key. This keeps the cache effective for repeated events while separating variants, and costs no extra TdhGetEventInformation call. The hash is FNV-1a over a small blob; 0 is reserved to mean "no metadata" so manifest-based events, which the existing fields already identify uniquely, are unaffected.
Also fixes two defects in the metadata parsing this relies on:
The name was read with an unbounded string_view constructor. A record whose name is not nul-terminated would read past the end of the extended data item. The search is now bounded by the metadata extent.
The parser required the metadata Size field to exactly equal the extended data item size. Size describes the pseudo-structure and the item may be larger, so valid events lost their name and fell back to the ambiguous key. Size is now range-checked against the item instead.
And fixes schema_key::operator=, which recursed until the stack was exhausted: the user-declared copy constructor and copy assignment suppress the implicit move operations, so the std::swap it used resolved back to operator= itself. It now assigns members directly and re-internalizes the name to preserve the copy constructor's ownership invariant.