Skip to content

Commit

Permalink
Add proper error checking around GetModuleMetadata (#5985 => v2) (#5992)
Browse files Browse the repository at this point in the history 
## Summary of changes

Explicitly check for `S_OK` when calling `GetModuleMetadata`.

## Reason for change

`GetModuleMetadata` can return `S_FALSE` (one known case is if the
module is a resource, but there might be other). We've been using the
`SUCCEEDED`/`FAILED` macros to check the error code, but `S_FALSE` is
not considered as an error so we would mistakenly assume that the call
succeeded.

## Implementation details

Note that I only updated the error checks. There are a couple of places
where we call `GetModuleMetadata` without checking the error code, I
haven't touched those.

For `GetModuleInterfaces` in IAST, I also moved the call to `Release` to
only do it if the call succeeded.
  • Loading branch information
@kevingosse
kevingosse authored Sep 6, 2024
1 parent 4c5c245 commit ff4bb6d
Show file tree
Hide file tree
Showing 8 changed files with 36 additions and 25 deletions.
8 changes: 7 additions & 1 deletion ...ive/instrumented_assembly_generator/instrumented_assembly_generator_cor_profiler_info.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,13 @@ HRESULT CorProfilerInfo::GetModuleMetaData(ModuleID moduleId, DWORD dwOpenFlags,
{
HRESULT hr;
ComPtr<IUnknown> temp;
IfFailRet(m_corProfilerInfo->GetModuleMetaData(moduleId, dwOpenFlags, riid, temp.GetAddressOf()));
hr = m_corProfilerInfo->GetModuleMetaData(moduleId, dwOpenFlags, riid, temp.GetAddressOf());

if (hr != S_OK)
{
return hr;
}

try
{
const auto metadataInterfaces = new MetadataInterfaces(temp);
Expand Down
9 changes: 7 additions & 2 deletions ...rProfiler.Native/instrumented_assembly_generator/instrumented_assembly_generator_helper.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -288,8 +288,13 @@ inline HRESULT WriteILChanges(ModuleID moduleId, mdMethodDef methodToken, LPCBYT
try
{
ComPtr<IUnknown> metadataInterfaces;
IfFailRet(corProfilerInfo->GetModuleMetaData(moduleId, CorOpenFlags::ofRead, IID_IMetaDataImport,
metadataInterfaces.GetAddressOf()));
hr = corProfilerInfo->GetModuleMetaData(moduleId, CorOpenFlags::ofRead, IID_IMetaDataImport,
metadataInterfaces.GetAddressOf());

if (hr != S_OK)
{
return hr;
}

auto metadataImport = metadataInterfaces.As<IMetaDataImport>(IID_IMetaDataImport);

Expand Down
2 changes: 1 addition & 1 deletion tracer/src/Datadog.Tracer.Native/clr_helpers.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1107,7 +1107,7 @@ HRESULT ResolveTypeInternal(ICorProfilerInfo4* info,
ComPtr<IUnknown> metadata_interfaces;
auto hr = info->GetModuleMetaData(moduleId, ofRead, IID_IMetaDataImport2,
metadata_interfaces.GetAddressOf());
if (FAILED(hr))
if (hr != S_OK)
{
Logger::Warn("[ResolveTypeInternal] GetModuleMetaData has failed with: ", shared::WSTRING(refTypeName.data()));
continue;
Expand Down
10 changes: 5 additions & 5 deletions tracer/src/Datadog.Tracer.Native/cor_profiler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -416,7 +416,7 @@ HRESULT STDMETHODCALLTYPE CorProfiler::AssemblyLoadFinished(AssemblyID assembly_
auto hr = this->info_->GetModuleMetaData(assembly_info.manifest_module_id, ofRead,
IID_IMetaDataImport2, metadata_interfaces.GetAddressOf());

if (FAILED(hr))
if (hr != S_OK)
{
Logger::Warn("AssemblyLoadFinished failed to get metadata interface for module id ",
assembly_info.manifest_module_id, " from assembly ", assembly_info.name, " HRESULT=0x",
Expand Down Expand Up @@ -904,7 +904,7 @@ HRESULT CorProfiler::TryRejitModule(ModuleID module_id, std::vector<ModuleID>& m
auto hr = this->info_->GetModuleMetaData(module_id, ofRead | ofWrite, IID_IMetaDataImport2,
metadata_interfaces.GetAddressOf());

if (FAILED(hr))
if (hr != S_OK)
{
Logger::Warn("ModuleLoadFinished failed to get metadata interface for ", module_id, " ",
module_info.assembly.name);
Expand Down Expand Up @@ -1019,7 +1019,7 @@ HRESULT CorProfiler::TryRejitModule(ModuleID module_id, std::vector<ModuleID>& m
auto hr = this->info_->GetModuleMetaData(module_id, ofRead, IID_IMetaDataImport2,
metadata_interfaces.GetAddressOf());

if (FAILED(hr))
if (hr != S_OK)
{
Logger::Warn("ModuleLoadFinished failed to get metadata interface for ", module_id, " ",
module_info.assembly.name);
Expand Down Expand Up @@ -3076,7 +3076,7 @@ HRESULT CorProfiler::GenerateVoidILStartupMethod(const ModuleID module_id, mdMet
ComPtr<IUnknown> metadata_interfaces;
auto hr = this->info_->GetModuleMetaData(module_id, ofRead | ofWrite, IID_IMetaDataImport2,
metadata_interfaces.GetAddressOf());
if (FAILED(hr))
if (hr != S_OK)
{
Logger::Warn("GenerateVoidILStartupMethod: failed to get metadata interface for ", module_id);
return hr;
Expand Down Expand Up @@ -3838,7 +3838,7 @@ HRESULT CorProfiler::AddIISPreStartInitFlags(const ModuleID module_id, const mdT
ComPtr<IUnknown> metadata_interfaces;
auto hr = this->info_->GetModuleMetaData(module_id, ofRead | ofWrite, IID_IMetaDataImport2,
metadata_interfaces.GetAddressOf());
if (FAILED(hr))
if (hr != S_OK)
{
Logger::Warn("GenerateVoidILStartupMethod: failed to get metadata interface for ", module_id);
return hr;
Expand Down
2 changes: 1 addition & 1 deletion tracer/src/Datadog.Tracer.Native/debugger_probes_instrumentation_requester.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -751,7 +751,7 @@ void DebuggerProbesInstrumentationRequester::ModuleLoadFinished_AddMetadataToMod
Logger::Debug(" Loading Assembly Metadata...");
auto hr = corProfilerInfo->GetModuleMetaData(moduleInfo.id, ofRead | ofWrite, IID_IMetaDataImport2,
metadataInterfaces.GetAddressOf());
if (FAILED(hr))
if (hr != S_OK)
{
Logger::Warn(
"DebuggerProbesInstrumentationRequester::sAddMetadataToModule failed to get metadata interface for ",
Expand Down
2 changes: 1 addition & 1 deletion tracer/src/Datadog.Tracer.Native/debugger_rejit_preprocessor.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ ULONG DebuggerRejitPreprocessor::PreprocessLineProbes(
Logger::Debug(" Loading Assembly Metadata...");
auto hr = corProfilerInfo->GetModuleMetaData(moduleInfo.id, ofRead | ofWrite, IID_IMetaDataImport2,
metadataInterfaces.GetAddressOf());
if (FAILED(hr))
if (hr != S_OK)
{
Logger::Warn("CallTarget_RequestRejitForModule failed to get metadata interface for ", moduleInfo.id, " ",
moduleInfo.assembly.name);
Expand Down
24 changes: 12 additions & 12 deletions tracer/src/Datadog.Tracer.Native/iast/dataflow.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -406,45 +406,45 @@ HRESULT Dataflow::GetModuleInterfaces(ModuleID moduleId, IMetaDataImport2** ppMe
IMetaDataAssemblyEmit** ppAssemblyEmit)
{
HRESULT hr = S_OK;
if (SUCCEEDED(hr))
if (hr == S_OK)
{
IUnknown* piUnk = nullptr;
hr = _profiler->GetModuleMetaData(moduleId, ofRead | ofWrite, IID_IMetaDataImport2, &piUnk);
if (SUCCEEDED(hr))
if (hr == S_OK)
{
hr = piUnk->QueryInterface(IID_IMetaDataImport2, (void**) ppMetadataImport);
REL(piUnk);
}
REL(piUnk);
}
if (SUCCEEDED(hr))
if (hr == S_OK)
{
IUnknown* piUnk = nullptr;
hr = _profiler->GetModuleMetaData(moduleId, ofRead | ofWrite, IID_IMetaDataEmit2, &piUnk);
if (SUCCEEDED(hr))
if (hr == S_OK)
{
hr = piUnk->QueryInterface(IID_IMetaDataEmit2, (void**) ppMetadataEmit);
REL(piUnk);
}
REL(piUnk);
}
if (SUCCEEDED(hr))
if (hr == S_OK)
{
IUnknown* piUnk = nullptr;
hr = _profiler->GetModuleMetaData(moduleId, ofRead | ofWrite, IID_IMetaDataAssemblyImport, &piUnk);
if (SUCCEEDED(hr))
if (hr == S_OK)
{
hr = piUnk->QueryInterface(IID_IMetaDataAssemblyImport, (void**) ppAssemblyImport);
REL(piUnk);
}
REL(piUnk);
}
if (SUCCEEDED(hr))
if (hr == S_OK)
{
IUnknown* piUnk = nullptr;
hr = _profiler->GetModuleMetaData(moduleId, ofRead | ofWrite, IID_IMetaDataAssemblyEmit, &piUnk);
if (SUCCEEDED(hr))
if (hr == S_OK)
{
hr = piUnk->QueryInterface(IID_IMetaDataAssemblyEmit, (void**) ppAssemblyEmit);
REL(piUnk);
}
REL(piUnk);
}
return hr;
}
Expand Down
4 changes: 2 additions & 2 deletions tracer/src/Datadog.Tracer.Native/rejit_preprocessor.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -576,7 +576,7 @@ ULONG RejitPreprocessor<RejitRequestDefinition>::PreprocessRejitRequests(
Logger::Debug(" Loading Assembly Metadata...");
auto hr = corProfilerInfo->GetModuleMetaData(moduleInfo.id, ofRead | ofWrite, IID_IMetaDataImport2,
metadataInterfaces.GetAddressOf());
if (FAILED(hr))
if (hr != S_OK)
{
Logger::Warn("CallTarget_RequestRejitForModule failed to get metadata interface for ",
moduleInfo.id, " ", moduleInfo.assembly.name);
Expand Down Expand Up @@ -799,7 +799,7 @@ ULONG RejitPreprocessor<RejitRequestDefinition>::PreprocessRejitRequests(
Logger::Debug(" Loading Assembly Metadata...");
auto hr = corProfilerInfo->GetModuleMetaData(moduleInfo.id, ofRead | ofWrite, IID_IMetaDataImport2,
metadataInterfaces.GetAddressOf());
if (FAILED(hr))
if (hr != S_OK)
{
Logger::Warn("CallTarget_RequestRejitForModule failed to get metadata interface for ",
moduleInfo.id, " ", moduleInfo.assembly.name);
Expand Down

0 comments on commit ff4bb6d

Please sign in to comment.