Skip to content

Commit

Permalink
Merge pull request #27 from albertziegenhagel/fix-dataitems
Browse files Browse the repository at this point in the history 
Fix use-after-free related to DataItems for breakpoints
  • Loading branch information
@albertziegenhagel
albertziegenhagel authored Oct 7, 2024
2 parents 09931ce + f0f8626 commit c025f58
Show file tree
Hide file tree
Showing 4 changed files with 86 additions and 92 deletions.
4 changes: 2 additions & 2 deletions .github/workflows/ci.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ jobs:
cd build/x64
cmake "${{ github.workspace }}\vsdbg-engine-extension" `
-G Ninja `
-DCMAKE_BUILD_TYPE=Debug `
-DCMAKE_BUILD_TYPE=RelWithDebInfo `
-DCMAKE_PREFIX_PATH="${{ github.workspace }}\Microsoft.VSSDK.Debugger.VSDebugEng.17.0.2012801\build\native;${{ github.workspace }}\Microsoft.VSSDK.Debugger.VSDConfigTool.17.0.2012801\build" `
-DCMAKE_INSTALL_PREFIX="${{ github.workspace }}\vsdbg-engine-extension" `
-DCHILDDEBUGGER_INSTALL_PDB=ON
Expand All @@ -70,7 +70,7 @@ jobs:
cd build/x86
cmake "${{ github.workspace }}\vsdbg-engine-extension" `
-G Ninja `
-DCMAKE_BUILD_TYPE=Debug `
-DCMAKE_BUILD_TYPE=RelWithDebInfo `
-DCMAKE_PREFIX_PATH="${{ github.workspace }}\Microsoft.VSSDK.Debugger.VSDebugEng.17.0.2012801\build\native;${{ github.workspace }}\Microsoft.VSSDK.Debugger.VSDConfigTool.17.0.2012801\build" `
-DCMAKE_INSTALL_PREFIX="${{ github.workspace }}\vsdbg-engine-extension" `
-DCHILDDEBUGGER_INSTALL_PDB=ON
Expand Down
4 changes: 2 additions & 2 deletions .github/workflows/package.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ jobs:
cd build/x64
cmake "${{ github.workspace }}\vsdbg-engine-extension" `
-G Ninja `
-DCMAKE_BUILD_TYPE=Debug `
-DCMAKE_BUILD_TYPE=RelWithDebInfo `
-DCMAKE_PREFIX_PATH="${{ github.workspace }}\Microsoft.VSSDK.Debugger.VSDebugEng.17.0.2012801\build\native;${{ github.workspace }}\Microsoft.VSSDK.Debugger.VSDConfigTool.17.0.2012801\build" `
-DCMAKE_INSTALL_PREFIX="${{ github.workspace }}\vsdbg-engine-extension" `
-DCHILDDEBUGGER_INSTALL_PDB=ON
Expand All @@ -71,7 +71,7 @@ jobs:
cd build/x86
cmake "${{ github.workspace }}\vsdbg-engine-extension" `
-G Ninja `
-DCMAKE_BUILD_TYPE=Debug `
-DCMAKE_BUILD_TYPE=RelWithDebInfo `
-DCMAKE_PREFIX_PATH="${{ github.workspace }}\Microsoft.VSSDK.Debugger.VSDebugEng.17.0.2012801\build\native;${{ github.workspace }}\Microsoft.VSSDK.Debugger.VSDConfigTool.17.0.2012801\build" `
-DCMAKE_INSTALL_PREFIX="${{ github.workspace }}\vsdbg-engine-extension" `
-DCHILDDEBUGGER_INSTALL_PDB=ON
Expand Down
30 changes: 20 additions & 10 deletions vsdbg-engine-extension/src/ChildDebuggerService.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -250,8 +250,14 @@ HRESULT handle_call_to_create_process(
}

// Create a new breakpoint to be triggered when the child process creation is done.
CComPtr<CreateOutInfo> out_info;
out_info.Attach(new CreateOutInfo(function_call_context.get_lpProcessInformation(), forced_suspension));
CComObject<CreateOutInfoDataItem>* com_obj;
if(auto hr = CComObject<CreateOutInfoDataItem>::CreateInstance(&com_obj); FAILED(hr))
{
logger.log(LogLevel::error, thread.Connection(), L" FAILED to create out-info ComObject instance\n");
return hr;
}
const CComPtr<CreateOutInfoDataItem> out_info(com_obj);
out_info->initialize(function_call_context.get_lpProcessInformation(), forced_suspension);

CComPtr<Breakpoints::DkmRuntimeInstructionBreakpoint> breakpoint;
if(Breakpoints::DkmRuntimeInstructionBreakpoint::Create(source_id, nullptr, address, false, out_info, &breakpoint) != S_OK)
Expand Down Expand Up @@ -536,8 +542,14 @@ HRESULT STDMETHODCALLTYPE CChildDebuggerService::OnModuleInstanceLoad(
}();

// Attach some information to the breakpoint about the function it has been generated for.
CComPtr<CreateInInfo> in_info;
in_info.Attach(new CreateInInfo(function_name->Value()[function_name->Length() - 1] == L'W', function_type));
CComObject<CreateInInfoDataItem>* com_obj;
if(auto hr = CComObject<CreateInInfoDataItem>::CreateInstance(&com_obj); FAILED(hr))
{
logger_.log(LogLevel::error, module_instance->Connection(), L" FAILED to create in-info ComObject instance\n");
return hr;
}
const CComPtr<CreateInInfoDataItem> in_info(com_obj);
in_info->initialize(function_name->Value()[function_name->Length() - 1] == L'W', function_type);

CComPtr<Breakpoints::DkmRuntimeInstructionBreakpoint> breakpoint;
if(Breakpoints::DkmRuntimeInstructionBreakpoint::Create(source_id, nullptr, address, false, in_info, &breakpoint) != S_OK)
Expand Down Expand Up @@ -569,9 +581,8 @@ HRESULT STDMETHODCALLTYPE CChildDebuggerService::OnRuntimeBreakpoint(
StringFromGUID2(runtime_breakpoint->SourceId(), guid_str.data(), guid_str.size());
logger_.log(LogLevel::trace, thread->Connection(), L" Source ID: {}\n", guid_str.data());

CComPtr<CreateInInfo> in_info;
runtime_breakpoint->GetDataItem(&in_info);
if(in_info != nullptr)
CComPtr<CreateInInfoDataItem> in_info;
if(auto hr = runtime_breakpoint->GetDataItem(&in_info); SUCCEEDED(hr) && in_info != nullptr)
{
// This is a breakpoint when entering a process creation function.
// We will do the following things:
Expand All @@ -598,9 +609,8 @@ HRESULT STDMETHODCALLTYPE CChildDebuggerService::OnRuntimeBreakpoint(
}
}

CComPtr<CreateOutInfo> out_info;
runtime_breakpoint->GetDataItem(&out_info);
if(out_info != nullptr)
CComPtr<CreateOutInfoDataItem> out_info;
if(auto hr = runtime_breakpoint->GetDataItem(&out_info); SUCCEEDED(hr) && out_info != nullptr)
{
// This is a breakpoint when a process creation has been completed.
// We will do the following things:
Expand Down
140 changes: 62 additions & 78 deletions vsdbg-engine-extension/src/DataItems.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,80 +4,29 @@

#include "CreateFunctionType.hpp"

// Base class for breakpoint information classes.
template<typename Interface>
class BaseObject : public Interface
{
volatile LONG ref_count_;

public:
virtual ~BaseObject() = default;

BaseObject() = default;

BaseObject(BaseObject&) = delete;
BaseObject(BaseObject&&) = delete;
BaseObject& operator=(BaseObject&) = delete;
BaseObject& operator=(BaseObject&&) = delete;

ULONG __stdcall AddRef() override
{
return (ULONG)InterlockedIncrement(&ref_count_);
}
ULONG __stdcall Release() override
{
auto result = (ULONG)InterlockedDecrement(&ref_count_);
if(result == 0)
{
delete this;
}
return result;
}
HRESULT __stdcall QueryInterface(REFIID riid, _Deref_out_ void** ppv) override
{
if(riid == __uuidof(IUnknown))
{
*ppv = static_cast<IUnknown*>(this);
AddRef();
return S_OK;
}
// This example is implementing the optional interface IDkmDisposableDataItem
if(riid == __uuidof(Microsoft::VisualStudio::Debugger::IDkmDisposableDataItem))
{
*ppv = static_cast<Microsoft::VisualStudio::Debugger::IDkmDisposableDataItem*>(this);
AddRef();
return S_OK;
}

*ppv = nullptr;
return E_NOINTERFACE;
}
};

// Class holding additional information for breakpoints to be triggered on
// a call to any of the process creation functions.
class __declspec(uuid("{1483C347-BDAD-4626-B33F-D16970542239}")) CreateInInfo :
public BaseObject<Microsoft::VisualStudio::Debugger::IDkmDisposableDataItem>
class __declspec(uuid("{1483C347-BDAD-4626-B33F-D16970542239}")) CreateInInfoDataItem :
public IUnknown,
public CComObjectRootEx<CComMultiThreadModel>
{
bool isUnicode_;
CreateFunctionType functionType_;

public:
explicit CreateInInfo(bool is_unicode, CreateFunctionType function_type) :
isUnicode_(is_unicode),
functionType_(function_type)
{}
explicit CreateInInfoDataItem() = default;

CreateInInfo(CreateInInfo&) = delete;
CreateInInfo(CreateInInfo&&) = delete;
CreateInInfo& operator=(CreateInInfo&) = delete;
CreateInInfo& operator=(CreateInInfo&&) = delete;
CreateInInfoDataItem(CreateInInfoDataItem&) = delete;
CreateInInfoDataItem(CreateInInfoDataItem&&) = delete;
CreateInInfoDataItem& operator=(CreateInInfoDataItem&) = delete;
CreateInInfoDataItem& operator=(CreateInInfoDataItem&&) = delete;

~CreateInInfo() override = default;
~CreateInInfoDataItem() = default;

HRESULT __stdcall OnClose() override
void initialize(bool is_unicode, CreateFunctionType function_type)
{
return S_OK;
isUnicode_ = is_unicode;
functionType_ = function_type;
}

[[nodiscard]] bool get_is_unicode() const
Expand All @@ -89,34 +38,51 @@ class __declspec(uuid("{1483C347-BDAD-4626-B33F-D16970542239}")) CreateInInfo :
{
return functionType_;
}

protected:
// NOLINTNEXTLINE(bugprone-reserved-identifier, readability-identifier-naming)
HRESULT _InternalQueryInterface(REFIID riid, void** object)
{
if(object == nullptr)
return E_POINTER;

if(riid == __uuidof(IUnknown))
{
*object = static_cast<IUnknown*>(this);
AddRef();
return S_OK;
}

*object = nullptr;
return E_NOINTERFACE;
}
};

// Class holding additional information for breakpoints to be triggered when
// we return from a call to any of the process creation functions.
class __declspec(uuid("{F1AB4299-C3EB-47C5-83B7-813E28B9DA89}")) CreateOutInfo :
public BaseObject<Microsoft::VisualStudio::Debugger::IDkmDisposableDataItem>
class __declspec(uuid("{F1AB4299-C3EB-47C5-83B7-813E28B9DA89}")) CreateOutInfoDataItem :
public IUnknown,
public CComObjectRootEx<CComMultiThreadModel>
{
UINT64 lpProcessInformation_;
bool suspended_;

public:
explicit CreateOutInfo(UINT64 process_information, bool suspended) :
lpProcessInformation_(process_information),
suspended_(suspended)
{}

CreateOutInfo(CreateOutInfo&) = delete;
CreateOutInfo(CreateOutInfo&&) = delete;
CreateOutInfo& operator=(CreateOutInfo&) = delete;
CreateOutInfo& operator=(CreateOutInfo&&) = delete;
explicit CreateOutInfoDataItem() = default;

~CreateOutInfo() override = default;

HRESULT __stdcall OnClose() override
void initialize(UINT64 process_information, bool suspended)
{
return S_OK;
lpProcessInformation_ = process_information;
suspended_ = suspended;
}

CreateOutInfoDataItem(CreateOutInfoDataItem&) = delete;
CreateOutInfoDataItem(CreateOutInfoDataItem&&) = delete;
CreateOutInfoDataItem& operator=(CreateOutInfoDataItem&) = delete;
CreateOutInfoDataItem& operator=(CreateOutInfoDataItem&&) = delete;

~CreateOutInfoDataItem() = default;

[[nodiscard]] UINT64 get_process_information_address() const
{
return lpProcessInformation_;
Expand All @@ -126,6 +92,24 @@ class __declspec(uuid("{F1AB4299-C3EB-47C5-83B7-813E28B9DA89}")) CreateOutInfo :
{
return suspended_;
}

protected:
// NOLINTNEXTLINE(bugprone-reserved-identifier, readability-identifier-naming)
HRESULT _InternalQueryInterface(REFIID riid, void** object)
{
if(object == nullptr)
return E_POINTER;

if(riid == __uuidof(IUnknown))
{
*object = static_cast<IUnknown*>(this);
AddRef();
return S_OK;
}

*object = nullptr;
return E_NOINTERFACE;
}
};

class ATL_NO_VTABLE __declspec(uuid("{0709D0FC-76B1-44E8-B781-E8C43461CFAC}")) ChildProcessDataItem :
Expand Down

0 comments on commit c025f58

Please sign in to comment.