Skip to content

Reorder static fields in LoggingEventSource to prevent reentrant bug in FilterSpec parsing#127686

Merged
hoyosjs merged 6 commits intodotnet:mainfrom
hoyosjs:juhoyosa/fix-loggingeventsource
May 5, 2026
Merged

Reorder static fields in LoggingEventSource to prevent reentrant bug in FilterSpec parsing#127686
hoyosjs merged 6 commits intodotnet:mainfrom
hoyosjs:juhoyosa/fix-loggingeventsource

Conversation

@hoyosjs
Copy link
Copy Markdown
Member

@hoyosjs hoyosjs commented May 2, 2026

Fixes #127681

@dotnet-policy-service
Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-extensions-logging
See info in area-owners.md if you want to be subscribed.

Comment thread src/libraries/Microsoft.Extensions.Logging.EventSource/src/LoggingEventSource.cs Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a subtle static initialization order hazard in Microsoft.Extensions.Logging.EventSource.LoggingEventSource where OnEventCommand can be invoked re-entrantly during LoggingEventSource type initialization, causing ParseFilterSpec to observe null separator arrays and parse FilterSpecs incorrectly.

Changes:

  • Moves s_semicolon / s_colon static separator initializers ahead of Instance so they’re initialized before any re-entrant ParseFilterSpec call.
  • Adds an explanatory comment documenting the re-entrancy scenario and why declaration order matters.

@tarekgh
Copy link
Copy Markdown
Member

tarekgh commented May 2, 2026

CC @noahfalk @jkotas

Copy link
Copy Markdown
Member

@tarekgh tarekgh left a comment

Choose a reason for hiding this comment

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

LGTM.

@hoyosjs
Copy link
Copy Markdown
Member Author

hoyosjs commented May 2, 2026

Added a test - I can back out if we think it adds more toil than value

Comment thread src/libraries/Microsoft.Extensions.Logging.EventSource/src/LoggingEventSource.cs Outdated
Copilot AI review requested due to automatic review settings May 2, 2026 20:20
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Comment thread src/libraries/Microsoft.Extensions.Logging.EventSource/src/LoggingEventSource.cs Outdated
@hoyosjs hoyosjs enabled auto-merge (squash) May 4, 2026 21:38
@hoyosjs
Copy link
Copy Markdown
Member Author

hoyosjs commented May 5, 2026

/ba-g the helix legs timed out waiting on assignment after 8 (!!!) hours

@hoyosjs hoyosjs merged commit 55707bf into dotnet:main May 5, 2026
87 of 90 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Static field initialization order bug in LoggingEventSource causes ParseFilterSpec to silently produce wrong rules

6 participants