Stefan Dösinger : ddraw: Protect against invalid clipper pointers.

[Alexandre Julliard]

Module: wine
Branch: master
Commit: f8361d27e4912193ed8ecdf302d3df250e9480e5
URL:    https://source.winehq.org/git/wine.git/?a=commit;h=f8361d27e4912193ed8ecdf30...

Author: Stefan Dösinger 
Date:   Wed Mar 13 13:18:31 2019 +0100

ddraw: Protect against invalid clipper pointers.

Signed-off-by: Stefan Dösinger 
Signed-off-by: Henri Verbeet 
Signed-off-by: Alexandre Julliard

---

dlls/ddraw/clipper.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 64 insertions(+), 3 deletions(-)

diff --git a/dlls/ddraw/clipper.c b/dlls/ddraw/clipper.c
index 01cac40..e4b7ec2 100644
--- a/dlls/ddraw/clipper.c
+++ b/dlls/ddraw/clipper.c
@@ -26,17 +26,43 @@

WINE_DEFAULT_DEBUG_CHANNEL(ddraw);

+static const struct IDirectDrawClipperVtbl ddraw_clipper_vtbl;
+
 static inline struct ddraw_clipper *impl_from_IDirectDrawClipper(IDirectDrawClipper *iface)
 {
     return CONTAINING_RECORD(iface, struct ddraw_clipper, IDirectDrawClipper_iface);
 }

+static BOOL ddraw_clipper_is_valid(const struct ddraw_clipper *clipper)
+{
+    /* Native is very lenient when you invoke the clipper methods with a clipper pointer that
+     * points to something that is either not accessible or not a clipper, or if you break
+     * a clipper after assigning it to a surface. Deus Ex: Goty depends on this. */
+
+    if (IsBadReadPtr(clipper, sizeof(*clipper)))
+    {
+        WARN("The application gave us an invalid clipper pointer %p.\n", clipper);
+        return FALSE;
+    }
+    if (clipper->IDirectDrawClipper_iface.lpVtbl != &ddraw_clipper_vtbl)
+    {
+        WARN("The clipper vtable is modified: %p, expected %p.\n",
+                clipper->IDirectDrawClipper_iface.lpVtbl, &ddraw_clipper_vtbl);
+        return FALSE;
+    }
+
+    return TRUE;
+}
+
 static HRESULT WINAPI ddraw_clipper_QueryInterface(IDirectDrawClipper *iface, REFIID iid, void **object)
 {
     struct ddraw_clipper *clipper = impl_from_IDirectDrawClipper(iface);

TRACE("iface %p, iid %s, object %p.\n", iface, debugstr_guid(iid), object);

+    if (!ddraw_clipper_is_valid(clipper))
+        return DDERR_INVALIDPARAMS;
+
     if (IsEqualGUID(&IID_IDirectDrawClipper, iid)
             || IsEqualGUID(&IID_IUnknown, iid))
     {
@@ -54,17 +80,31 @@ static HRESULT WINAPI ddraw_clipper_QueryInterface(IDirectDrawClipper *iface, RE
 static ULONG WINAPI ddraw_clipper_AddRef(IDirectDrawClipper *iface)
 {
     struct ddraw_clipper *clipper = impl_from_IDirectDrawClipper(iface);
-    ULONG refcount = InterlockedIncrement(&clipper->ref);
+    ULONG refcount;

-    TRACE("%p increasing refcount to %u.\n", clipper, refcount);
+    if (!ddraw_clipper_is_valid(clipper))
+    {
+        WARN("Invalid clipper, returning 0.\n");
+        return 0;
+    }

+    refcount = InterlockedIncrement(&clipper->ref);
+    TRACE("%p increasing refcount to %u.\n", clipper, refcount);
     return refcount;
 }

static ULONG WINAPI ddraw_clipper_Release(IDirectDrawClipper *iface)
 {
     struct ddraw_clipper *clipper = impl_from_IDirectDrawClipper(iface);
-    ULONG refcount = InterlockedDecrement(&clipper->ref);
+    ULONG refcount;
+
+    if (!ddraw_clipper_is_valid(clipper))
+    {
+        WARN("Invalid clipper, returning 0.\n");
+        return 0;
+    }
+
+    refcount = InterlockedDecrement(&clipper->ref);

TRACE("%p decreasing refcount to %u.\n", clipper, refcount);

@@ -72,6 +112,7 @@ static ULONG WINAPI ddraw_clipper_Release(IDirectDrawClipper *iface)
     {
         if (clipper->region)
             DeleteObject(clipper->region);
+        clipper->IDirectDrawClipper_iface.lpVtbl = NULL; /* Should help with detecting freed clippers. */
         heap_free(clipper);
     }

@@ -84,6 +125,9 @@ static HRESULT WINAPI ddraw_clipper_SetHWnd(IDirectDrawClipper *iface, DWORD fla

TRACE("iface %p, flags %#x, window %p.\n", iface, flags, window);

+    if (!ddraw_clipper_is_valid(clipper))
+        return DDERR_INVALIDPARAMS;
+
     if (flags)
     {
         FIXME("flags %#x, not supported.\n", flags);
@@ -161,6 +205,9 @@ static HRESULT WINAPI ddraw_clipper_GetClipList(IDirectDrawClipper *iface, RECT
     TRACE("iface %p, rect %s, clip_list %p, clip_list_size %p.\n",
             iface, wine_dbgstr_rect(rect), clip_list, clip_list_size);

+    if (!ddraw_clipper_is_valid(clipper))
+        return DDERR_INVALIDPARAMS;
+
     wined3d_mutex_lock();

if (clipper->window)
@@ -238,6 +285,9 @@ static HRESULT WINAPI ddraw_clipper_SetClipList(IDirectDrawClipper *iface, RGNDA

TRACE("iface %p, region %p, flags %#x.\n", iface, region, flags);

+    if (!ddraw_clipper_is_valid(clipper))
+        return DDERR_INVALIDPARAMS;
+
     wined3d_mutex_lock();

if (clipper->window)
@@ -268,6 +318,9 @@ static HRESULT WINAPI ddraw_clipper_GetHWnd(IDirectDrawClipper *iface, HWND *win

TRACE("iface %p, window %p.\n", iface, window);

+    if (!ddraw_clipper_is_valid(clipper))
+        return DDERR_INVALIDPARAMS;
+
     wined3d_mutex_lock();
     *window = clipper->window;
     wined3d_mutex_unlock();
@@ -282,6 +335,9 @@ static HRESULT WINAPI ddraw_clipper_Initialize(IDirectDrawClipper *iface,

TRACE("iface %p, ddraw %p, flags %#x.\n", iface, ddraw, flags);

+    if (!ddraw_clipper_is_valid(clipper))
+        return DDERR_INVALIDPARAMS;
+
     wined3d_mutex_lock();
     if (clipper->initialized)
     {
@@ -297,8 +353,13 @@ static HRESULT WINAPI ddraw_clipper_Initialize(IDirectDrawClipper *iface,

static HRESULT WINAPI ddraw_clipper_IsClipListChanged(IDirectDrawClipper *iface, BOOL *changed)
 {
+    struct ddraw_clipper *clipper = impl_from_IDirectDrawClipper(iface);
+
     FIXME("iface %p, changed %p stub!\n", iface, changed);

+    if (!ddraw_clipper_is_valid(clipper))
+        return DDERR_INVALIDPARAMS;
+
     /* XXX What is safest? */
     *changed = FALSE;