Skip to content

Commit

Permalink
Fix ASAN memory leak in FileChecker (#5718)
Browse files Browse the repository at this point in the history
AllocVFSIncludeHandler was returning it's IUnknown object by Detach()ing
from a CComPtr, then assiging to CComPtr. This would result in the
object have a ref count of 2, and when the second CComPtr destructed, it
would go down to 1, and leak memory.

The fix is to return the object as a CComPtr; however, with Clang, this
failed to compile because it finds no conversion from a CComPtr of child
class type to a CComPtr of base class type. This compiles fine on MSVC,
so I debugged it, and realized that MSVC compiles this as first a
conversion to T*, then a constructor call. I suspect MSVC is wrong here.
In any case, this change adds a template conversion constructor to
CComPtr, very much like the existing template assignment operator.
  • Loading branch information
amaiorano authored Sep 18, 2023
1 parent 08215da commit 906fc46
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 2 deletions.
7 changes: 7 additions & 0 deletions include/dxc/WinAdapter.h
Original file line number Diff line number Diff line change
Expand Up @@ -748,6 +748,13 @@ template <class T> class CComPtr : public CComPtrBase<T> {
return *this;
}

// NOTE: This conversion constructor is not part of the official CComPtr spec;
// however, it is needed to convert CComPtr<Q> to CComPtr<T> where T derives from Q on Clang.
// MSVC compiles this conversion as first a call to CComPtr<Q>::operator T*, followed by CComPtr<T>(T*),
// but Clang fails to compile with error: no viable conversion from 'CComPtr<Q>' to 'CComPtr<T>'.
template <typename Q>
CComPtr(const CComPtr<Q> &lp) throw() : CComPtrBase<T>(lp.p) {}

T *operator=(const CComPtr<T> &lp) throw() {
if (*this != lp) {
CComPtr(lp).Swap(*this);
Expand Down
4 changes: 2 additions & 2 deletions tools/clang/unittests/HLSLTestLib/FileCheckerTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,7 @@ class IncludeHandlerVFSOverlayForTest : public IDxcIncludeHandler {
}
};

static IncludeHandlerVFSOverlayForTest *AllocVFSIncludeHandler(IUnknown *pUnkLibrary, const FileMap *pVFS) {
static CComPtr<IncludeHandlerVFSOverlayForTest> AllocVFSIncludeHandler(IUnknown *pUnkLibrary, const FileMap *pVFS) {
CComPtr<IncludeHandlerVFSOverlayForTest> pVFSIncludeHandler = IncludeHandlerVFSOverlayForTest::Alloc(DxcGetThreadMallocNoRef());
IFTBOOL(pVFSIncludeHandler, E_OUTOFMEMORY);
if (pUnkLibrary) {
Expand All @@ -273,7 +273,7 @@ static IncludeHandlerVFSOverlayForTest *AllocVFSIncludeHandler(IUnknown *pUnkLib
pVFSIncludeHandler->pInnerIncludeHandler = pInnerIncludeHandler;
}
pVFSIncludeHandler->pVFS = pVFS;
return pVFSIncludeHandler.Detach();
return pVFSIncludeHandler;
}

static void AddOutputsToFileMap(IUnknown *pUnkResult, FileMap *pVFS) {
Expand Down

0 comments on commit 906fc46

Please sign in to comment.