From 70a29c2f120913dad38af9f3324e86a02331c5d3 Mon Sep 17 00:00:00 2001 From: narzoul Date: Sun, 15 May 2016 17:59:35 +0200 Subject: [PATCH] Fixed unsafe use of released primary surface interface --- DDrawCompat/CompatActivateAppHandler.cpp | 9 ++-- DDrawCompat/CompatDirectDrawSurface.cpp | 7 +-- DDrawCompat/CompatDirectDrawSurface.h | 39 ++++++++++++++++ DDrawCompat/CompatGdi.cpp | 8 ++-- DDrawCompat/CompatPrimarySurface.cpp | 48 ++++++++++---------- DDrawCompat/CompatPrimarySurface.h | 9 ++-- DDrawCompat/CompatPtr.h | 58 ++++++++++++++++++++++++ DDrawCompat/CompatQueryInterface.h | 28 ++++++++++++ DDrawCompat/CompatRef.h | 30 ++++++++++++ DDrawCompat/CompatVtable.h | 24 +++++++--- DDrawCompat/CompatWeakPtr.h | 54 ++++++++++++++++++++++ DDrawCompat/DDrawCompat.vcxproj | 4 ++ DDrawCompat/DDrawCompat.vcxproj.filters | 12 +++++ DDrawCompat/RealPrimarySurface.cpp | 12 +++-- 14 files changed, 291 insertions(+), 51 deletions(-) create mode 100644 DDrawCompat/CompatPtr.h create mode 100644 DDrawCompat/CompatQueryInterface.h create mode 100644 DDrawCompat/CompatRef.h create mode 100644 DDrawCompat/CompatWeakPtr.h diff --git a/DDrawCompat/CompatActivateAppHandler.cpp b/DDrawCompat/CompatActivateAppHandler.cpp index 898a863..0bf9377 100644 --- a/DDrawCompat/CompatActivateAppHandler.cpp +++ b/DDrawCompat/CompatActivateAppHandler.cpp @@ -3,6 +3,7 @@ #include "CompatDirectDrawSurface.h" #include "CompatGdi.h" #include "CompatPrimarySurface.h" +#include "CompatPtr.h" #include "DDrawLog.h" extern HWND g_mainWindow; @@ -38,12 +39,10 @@ namespace &dd, dm.width, dm.height, 32, dm.refreshRate, 0); } - if (CompatPrimarySurface::surface) + auto primary(CompatPrimarySurface::getPrimary()); + if (primary && SUCCEEDED(primary->Restore(primary))) { - CompatDirectDrawSurface::s_origVtable.Restore( - CompatPrimarySurface::surface); - CompatDirectDrawSurface::fixSurfacePtrs( - *CompatPrimarySurface::surface); + CompatDirectDrawSurface::fixSurfacePtrs(*primary); CompatGdi::invalidate(nullptr); } } diff --git a/DDrawCompat/CompatDirectDrawSurface.cpp b/DDrawCompat/CompatDirectDrawSurface.cpp index 3538eae..0061050 100644 --- a/DDrawCompat/CompatDirectDrawSurface.cpp +++ b/DDrawCompat/CompatDirectDrawSurface.cpp @@ -236,11 +236,8 @@ HRESULT CompatDirectDrawSurface::createCompatPrimarySurface( return result; } - IDirectDrawSurface7* compatSurface7 = nullptr; - s_origVtable.QueryInterface(compatSurface, IID_IDirectDrawSurface7, - reinterpret_cast(&compatSurface7)); - CompatPrimarySurface::setPrimary(compatSurface7); - CompatDirectDrawSurface::s_origVtable.Release(compatSurface7); + CompatPtr primary(Compat::queryInterface(compatSurface)); + CompatPrimarySurface::setPrimary(*primary); return DD_OK; } diff --git a/DDrawCompat/CompatDirectDrawSurface.h b/DDrawCompat/CompatDirectDrawSurface.h index 7a1e1dc..3e30372 100644 --- a/DDrawCompat/CompatDirectDrawSurface.h +++ b/DDrawCompat/CompatDirectDrawSurface.h @@ -1,5 +1,7 @@ #pragma once +#include + #include "CompatVtable.h" #include "DDrawTypes.h" #include "DirectDrawSurfaceVtblVisitor.h" @@ -66,3 +68,40 @@ public: private: static void restorePrimaryCaps(TDdsCaps& caps); }; + +namespace Compat +{ + template + struct IsDirectDrawSurfaceIntf : std::false_type {}; + + template<> struct IsDirectDrawSurfaceIntf : std::true_type {}; + template<> struct IsDirectDrawSurfaceIntf : std::true_type {}; + template<> struct IsDirectDrawSurfaceIntf : std::true_type {}; + template<> struct IsDirectDrawSurfaceIntf : std::true_type {}; + template<> struct IsDirectDrawSurfaceIntf : std::true_type {}; + + template + std::enable_if_t::value && IsDirectDrawSurfaceIntf::value> + queryInterface(OrigIntf& origIntf, NewIntf*& newIntf) + { + CompatDirectDrawSurface::s_origVtable.QueryInterface( + &origIntf, CompatDirectDrawSurface::s_iid, reinterpret_cast(&newIntf)); + } + + template + std::enable_if_t::value> + queryInterface(IUnknown& origIntf, NewIntf*& newIntf) + { + CompatDirectDrawSurface::s_origVtable.QueryInterface( + reinterpret_cast(&origIntf), + CompatDirectDrawSurface::s_iid, reinterpret_cast(&newIntf)); + } + + template + std::enable_if_t::value> + queryInterface(OrigIntf& origIntf, IUnknown*& newIntf) + { + CompatDirectDrawSurface::s_origVtable.QueryInterface( + &origIntf, IID_IUnknown, reinterpret_cast(&newIntf)); + } +} diff --git a/DDrawCompat/CompatGdi.cpp b/DDrawCompat/CompatGdi.cpp index 9f54ad9..fbd599e 100644 --- a/DDrawCompat/CompatGdi.cpp +++ b/DDrawCompat/CompatGdi.cpp @@ -59,8 +59,8 @@ namespace { DDSURFACEDESC2 desc = {}; desc.dwSize = sizeof(desc); - if (FAILED(CompatDirectDrawSurface::s_origVtable.Lock( - CompatPrimarySurface::surface, nullptr, &desc, DDLOCK_WAIT, nullptr))) + auto primary(CompatPrimarySurface::getPrimary()); + if (FAILED(primary->Lock(primary, nullptr, &desc, DDLOCK_WAIT, nullptr))) { return false; } @@ -74,8 +74,8 @@ namespace void unlockPrimarySurface() { GdiFlush(); - CompatDirectDrawSurface::s_origVtable.Unlock( - CompatPrimarySurface::surface, nullptr); + auto primary(CompatPrimarySurface::getPrimary()); + primary->Unlock(primary, nullptr); RealPrimarySurface::invalidate(nullptr); RealPrimarySurface::update(); diff --git a/DDrawCompat/CompatPrimarySurface.cpp b/DDrawCompat/CompatPrimarySurface.cpp index 652138e..582f600 100644 --- a/DDrawCompat/CompatPrimarySurface.cpp +++ b/DDrawCompat/CompatPrimarySurface.cpp @@ -4,28 +4,21 @@ #include "CompatDirectDraw.h" #include "CompatDirectDrawSurface.h" #include "CompatPrimarySurface.h" +#include "CompatPtr.h" #include "IReleaseNotifier.h" #include "RealPrimarySurface.h" namespace { + CompatWeakPtr g_primarySurface = nullptr; std::vector g_primarySurfacePtrs; - void addPrimary(IDirectDrawSurface7* surface, const IID& iid) - { - IUnknown* intf = nullptr; - CompatDirectDrawSurface::s_origVtable.QueryInterface( - surface, iid, reinterpret_cast(&intf)); - g_primarySurfacePtrs.push_back(intf); - intf->lpVtbl->Release(intf); - } - void onRelease() { Compat::LogEnter("CompatPrimarySurface::onRelease"); g_primarySurfacePtrs.clear(); - CompatPrimarySurface::surface = nullptr; + g_primarySurface = nullptr; CompatPrimarySurface::palette = nullptr; CompatPrimarySurface::width = 0; CompatPrimarySurface::height = 0; @@ -59,32 +52,41 @@ namespace CompatPrimarySurface template DisplayMode getDisplayMode(IDirectDraw4& dd); template DisplayMode getDisplayMode(IDirectDraw7& dd); - bool isPrimary(void* surfacePtr) + CompatPtr getPrimary() { - return g_primarySurfacePtrs.end() != - std::find(g_primarySurfacePtrs.begin(), g_primarySurfacePtrs.end(), surfacePtr); + if (!g_primarySurface) + { + return nullptr; + } + return CompatPtr( + Compat::queryInterface(g_primarySurface.get())); } - void setPrimary(IDirectDrawSurface7* surfacePtr) + bool isPrimary(void* surface) { - surface = surfacePtr; + return g_primarySurfacePtrs.end() != + std::find(g_primarySurfacePtrs.begin(), g_primarySurfacePtrs.end(), surface); + } + + void setPrimary(CompatRef surface) + { + CompatPtr surfacePtr(Compat::queryInterface(&surface)); + g_primarySurface = surfacePtr; g_primarySurfacePtrs.clear(); + g_primarySurfacePtrs.push_back(&surface); + g_primarySurfacePtrs.push_back(CompatPtr(surfacePtr)); + g_primarySurfacePtrs.push_back(CompatPtr(surfacePtr)); + g_primarySurfacePtrs.push_back(CompatPtr(surfacePtr)); g_primarySurfacePtrs.push_back(surfacePtr); - addPrimary(surfacePtr, IID_IDirectDrawSurface4); - addPrimary(surfacePtr, IID_IDirectDrawSurface3); - addPrimary(surfacePtr, IID_IDirectDrawSurface2); - addPrimary(surfacePtr, IID_IDirectDrawSurface); IReleaseNotifier* releaseNotifierPtr = &releaseNotifier; - CompatDirectDrawSurface::s_origVtable.SetPrivateData( - surfacePtr, IID_IReleaseNotifier, releaseNotifierPtr, sizeof(releaseNotifierPtr), - DDSPD_IUNKNOWNPOINTER); + surface->SetPrivateData(&surface, IID_IReleaseNotifier, + releaseNotifierPtr, sizeof(releaseNotifierPtr), DDSPD_IUNKNOWNPOINTER); } DisplayMode displayMode = {}; bool isDisplayModeChanged = false; - IDirectDrawSurface7* surface = nullptr; LPDIRECTDRAWPALETTE palette = nullptr; PALETTEENTRY paletteEntries[256] = {}; LONG width = 0; diff --git a/DDrawCompat/CompatPrimarySurface.h b/DDrawCompat/CompatPrimarySurface.h index 001ba60..4d14314 100644 --- a/DDrawCompat/CompatPrimarySurface.h +++ b/DDrawCompat/CompatPrimarySurface.h @@ -4,6 +4,9 @@ #include +#include "CompatPtr.h" +#include "CompatRef.h" + class IReleaseNotifier; namespace CompatPrimarySurface @@ -19,12 +22,12 @@ namespace CompatPrimarySurface template DisplayMode getDisplayMode(TDirectDraw& dd); - bool isPrimary(void* surfacePtr); - void setPrimary(IDirectDrawSurface7* surfacePtr); + CompatPtr getPrimary(); + bool isPrimary(void* surface); + void setPrimary(CompatRef surface); extern DisplayMode displayMode; extern bool isDisplayModeChanged; - extern IDirectDrawSurface7* surface; extern LPDIRECTDRAWPALETTE palette; extern PALETTEENTRY paletteEntries[256]; extern LONG width; diff --git a/DDrawCompat/CompatPtr.h b/DDrawCompat/CompatPtr.h new file mode 100644 index 0000000..46a29af --- /dev/null +++ b/DDrawCompat/CompatPtr.h @@ -0,0 +1,58 @@ +#pragma once + +#include + +#include "CompatQueryInterface.h" +#include "CompatWeakPtr.h" + +template +class CompatPtr : public CompatWeakPtr +{ +public: + CompatPtr(std::nullptr_t = nullptr) + { + } + + explicit CompatPtr(Intf* intf) : CompatWeakPtr(intf) + { + } + + CompatPtr(const CompatPtr& other) + { + m_intf = Compat::queryInterface(other.get()); + } + + template + CompatPtr(const CompatPtr& other) + { + m_intf = Compat::queryInterface(other.get()); + } + + ~CompatPtr() + { + release(); + } + + CompatPtr& operator=(CompatPtr rhs) + { + swap(rhs); + return *this; + } + + Intf* detach() + { + Intf* intf = m_intf; + m_intf = nullptr; + return intf; + } + + void reset(Intf* intf = nullptr) + { + *this = CompatPtr(intf); + } + + void swap(CompatPtr& other) + { + std::swap(m_intf, other.m_intf); + } +}; diff --git a/DDrawCompat/CompatQueryInterface.h b/DDrawCompat/CompatQueryInterface.h new file mode 100644 index 0000000..d387e14 --- /dev/null +++ b/DDrawCompat/CompatQueryInterface.h @@ -0,0 +1,28 @@ +#pragma once + +struct IUnknown; + +namespace Compat +{ + template + void queryInterface(Intf& origIntf, Intf*& newIntf) + { + newIntf = &origIntf; + newIntf->lpVtbl->AddRef(newIntf); + } + + void queryInterface(IUnknown&, IUnknown*&) = delete; + + template + NewIntf* queryInterface(OrigIntf* origIntf) + { + if (!origIntf) + { + return nullptr; + } + + NewIntf* newIntf = nullptr; + queryInterface(*origIntf, newIntf); + return newIntf; + } +} diff --git a/DDrawCompat/CompatRef.h b/DDrawCompat/CompatRef.h new file mode 100644 index 0000000..f79eb6a --- /dev/null +++ b/DDrawCompat/CompatRef.h @@ -0,0 +1,30 @@ +#pragma once + +#include "CompatVtable.h" + +template +class CompatRef +{ +public: + CompatRef(Intf& intf) : m_intf(intf) + { + } + + const Vtable* operator->() const + { + return &CompatVtableBase::getOrigVtable(m_intf); + } + + Intf* operator&() const + { + return &m_intf; + } + + Intf& get() const + { + return m_intf; + } + +private: + Intf& m_intf; +}; diff --git a/DDrawCompat/CompatVtable.h b/DDrawCompat/CompatVtable.h index 9489eac..4c4e740 100644 --- a/DDrawCompat/CompatVtable.h +++ b/DDrawCompat/CompatVtable.h @@ -11,12 +11,24 @@ template using Vtable = typename std::remove_pointer::type; -template -class CompatVtable +template +class CompatVtableBase { public: typedef Interface Interface; + static const Vtable& getOrigVtable(Interface& intf) + { + return s_origVtable.AddRef ? s_origVtable : *intf.lpVtbl; + } + + static Vtable s_origVtable; +}; + +template +class CompatVtable : public CompatVtableBase +{ +public: static void hookVtable(Interface& intf) { static bool isInitialized = false; @@ -32,8 +44,6 @@ public: } } - static Vtable s_origVtable; - private: class InitVisitor { @@ -122,11 +132,11 @@ private: static std::map, std::string> s_funcNames; }; -template -Vtable* CompatVtable::s_vtablePtr = nullptr; +template +Vtable CompatVtableBase::s_origVtable = {}; template -Vtable CompatVtable::s_origVtable = {}; +Vtable* CompatVtable::s_vtablePtr = nullptr; template Vtable CompatVtable::s_compatVtable(CompatInterface::getCompatVtable()); diff --git a/DDrawCompat/CompatWeakPtr.h b/DDrawCompat/CompatWeakPtr.h new file mode 100644 index 0000000..6c67a0f --- /dev/null +++ b/DDrawCompat/CompatWeakPtr.h @@ -0,0 +1,54 @@ +#pragma once + +#include "CompatVtable.h" + +template +class CompatWeakPtr +{ +public: + CompatWeakPtr(Intf* intf = nullptr) : m_intf(intf) + { + } + + Intf& operator*() const + { + return *m_intf; + } + + const Vtable* operator->() const + { + return &CompatVtableBase::getOrigVtable(*m_intf); + } + + operator Intf*() const + { + return m_intf; + } + + Intf* get() const + { + return m_intf; + } + + Intf* const& getRef() const + { + return m_intf; + } + + Intf*& getRef() + { + return m_intf; + } + + void release() + { + if (m_intf) + { + m_intf->lpVtbl->Release(m_intf); + m_intf = nullptr; + } + } + +protected: + Intf* m_intf; +}; diff --git a/DDrawCompat/DDrawCompat.vcxproj b/DDrawCompat/DDrawCompat.vcxproj index 92d0739..16b4498 100644 --- a/DDrawCompat/DDrawCompat.vcxproj +++ b/DDrawCompat/DDrawCompat.vcxproj @@ -157,7 +157,11 @@ + + + + diff --git a/DDrawCompat/DDrawCompat.vcxproj.filters b/DDrawCompat/DDrawCompat.vcxproj.filters index 1578c88..9b2adf7 100644 --- a/DDrawCompat/DDrawCompat.vcxproj.filters +++ b/DDrawCompat/DDrawCompat.vcxproj.filters @@ -114,6 +114,18 @@ Header Files + + Header Files + + + Header Files + + + Header Files + + + Header Files + diff --git a/DDrawCompat/RealPrimarySurface.cpp b/DDrawCompat/RealPrimarySurface.cpp index 9cb8a28..66d0a97 100644 --- a/DDrawCompat/RealPrimarySurface.cpp +++ b/DDrawCompat/RealPrimarySurface.cpp @@ -56,10 +56,11 @@ namespace clipper->lpVtbl->Release(clipper); } + auto primary(CompatPrimarySurface::getPrimary()); if (CompatPrimarySurface::pixelFormat.dwRGBBitCount <= 8) { origVtable.Blt(CompatPaletteConverter::getSurface(), &g_updateRect, - CompatPrimarySurface::surface, &g_updateRect, DDBLT_WAIT, nullptr); + primary, &g_updateRect, DDBLT_WAIT, nullptr); HDC destDc = nullptr; origVtable.GetDC(dest, &destDc); @@ -79,7 +80,7 @@ namespace else { result = SUCCEEDED(origVtable.Blt(dest, &g_updateRect, - CompatPrimarySurface::surface, &g_updateRect, DDBLT_WAIT, nullptr)); + primary, &g_updateRect, DDBLT_WAIT, nullptr)); } if (result) @@ -394,6 +395,9 @@ void RealPrimarySurface::updatePalette(DWORD startingEntry, DWORD count) { CompatPaletteConverter::updatePalette(startingEntry, count); CompatGdi::updatePalette(startingEntry, count); - invalidate(nullptr); - update(); + if (CompatPrimarySurface::palette) + { + invalidate(nullptr); + update(); + } }