diff --git a/profiler/src/ProfilerEngine/Datadog.Profiler.Native/CorProfilerCallback.cpp b/profiler/src/ProfilerEngine/Datadog.Profiler.Native/CorProfilerCallback.cpp index 5e5314074149..d6f90e843b1f 100644 --- a/profiler/src/ProfilerEngine/Datadog.Profiler.Native/CorProfilerCallback.cpp +++ b/profiler/src/ProfilerEngine/Datadog.Profiler.Native/CorProfilerCallback.cpp @@ -1051,7 +1051,17 @@ void CorProfilerCallback::DisposeInternal() _pEtwEventsManager->Stop(); } - DisposeServices(); + { + // _isServicesShutdown must be set to true before this lock releases, not after - see + // EngineActiveGuard.h. From this point on, no EngineActiveGuard can ever report + // IsActive() again (the mutex's own synchronizes-with guarantee ensures every future + // lock acquisition, on any thread, observes _isServicesShutdown == true), so no guarded + // ICorProfilerCallback method can race DisposeServices() below, or run after it and + // find already-destroyed service pointers. + std::unique_lock exclusiveLock(_engineLifetimeMutex); + _isServicesShutdown = true; + DisposeServices(); + } ICorProfilerInfo5* pCorProfilerInfo = _pCorProfilerInfo; if (pCorProfilerInfo != nullptr) @@ -1997,6 +2007,15 @@ HRESULT STDMETHODCALLTYPE CorProfilerCallback::AppDomainCreationStarted(AppDomai HRESULT STDMETHODCALLTYPE CorProfilerCallback::AppDomainCreationFinished(AppDomainID appDomainId, HRESULT hrStatus) { + // Previously unguarded entirely (unlike its siblings below, which at least had the racy + // _isInitialized check) - _pRuntimeIdStore is a _services-managed pointer DisposeServices() + // can free concurrently. See EngineActiveGuard.h. + EngineActiveGuard engineGuard(_isInitialized, _engineLifetimeMutex, _isServicesShutdown); + if (!engineGuard.IsActive()) + { + return S_OK; + } + _pAppDomainStore->Register(appDomainId); if (_pConfiguration->GetDeploymentMode() == DeploymentMode::SingleStepInstrumentation) { @@ -2045,9 +2064,9 @@ HRESULT STDMETHODCALLTYPE CorProfilerCallback::ModuleLoadStarted(ModuleID module HRESULT STDMETHODCALLTYPE CorProfilerCallback::ModuleLoadFinished(ModuleID moduleId, HRESULT hrStatus) { - if (false == _isInitialized.load()) + EngineActiveGuard engineGuard(_isInitialized, _engineLifetimeMutex, _isServicesShutdown); + if (!engineGuard.IsActive()) { - // If this CorProfilerCallback has not yet initialized, or if it has already shut down, then this callback is a No-Op. return S_OK; } @@ -2154,9 +2173,9 @@ HRESULT STDMETHODCALLTYPE CorProfilerCallback::ThreadCreated(ThreadID threadId) { Log::Debug("Callback invoked: ThreadCreated(threadId=0x", std::hex, threadId, std::dec, ")"); - if (false == _isInitialized.load()) + EngineActiveGuard engineGuard(_isInitialized, _engineLifetimeMutex, _isServicesShutdown); + if (!engineGuard.IsActive()) { - // If this CorProfilerCallback has not yet initialized, or if it has already shut down, then this callback is a No-Op. return S_OK; } @@ -2208,9 +2227,9 @@ HRESULT STDMETHODCALLTYPE CorProfilerCallback::ThreadDestroyed(ThreadID threadId { Log::Debug("Callback invoked: ThreadDestroyed(threadId=0x", std::hex, threadId, std::dec, ")"); - if (false == _isInitialized.load()) + EngineActiveGuard engineGuard(_isInitialized, _engineLifetimeMutex, _isServicesShutdown); + if (!engineGuard.IsActive()) { - // If this CorProfilerCallback has not yet initialized, or if it has already shut down, then this callback is a No-Op. return S_OK; } @@ -2257,9 +2276,9 @@ HRESULT STDMETHODCALLTYPE CorProfilerCallback::ThreadAssignedToOSThread(ThreadID { Log::Debug("Callback invoked: ThreadAssignedToOSThread(managedThreadId=0x", std::hex, managedThreadId, ", osThreadId=", std::dec, osThreadId, ")"); - if (false == _isInitialized.load()) + EngineActiveGuard engineGuard(_isInitialized, _engineLifetimeMutex, _isServicesShutdown); + if (!engineGuard.IsActive()) { - // If this CorProfilerCallback has not yet initialized, or if it has already shut down, then this callback is a No-Op. return S_OK; } @@ -2368,9 +2387,9 @@ HRESULT STDMETHODCALLTYPE CorProfilerCallback::ThreadAssignedToOSThread(ThreadID HRESULT STDMETHODCALLTYPE CorProfilerCallback::ThreadNameChanged(ThreadID threadId, ULONG cchName, WCHAR name[]) { - if (false == _isInitialized.load()) + EngineActiveGuard engineGuard(_isInitialized, _engineLifetimeMutex, _isServicesShutdown); + if (!engineGuard.IsActive()) { - // If this CorProfilerCallback has not yet initialized, or if it has already shut down, then this callback is a No-Op. return S_OK; } @@ -2505,9 +2524,9 @@ HRESULT STDMETHODCALLTYPE CorProfilerCallback::RootReferences(ULONG cRootRefs, O HRESULT STDMETHODCALLTYPE CorProfilerCallback::ExceptionThrown(ObjectID thrownObjectId) { - if (false == _isInitialized.load()) + EngineActiveGuard engineGuard(_isInitialized, _engineLifetimeMutex, _isServicesShutdown); + if (!engineGuard.IsActive()) { - // If this CorProfilerCallback has not yet initialized, or if it has already shut down, then this callback is a No-Op. return S_OK; } diff --git a/profiler/src/ProfilerEngine/Datadog.Profiler.Native/CorProfilerCallback.h b/profiler/src/ProfilerEngine/Datadog.Profiler.Native/CorProfilerCallback.h index 2d978b0cee45..a4d112f97cc4 100644 --- a/profiler/src/ProfilerEngine/Datadog.Profiler.Native/CorProfilerCallback.h +++ b/profiler/src/ProfilerEngine/Datadog.Profiler.Native/CorProfilerCallback.h @@ -10,6 +10,7 @@ #include "AllocationsProvider.h" #include "ApplicationStore.h" +#include "EngineActiveGuard.h" #include "EventPipeEventsManager.h" #include "ExceptionsProvider.h" #include "IAppDomainStore.h" @@ -50,6 +51,7 @@ #include #include +#include #include class ContentionProvider; @@ -246,6 +248,12 @@ private : std::shared_ptr _metricsSender; std::atomic _isInitialized{false}; // pay attention to keeping ProfilerEngineStatus::IsProfilerEngiveActive in sync with this! + // Guards ICorProfilerCallback methods (see EngineActiveGuard.h) against DisposeInternal() + // concurrently tearing down the service pointers below. _isServicesShutdown must only ever be + // read/written while holding this mutex (shared or exclusive) - never as a bare flag check. + mutable std::shared_mutex _engineLifetimeMutex; + bool _isServicesShutdown = false; + // The pointer here are observable pointer which means that they are used only to access the data. // Their lifetime is managed by the _services vector. IThreadsCpuManager* _pThreadsCpuManager = nullptr; diff --git a/profiler/src/ProfilerEngine/Datadog.Profiler.Native/EngineActiveGuard.h b/profiler/src/ProfilerEngine/Datadog.Profiler.Native/EngineActiveGuard.h new file mode 100644 index 000000000000..9c9aceb3a337 --- /dev/null +++ b/profiler/src/ProfilerEngine/Datadog.Profiler.Native/EngineActiveGuard.h @@ -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 +#include + +// Non-blocking guard for ICorProfilerCallback methods that read service pointers whose lifetime +// 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 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 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 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 _lock; + bool _isActive; +}; diff --git a/profiler/test/Datadog.Profiler.Native.Tests/EngineActiveGuardTest.cpp b/profiler/test/Datadog.Profiler.Native.Tests/EngineActiveGuardTest.cpp new file mode 100644 index 000000000000..a4bccd4ca6a6 --- /dev/null +++ b/profiler/test/Datadog.Profiler.Native.Tests/EngineActiveGuardTest.cpp @@ -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 +#include +#include +#include +#include +#include + +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 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 isInitialized{false}; + std::shared_mutex mutex; + bool isServicesShutdown = false; + + EngineActiveGuard guard(isInitialized, mutex, isServicesShutdown); + + ASSERT_FALSE(guard.IsActive()); +} + +TEST(EngineActiveGuardTest, NotActiveOnceServicesShutdownFlagIsSet) +{ + std::atomic 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 isInitialized{true}; + std::shared_mutex mutex; + bool isServicesShutdown = false; + + std::promise writerHasTheLock; + std::promise readerHasChecked; + auto readerHasCheckedFuture = readerHasChecked.get_future(); + + // Mirrors DisposeInternal() holding the exclusive lock for the duration of DisposeServices(). + std::thread writer( + [&] + { + std::unique_lock 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 isInitialized{true}; + std::shared_mutex mutex; + bool isServicesShutdown = false; + + auto guard = std::make_unique(isInitialized, mutex, isServicesShutdown); + ASSERT_TRUE(guard->IsActive()); + + std::atomic writerAcquired{false}; + std::thread writer( + [&] + { + std::unique_lock 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 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()); +}