[Profiler] Fix crashes at shutdown #9050
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
| // Unless explicitly stated otherwise all files in this repository are licensed under the Apache 2 License. | ||
| // This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2022 Datadog, Inc. | ||
|
|
||
| #pragma once | ||
|
|
||
| #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 |
||
| // is tied to CorProfilerCallback::DisposeServices(). The CLR can invoke these callbacks | ||
| // concurrently with CorProfilerCallback::Shutdown()/DisposeInternal() tearing services down - with | ||
| // no interlock, a callback that reads "the engine looks alive" can still end up touching an | ||
| // already-destroyed (or mid-destruction) service afterward. A real crash was traced to exactly | ||
| // this: ThreadNameChanged read _isInitialized.load() == true, then CorProfilerCallback:: | ||
| // DisposeInternal() destroyed ThreadsCpuManager on another thread before the callback reached its | ||
| // use of it - a stock std::atomic<bool> check has no way to close that gap, because there is | ||
| // nothing serializing "read the flag" against "the object is being destroyed right now". | ||
| // | ||
| // This class checks two things, replacing the single (and, for the shutdown half, racy) | ||
| // `if (false == _isInitialized.load()) return S_OK;` pattern the affected callbacks used to have: | ||
| // - isInitialized: has Initialize() finished constructing the services yet? This direction of | ||
| // the lifecycle never destroys anything concurrently, so a bare atomic read is fine here, same | ||
| // as before. | ||
| // - isServicesShutdown: has teardown started? This direction *does* concurrently destroy things, so it | ||
| // can only ever be answered while holding the same mutex the teardown path holds exclusively | ||
| // while it tears things down - never as a bare, lock-free flag read. | ||
| // | ||
| // Usage in a callback: | ||
| // EngineActiveGuard engineGuard(_isInitialized, _engineLifetimeMutex, _isServicesShutdown); | ||
| // if (!engineGuard.IsActive()) | ||
| // { | ||
| // return S_OK; | ||
| // } | ||
| // ... safe to use service pointers for engineGuard's lifetime ... | ||
| // | ||
| // Usage at teardown (CorProfilerCallback::DisposeInternal()): | ||
| // { | ||
| // std::unique_lock<std::shared_mutex> exclusiveLock(_engineLifetimeMutex); | ||
| // _isServicesShutdown = true; // must be set before the lock is released below, not after | ||
| // DisposeServices(); | ||
| // } // lock releases here (normal RAII) - safe, because any callback that acquires the | ||
| // // lock afterward is guaranteed - via the mutex's own synchronizes-with relationship - | ||
| // // to observe _isServicesShutdown == true, and will no-op before touching any service pointer. | ||
| // | ||
| // _isServicesShutdown must never be read or written except while holding _engineLifetimeMutex (shared or | ||
| // exclusive). Reading it as a bare flag anywhere - even "just this once, for a quick check" - | ||
| // reintroduces the exact race this class exists to close. | ||
| class EngineActiveGuard | ||
| { | ||
| public: | ||
| EngineActiveGuard(std::atomic<bool> const& isInitialized, std::shared_mutex& mutex, bool const& isServicesShutdown) : | ||
| _lock(mutex, std::try_to_lock), | ||
| _isActive(isInitialized.load() && _lock.owns_lock() && !isServicesShutdown) | ||
| { | ||
| } | ||
|
|
||
| ~EngineActiveGuard() = default; | ||
| EngineActiveGuard(EngineActiveGuard const&) = delete; | ||
| EngineActiveGuard& operator=(EngineActiveGuard const&) = delete; | ||
| EngineActiveGuard(EngineActiveGuard&&) = delete; | ||
| EngineActiveGuard& operator=(EngineActiveGuard&&) = delete; | ||
|
|
||
| // True if the engine was confirmed initialized-and-not-yet-shut-down for the duration of this | ||
| // guard's lifetime. Service pointers may only be used while this is true, and only for as | ||
| // long as this guard object stays in scope. | ||
| bool IsActive() const { return _isActive; } | ||
|
|
||
| private: | ||
| std::shared_lock<std::shared_mutex> _lock; | ||
| bool _isActive; | ||
| }; | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,142 @@ | ||
| // Unless explicitly stated otherwise all files in this repository are licensed under the Apache 2 License. | ||
| // This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2022 Datadog, Inc. | ||
|
|
||
| #include "gtest/gtest.h" | ||
|
|
||
| #include "EngineActiveGuard.h" | ||
|
|
||
| #include <atomic> | ||
| #include <chrono> | ||
| #include <future> | ||
| #include <memory> | ||
| #include <shared_mutex> | ||
| #include <thread> | ||
|
|
||
| using namespace std::chrono_literals; | ||
|
|
||
| // These tests cover the fix for a shutdown-time use-after-free: an ICorProfilerCallback method | ||
| // used to check _isInitialized.load() - a bare, lock-free atomic read - then, several lines | ||
| // later, dereference a service pointer that CorProfilerCallback::DisposeInternal() could | ||
| // concurrently null out and destroy on another thread. A real crash (ExecutionEngineException, | ||
| // a ThreadsCpuManager use-after-free deep inside std::unordered_map's internals) was traced to | ||
| // exactly this gap: the check and the use were never serialized against each other at all. | ||
| // | ||
| // EngineActiveGuard replaces that single check with two: isInitialized (has Initialize() | ||
| // finished? - a plain atomic read is fine, this direction never destroys anything concurrently) | ||
| // and isServicesShutdown (has teardown started? - only ever answered while holding the same | ||
| // mutex the teardown path holds exclusively). These tests exercise both halves directly, without | ||
| // needing a full CorProfilerCallback. | ||
|
|
||
| TEST(EngineActiveGuardTest, ActiveWhenInitializedNotShutdownAndUncontended) | ||
| { | ||
| std::atomic<bool> isInitialized{true}; | ||
| std::shared_mutex mutex; | ||
| bool isServicesShutdown = false; | ||
|
|
||
| EngineActiveGuard guard(isInitialized, mutex, isServicesShutdown); | ||
|
|
||
| ASSERT_TRUE(guard.IsActive()); | ||
| } | ||
|
|
||
| TEST(EngineActiveGuardTest, NotActiveBeforeInitialization) | ||
| { | ||
| // Mirrors the "has not yet initialized" half of the original | ||
| // `if (false == _isInitialized.load()) return S_OK;` check - a callback firing before | ||
| // Initialize() has finished constructing the services must still no-op, exactly as before. | ||
| std::atomic<bool> isInitialized{false}; | ||
| std::shared_mutex mutex; | ||
| bool isServicesShutdown = false; | ||
|
|
||
| EngineActiveGuard guard(isInitialized, mutex, isServicesShutdown); | ||
|
|
||
| ASSERT_FALSE(guard.IsActive()); | ||
| } | ||
|
|
||
| TEST(EngineActiveGuardTest, NotActiveOnceServicesShutdownFlagIsSet) | ||
| { | ||
| std::atomic<bool> isInitialized{true}; | ||
| std::shared_mutex mutex; | ||
| bool isServicesShutdown = true; | ||
|
|
||
| // No writer holds the mutex here - a bare try_to_lock would succeed. This is exactly the gap | ||
| // the fix closes: acquiring the lock is not enough on its own, IsActive() must also observe | ||
| // isServicesShutdown while still holding it, or a callback arriving right after teardown | ||
| // releases the lock would sail through and touch already-destroyed service pointers. | ||
| EngineActiveGuard guard(isInitialized, mutex, isServicesShutdown); | ||
|
|
||
| ASSERT_FALSE(guard.IsActive()); | ||
| } | ||
|
|
||
| TEST(EngineActiveGuardTest, NotActiveWhileWriterHoldsTheExclusiveLock) | ||
| { | ||
| std::atomic<bool> isInitialized{true}; | ||
| std::shared_mutex mutex; | ||
| bool isServicesShutdown = false; | ||
|
|
||
| std::promise<void> writerHasTheLock; | ||
| std::promise<void> readerHasChecked; | ||
| auto readerHasCheckedFuture = readerHasChecked.get_future(); | ||
|
|
||
| // Mirrors DisposeInternal() holding the exclusive lock for the duration of DisposeServices(). | ||
| std::thread writer( | ||
| [&] | ||
| { | ||
| std::unique_lock<std::shared_mutex> exclusiveLock(mutex); | ||
| writerHasTheLock.set_value(); | ||
| readerHasCheckedFuture.wait(); | ||
| }); | ||
|
|
||
| writerHasTheLock.get_future().wait(); | ||
|
|
||
| // A callback arriving while teardown is in progress must fail to acquire the lock at all | ||
| // (non-blocking try_to_lock), not block waiting for it - callbacks must never wait. | ||
| EngineActiveGuard guard(isInitialized, mutex, isServicesShutdown); | ||
| ASSERT_FALSE(guard.IsActive()); | ||
|
|
||
| readerHasChecked.set_value(); | ||
| writer.join(); | ||
| } | ||
|
|
||
| TEST(EngineActiveGuardTest, WriterBlocksUntilReaderGuardIsReleased) | ||
| { | ||
| std::atomic<bool> isInitialized{true}; | ||
| std::shared_mutex mutex; | ||
| bool isServicesShutdown = false; | ||
|
|
||
| auto guard = std::make_unique<EngineActiveGuard>(isInitialized, mutex, isServicesShutdown); | ||
| ASSERT_TRUE(guard->IsActive()); | ||
|
|
||
| std::atomic<bool> writerAcquired{false}; | ||
| std::thread writer( | ||
| [&] | ||
| { | ||
| std::unique_lock<std::shared_mutex> exclusiveLock(mutex); | ||
| writerAcquired.store(true); | ||
| }); | ||
|
|
||
| // The writer (teardown) must not be able to proceed while an in-flight callback still holds | ||
| // its shared lock - this is what guarantees a callback that already passed its guard check | ||
| // gets to finish using service pointers before DisposeServices() can destroy them. | ||
| std::this_thread::sleep_for(50ms); | ||
| ASSERT_FALSE(writerAcquired.load()); | ||
|
|
||
| guard.reset(); // release the reader's shared lock | ||
| writer.join(); | ||
|
|
||
| ASSERT_TRUE(writerAcquired.load()); | ||
| } | ||
|
|
||
| TEST(EngineActiveGuardTest, MultipleReadersCanBeActiveConcurrently) | ||
| { | ||
| std::atomic<bool> isInitialized{true}; | ||
| std::shared_mutex mutex; | ||
| bool isServicesShutdown = false; | ||
|
|
||
| // Callbacks legitimately run concurrently on different threads in normal operation - the | ||
| // guard must not serialize them against each other, only against the writer. | ||
| EngineActiveGuard first(isInitialized, mutex, isServicesShutdown); | ||
| EngineActiveGuard second(isInitialized, mutex, isServicesShutdown); | ||
|
|
||
| ASSERT_TRUE(first.IsActive()); | ||
| ASSERT_TRUE(second.IsActive()); | ||
| } |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make it simpler