Skip to content

Commit

Permalink
Revert "Load the tracer/profiler after guardrails checks" (#6200)
Browse files Browse the repository at this point in the history 
Reverts #5968

It seems it causes a crash when .NET Framework and .NET Core are loaded
in the same process.
  • Loading branch information
@kevingosse
kevingosse authored Oct 25, 2024
1 parent 47a2861 commit 4d9df8b
Show file tree
Hide file tree
Showing 9 changed files with 46 additions and 164 deletions.
23 changes: 0 additions & 23 deletions Datadog.Trace.Native.sln
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,6 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Datadog.Trace.ClrProfiler.M
EndProject
Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "Datadog.Trace.ClrProfiler.Native", "shared\src\Datadog.Trace.ClrProfiler.Native\Datadog.Trace.ClrProfiler.Native.vcxproj", "{48D12C26-6E63-419C-BFE2-E668E8550613}"
EndProject
Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "Datadog.Trace.ClrProfiler.Native.Tests", "shared\test\Datadog.Trace.ClrProfiler.Native.Tests\Datadog.Trace.ClrProfiler.Native.Tests.vcxproj", "{3FCBD2EB-ACBE-4DA1-850F-12BEE08B937B}"
EndProject
Global
GlobalSection(SolutionConfigurationPlatforms) = preSolution
Debug|Any CPU = Debug|Any CPU
Expand Down Expand Up @@ -178,26 +176,6 @@ Global
{48D12C26-6E63-419C-BFE2-E668E8550613}.Release|x64.Build.0 = Release|x64
{48D12C26-6E63-419C-BFE2-E668E8550613}.Release|x86.ActiveCfg = Release|Win32
{48D12C26-6E63-419C-BFE2-E668E8550613}.Release|x86.Build.0 = Release|Win32
{3FCBD2EB-ACBE-4DA1-850F-12BEE08B937B}.Debug|Any CPU.ActiveCfg = Debug|x64
{3FCBD2EB-ACBE-4DA1-850F-12BEE08B937B}.Debug|Any CPU.Build.0 = Debug|x64
{3FCBD2EB-ACBE-4DA1-850F-12BEE08B937B}.Debug|ARM64.ActiveCfg = Debug|ARM64
{3FCBD2EB-ACBE-4DA1-850F-12BEE08B937B}.Debug|ARM64.Build.0 = Debug|ARM64
{3FCBD2EB-ACBE-4DA1-850F-12BEE08B937B}.Debug|ARM64EC.ActiveCfg = Debug|x64
{3FCBD2EB-ACBE-4DA1-850F-12BEE08B937B}.Debug|ARM64EC.Build.0 = Debug|x64
{3FCBD2EB-ACBE-4DA1-850F-12BEE08B937B}.Debug|x64.ActiveCfg = Debug|x64
{3FCBD2EB-ACBE-4DA1-850F-12BEE08B937B}.Debug|x64.Build.0 = Debug|x64
{3FCBD2EB-ACBE-4DA1-850F-12BEE08B937B}.Debug|x86.ActiveCfg = Debug|Win32
{3FCBD2EB-ACBE-4DA1-850F-12BEE08B937B}.Debug|x86.Build.0 = Debug|Win32
{3FCBD2EB-ACBE-4DA1-850F-12BEE08B937B}.Release|Any CPU.ActiveCfg = Release|x64
{3FCBD2EB-ACBE-4DA1-850F-12BEE08B937B}.Release|Any CPU.Build.0 = Release|x64
{3FCBD2EB-ACBE-4DA1-850F-12BEE08B937B}.Release|ARM64.ActiveCfg = Release|ARM64
{3FCBD2EB-ACBE-4DA1-850F-12BEE08B937B}.Release|ARM64.Build.0 = Release|ARM64
{3FCBD2EB-ACBE-4DA1-850F-12BEE08B937B}.Release|ARM64EC.ActiveCfg = Release|x64
{3FCBD2EB-ACBE-4DA1-850F-12BEE08B937B}.Release|ARM64EC.Build.0 = Release|x64
{3FCBD2EB-ACBE-4DA1-850F-12BEE08B937B}.Release|x64.ActiveCfg = Release|x64
{3FCBD2EB-ACBE-4DA1-850F-12BEE08B937B}.Release|x64.Build.0 = Release|x64
{3FCBD2EB-ACBE-4DA1-850F-12BEE08B937B}.Release|x86.ActiveCfg = Release|Win32
{3FCBD2EB-ACBE-4DA1-850F-12BEE08B937B}.Release|x86.Build.0 = Release|Win32
EndGlobalSection
GlobalSection(SolutionProperties) = preSolution
HideSolutionNode = FALSE
Expand All @@ -210,7 +188,6 @@ Global
{6CE95C50-9533-4650-8F11-BCE30908DCDF} = {B9AA20A4-0F9A-47FB-B3BE-A5BDEA42EFF0}
{0686E907-996A-4D6D-A685-D9C0F932C405} = {9E5F0022-0A50-40BF-AC6A-C3078585ECAB}
{48D12C26-6E63-419C-BFE2-E668E8550613} = {9E5F0022-0A50-40BF-AC6A-C3078585ECAB}
{3FCBD2EB-ACBE-4DA1-850F-12BEE08B937B} = {8CEC2042-F11C-49F5-A674-2355793B600A}
EndGlobalSection
GlobalSection(ExtensibilityGlobals) = postSolution
SolutionGuid = {160A1D00-1F5B-40F8-A155-621B4459D78F}
Expand Down
14 changes: 1 addition & 13 deletions shared/src/Datadog.Trace.ClrProfiler.Native/cor_profiler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -257,25 +257,13 @@ namespace datadog::shared::nativeloader
}

//
// Initialize the dispatcher
// Get and set profiler pointers
//
if (m_dispatcher == nullptr)
{
Log::Error("Dispatcher is not set.");
single_step_guard_rails.RecordBootstrapError(runtimeInformation, "initialization_error");
return E_FAIL;
}

if (FAILED(m_dispatcher->Initialize()))
{
Log::Error("Error initializing the dispatcher.");
single_step_guard_rails.RecordBootstrapError(runtimeInformation, "initialization_error");
return E_FAIL;
}

//
// Get and set profiler pointers
//
IDynamicInstance* cpInstance = m_dispatcher->GetContinuousProfilerInstance();
if (cpInstance != nullptr)
{
Expand Down
14 changes: 12 additions & 2 deletions shared/src/Datadog.Trace.ClrProfiler.Native/cor_profiler_class_factory.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,13 @@ HRESULT STDMETHODCALLTYPE CorProfilerClassFactory::QueryInterface(REFIID riid, v
{
*ppvObject = this;
this->AddRef();


// We try to load the class factory of all target cor profilers.
if (FAILED(m_dispatcher->LoadClassFactory(riid)))
{
Log::Warn("Error loading all cor profiler class factories.");
}

return S_OK;
}

Expand Down Expand Up @@ -65,7 +71,11 @@ HRESULT STDMETHODCALLTYPE CorProfilerClassFactory::CreateInstance(IUnknown* pUnk

auto profiler = new datadog::shared::nativeloader::CorProfiler(m_dispatcher);
HRESULT res = profiler->QueryInterface(riid, ppvObject);
if (FAILED(res))
if (SUCCEEDED(res))
{
m_dispatcher->LoadInstance(pUnkOuter, riid);
}
else
{
delete profiler;
}
Expand Down
3 changes: 2 additions & 1 deletion shared/src/Datadog.Trace.ClrProfiler.Native/dllmain.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,8 @@ EXTERN_C BOOL STDMETHODCALLTYPE DllMain(HMODULE hModule, DWORD ul_reason_for_cal
}
#endif

dispatcher = new DynamicDispatcherImpl();
dispatcher = new DynamicDispatcherImpl();
dispatcher->LoadConfiguration(GetConfigurationFilePath());

// *****************************************************************************************************************
break;
Expand Down
127 changes: 20 additions & 107 deletions shared/src/Datadog.Trace.ClrProfiler.Native/dynamic_dispatcher.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,43 +22,10 @@ namespace datadog::shared::nativeloader
DynamicDispatcherImpl::DynamicDispatcherImpl() :
m_continuousProfilerInstance(nullptr),
m_tracerInstance(nullptr),
m_customInstance(nullptr),
m_initialized(false),
m_initializationResult(E_UNEXPECTED)
m_customInstance(nullptr)
{
}

HRESULT DynamicDispatcherImpl::Initialize()
{
if (m_initialized)
{
return m_initializationResult;
}

m_initialized = true;

LoadConfiguration(GetConfigurationFilePath());

m_initializationResult = LoadClassFactory(IID_IClassFactory);

if (FAILED(m_initializationResult))
{
Log::Error("Error loading all cor profiler class factories.");
return m_initializationResult;
}

m_initializationResult = LoadInstance();

if (FAILED(m_initializationResult))
{
Log::Error("Error loading all cor profiler instances.");
return m_initializationResult;
}

m_initializationResult = S_OK;
return m_initializationResult;
}

void DynamicDispatcherImpl::LoadConfiguration(fs::path&& configFilePath)
{
::shared::WSTRING nativeLoaderPath = ::shared::GetCurrentModuleFileName();
Expand Down Expand Up @@ -190,28 +157,19 @@ namespace datadog::shared::nativeloader

HRESULT DynamicDispatcherImpl::LoadClassFactory(REFIID riid)
{
// We consider the loading a success if at least one class factory is properly loaded.
HRESULT GHR = E_FAIL;
HRESULT GHR = S_OK;

if (m_continuousProfilerInstance != nullptr)
{
HRESULT result = m_continuousProfilerInstance->LoadClassFactory(riid);
if (FAILED(result))
{
Log::Warn("DynamicDispatcherImpl::LoadClassFactory: Error trying to load continuous profiler class factory in: ",
m_continuousProfilerInstance->GetFilePath(), ", error code: ", result);
m_continuousProfilerInstance->GetFilePath());

// If we cannot load the class factory we release the instance.
m_continuousProfilerInstance.release();

if (GHR != S_OK)
{
GHR = result;
}
}
else
{
GHR = S_OK;
GHR = result;
}
}

Expand All @@ -220,20 +178,11 @@ namespace datadog::shared::nativeloader
HRESULT result = m_tracerInstance->LoadClassFactory(riid);
if (FAILED(result))
{
Log::Warn("DynamicDispatcherImpl::LoadClassFactory: Error trying to load tracer class factory in: ",
m_tracerInstance->GetFilePath(), ", error code: ", result);
Log::Warn("DynamicDispatcherImpl::LoadClassFactory: Error trying to load tracer class factory in: ", m_tracerInstance->GetFilePath());

// If we cannot load the class factory we release the instance.
m_tracerInstance.release();

if (GHR != S_OK)
{
GHR = result;
}
}
else
{
GHR = S_OK;
GHR = result;
}
}

Expand All @@ -242,94 +191,58 @@ namespace datadog::shared::nativeloader
HRESULT result = m_customInstance->LoadClassFactory(riid);
if (FAILED(result))
{
Log::Warn("DynamicDispatcherImpl::LoadClassFactory: Error trying to load custom class factory in: ",
m_customInstance->GetFilePath(), ", error code: ", result);
Log::Warn("DynamicDispatcherImpl::LoadClassFactory: Error trying to load custom class factory in: ", m_customInstance->GetFilePath());

// If we cannot load the class factory we release the instance.
m_customInstance.release();

if (GHR != S_OK)
{
GHR = result;
}
}
else
{
GHR = S_OK;
GHR = result;
}
}

return GHR;
}

HRESULT DynamicDispatcherImpl::LoadInstance()
HRESULT DynamicDispatcherImpl::LoadInstance(IUnknown* pUnkOuter, REFIID riid)
{
// We consider the loading a success if at least one class factory is properly loaded.
HRESULT GHR = E_FAIL;
HRESULT GHR = S_OK;

if (m_continuousProfilerInstance != nullptr)
{
HRESULT result = m_continuousProfilerInstance->LoadInstance();
HRESULT result = m_continuousProfilerInstance->LoadInstance(pUnkOuter, riid);
if (FAILED(result))
{
Log::Warn("DynamicDispatcherImpl::LoadInstance: Error trying to load the continuous profiler instance in: ",
m_continuousProfilerInstance->GetFilePath(), ", error code: ", result);
m_continuousProfilerInstance->GetFilePath());

// If we cannot load the class factory we release the instance.
m_continuousProfilerInstance.release();

if (GHR != S_OK)
{
GHR = result;
}
}
else
{
GHR = S_OK;
GHR = result;
}
}

if (m_tracerInstance != nullptr)
{
HRESULT result = m_tracerInstance->LoadInstance();
HRESULT result = m_tracerInstance->LoadInstance(pUnkOuter, riid);
if (FAILED(result))
{
Log::Warn("DynamicDispatcherImpl::LoadInstance: Error trying to load the tracer instance in: ",
m_tracerInstance->GetFilePath(), ", error code: ", result);
Log::Warn("DynamicDispatcherImpl::LoadInstance: Error trying to load the tracer instance in: ", m_tracerInstance->GetFilePath());

// If we cannot load the class factory we release the instance.
m_tracerInstance.release();

if (GHR != S_OK)
{
GHR = result;
}
}
else
{
GHR = S_OK;
GHR = result;
}
}

if (m_customInstance != nullptr)
{
HRESULT result = m_customInstance->LoadInstance();
HRESULT result = m_customInstance->LoadInstance(pUnkOuter, riid);
if (FAILED(result))
{
Log::Warn("DynamicDispatcherImpl::LoadInstance: Error trying to load the custom instance in: ",
m_customInstance->GetFilePath(), ", error code: ", result);
Log::Warn("DynamicDispatcherImpl::LoadInstance: Error trying to load the custom instance in: ", m_customInstance->GetFilePath());

// If we cannot load the class factory we release the instance.
m_customInstance.release();

if (GHR != S_OK)
{
GHR = result;
}
}
else
{
GHR = S_OK;
GHR = result;
}
}

Expand Down Expand Up @@ -404,4 +317,4 @@ namespace datadog::shared::nativeloader
return m_customInstance.get();
}

} // namespace datadog::shared::nativeloader
} // namespace datadog::shared::nativeloader
19 changes: 6 additions & 13 deletions shared/src/Datadog.Trace.ClrProfiler.Native/dynamic_dispatcher.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,11 @@ namespace datadog::shared::nativeloader
//
class IDynamicDispatcher
{
protected:
virtual void LoadConfiguration(fs::path&& configFilePath) = 0;

public:
virtual ~IDynamicDispatcher() = default;
virtual HRESULT Initialize() = 0;
virtual void LoadConfiguration(fs::path&& configFilePath) = 0;
virtual HRESULT LoadClassFactory(REFIID riid) = 0;
virtual HRESULT LoadInstance(IUnknown* pUnkOuter, REFIID riid) = 0;
virtual HRESULT STDMETHODCALLTYPE DllCanUnloadNow() = 0;
virtual IDynamicInstance* GetContinuousProfilerInstance() = 0;
virtual IDynamicInstance* GetTracerInstance() = 0;
Expand All @@ -39,22 +38,16 @@ namespace datadog::shared::nativeloader
std::unique_ptr<IDynamicInstance> m_continuousProfilerInstance;
std::unique_ptr<IDynamicInstance> m_tracerInstance;
std::unique_ptr<IDynamicInstance> m_customInstance;
void LoadConfiguration(fs::path&& configFilePath) override;

public:
DynamicDispatcherImpl();
HRESULT Initialize() override;
void LoadConfiguration(fs::path&& configFilePath) override;
HRESULT LoadClassFactory(REFIID riid) override;
HRESULT LoadInstance(IUnknown* pUnkOuter, REFIID riid) override;
HRESULT STDMETHODCALLTYPE DllCanUnloadNow() override;
IDynamicInstance* GetContinuousProfilerInstance() override;
IDynamicInstance* GetTracerInstance() override;
IDynamicInstance* GetCustomInstance() override;

private:
HRESULT LoadClassFactory(REFIID riid);
HRESULT LoadInstance();

bool m_initialized;
HRESULT m_initializationResult;
};

} // namespace datadog::shared::nativeloader
2 changes: 1 addition & 1 deletion shared/src/Datadog.Trace.ClrProfiler.Native/dynamic_instance.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ namespace datadog::shared::nativeloader
return res;
}

HRESULT DynamicInstanceImpl::LoadInstance()
HRESULT DynamicInstanceImpl::LoadInstance(IUnknown* pUnkOuter, REFIID riid)
{
Log::Debug("DynamicInstanceImpl::LoadInstance");

Expand Down
4 changes: 2 additions & 2 deletions shared/src/Datadog.Trace.ClrProfiler.Native/dynamic_instance.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ namespace datadog::shared::nativeloader
public:
virtual ~IDynamicInstance() = default;
virtual HRESULT LoadClassFactory(REFIID riid) = 0;
virtual HRESULT LoadInstance() = 0;
virtual HRESULT LoadInstance(IUnknown* pUnkOuter, REFIID riid) = 0;
virtual HRESULT STDMETHODCALLTYPE DllCanUnloadNow() = 0;
virtual ICorProfilerCallback10* GetProfilerCallback() = 0;
virtual std::string GetFilePath() = 0;
Expand All @@ -42,7 +42,7 @@ namespace datadog::shared::nativeloader
DynamicInstanceImpl(const std::string& filePath, const std::string& clsid);
~DynamicInstanceImpl() override;
HRESULT LoadClassFactory(REFIID riid) override;
HRESULT LoadInstance() override;
HRESULT LoadInstance(IUnknown* pUnkOuter, REFIID riid) override;
HRESULT STDMETHODCALLTYPE DllCanUnloadNow() override;
ICorProfilerCallback10* GetProfilerCallback() override;
std::string GetFilePath() override;
Expand Down
Loading

0 comments on commit 4d9df8b

Please sign in to comment.