[Profiler] Fix crashes at shutdown - #9050
Open
gleocadie wants to merge 5 commits into
Open
Conversation
CorProfilerCallback's guarded ICorProfilerCallback methods checked _isInitialized.load() with no lock spanning check-to-use, so a callback could pass the check and still touch a service pointer DisposeServices() concurrently destroys - a real crash (ExecutionEngineException via a ThreadsCpuManager use-after-free) was traced to exactly this race. Adds a std::shared_mutex + EngineActiveGuard RAII helper: callbacks take a non-blocking shared lock and check _isShutdown while holding it (never as a bare flag read); DisposeInternal() takes an exclusive lock, sets _isShutdown, then tears down services - so no callback can observe the engine as alive past that point, whether it's already in flight or arrives after teardown completes. Applies it to AppDomainCreationFinished, previously fully unguarded despite using the _services-managed _pRuntimeIdStore. Remaining guarded callbacks (ThreadCreated, ThreadDestroyed, ThreadAssignedToOSThread, ThreadNameChanged, ModuleLoadFinished, ExceptionThrown) follow in subsequent commits.
Replaces the racy _isInitialized.load() guard with EngineActiveGuard in ThreadCreated, ThreadDestroyed, ThreadAssignedToOSThread, and ThreadNameChanged - the confirmed crash site (a ThreadPool worker renaming itself raced CorProfilerCallback::Shutdown() tearing down ThreadsCpuManager, producing a use-after-free deep inside std::unordered_map's internals, surfaced as ExecutionEngineException). Same mechanism as the previous commit, applied to the four callbacks that most directly motivated this fix.
Replaces the racy _isInitialized.load() guard with EngineActiveGuard in ModuleLoadFinished and ExceptionThrown, same mechanism as the previous two commits. This covers every ICorProfilerCallback method identified in the blast-radius audit as reading a _services-managed pointer behind the old check-then-use pattern. Two related, lower-priority items found during the audit are deliberately left out of this change: OnThreadRoutineFinished (Linux-only, guards via a different _this-null idiom, not implicated in the Windows crash this fix addresses) and OnStartDelayedProfiling (a different risk shape - concurrent iteration of _services during StartServices(), rather than a read of an already-nulled pointer).
…tdown flag The original _isInitialized.load() check the callbacks used to have was doing double duty: "has Initialize() finished yet" AND "has shutdown started". EngineActiveGuard only replaced the second half - a callback firing before Initialize() finishes constructing services would now incorrectly report IsActive() == true. Fixed by having the guard check isInitialized directly (a plain atomic read is fine here, since that direction of the lifecycle never destroys anything concurrently - only the shutdown direction needed the mutex). Also renames _isShutdown to _isServicesShutdown for clarity: it tracks specifically whether DisposeServices() has run, not overall process lifetime.
Covers both halves of the guard directly: not-yet-initialized, already-shut-down (the confirmed crash scenario), a writer blocking a would-be reader (non-blocking try_to_lock, callbacks never wait), a reader blocking a writer until released (in-flight callbacks finish before DisposeServices() can run), and concurrent readers not serializing against each other. All 583 tests in the native suite pass (1 pre-existing, unrelated skip).
github-actions
Bot
added
the
area:profiler
chrisnas
approved these changes
Aug 13, 2026
| #include <atomic> | ||
| #include <shared_mutex> | ||
|
|
||
| // Non-blocking guard for ICorProfilerCallback methods that read service pointers whose lifetime |
Contributor
There was a problem hiding this comment.
probably too verbose comment
| std::shared_ptr<IMetricsSender> _metricsSender; | ||
| std::atomic<bool> _isInitialized{false}; // pay attention to keeping ProfilerEngineStatus::IsProfilerEngiveActive in sync with this! | ||
|
|
||
| // Guards ICorProfilerCallback methods (see EngineActiveGuard.h) against DisposeInternal() |
BenchmarksBenchmark execution time: 2026-08-13 16:59:48 Comparing candidate commit f5c7cae in PR branch Found 0 performance improvements and 1 performance regressions! Performance is the same for 71 metrics, 0 unstable metrics, 65 known flaky benchmarks, 61 flaky benchmarks without significant changes.
|
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (9050) and master. ✅ No regressions detected |
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
2 participants
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.
Summary of changes
This PR fixes a bunch of crashes that happen at shutdown. TOCTOU : Time Of Check to Time Of Use.
ICorprofiler callbacks where using .NET profiler services at shutdown 💥
Reason for change
A crash report brought a crash of the .NET profiler:
And in some other threads were could see that IIS was shutting down the application.
Implementation details
Use
EngineActiveGuardto know if it's safe to use a service from within a callback.This uses a reader/writer lock to prevent services from being destroyed while executing a callback but also prevent the callback from using the service(s) if they are cleaned up.
Test coverage
Add unit tests.
Other details