[PATCH 1/4] ddraw: Protect against invalid clipper pointers (v2).
Signed-off-by: Stefan Dösinger <stefan(a)codeweavers.com> --- v2: Drop the magic and move bad clipper detection into a separate function instead of impl_from_IDirectDrawClipper. I also dropped patch 4 (see 159887) from this resubmission because with moving the bad clipper detection to a separate function there is still a difference between impl_from_... and unsafe_impl_from_... . --- 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 01cac40ec6e..e4b7ec2bd50 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; -- 2.19.2
Signed-off-by: Stefan Dösinger <stefan(a)codeweavers.com> --- dlls/ddraw/surface.c | 1 + 1 file changed, 1 insertion(+) diff --git a/dlls/ddraw/surface.c b/dlls/ddraw/surface.c index 2b788bd8d13..0063a1eddf8 100644 --- a/dlls/ddraw/surface.c +++ b/dlls/ddraw/surface.c @@ -4436,6 +4436,7 @@ static HRESULT WINAPI ddraw_surface7_GetClipper(IDirectDrawSurface7 *iface, IDir if (!surface->clipper) { wined3d_mutex_unlock(); + *Clipper = NULL; return DDERR_NOCLIPPERATTACHED; } -- 2.19.2
Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=49253 Your paranoid android. === debian9 (build log) === X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) === debian9 (build log) === X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig)
This fixes a memory leak on alt-tab in Deus Ex: GOTY. It steals a clipper reference from the primary surface, destroying the clipper. The primary will try to release its reference, causing a segfault that is handled fairly gracefully by the game. The game will seemingly continue fine, but the ddraw object is not destroyed, leaving the wined3d swapchain and its textures behind. Signed-off-by: Stefan Dösinger <stefan(a)codeweavers.com> --- dlls/ddraw/clipper.c | 18 ++++++++++++------ dlls/ddraw/ddraw_private.h | 9 +++++++++ dlls/ddraw/surface.c | 17 ++++++++--------- 3 files changed, 29 insertions(+), 15 deletions(-) diff --git a/dlls/ddraw/clipper.c b/dlls/ddraw/clipper.c index e4b7ec2bd50..7fbfd6561b1 100644 --- a/dlls/ddraw/clipper.c +++ b/dlls/ddraw/clipper.c @@ -77,7 +77,7 @@ static HRESULT WINAPI ddraw_clipper_QueryInterface(IDirectDrawClipper *iface, RE return E_NOINTERFACE; } -static ULONG WINAPI ddraw_clipper_AddRef(IDirectDrawClipper *iface) +ULONG WINAPI ddraw_clipper_AddRef(IDirectDrawClipper *iface) { struct ddraw_clipper *clipper = impl_from_IDirectDrawClipper(iface); ULONG refcount; @@ -93,7 +93,7 @@ static ULONG WINAPI ddraw_clipper_AddRef(IDirectDrawClipper *iface) return refcount; } -static ULONG WINAPI ddraw_clipper_Release(IDirectDrawClipper *iface) +ULONG WINAPI ddraw_clipper_Release(IDirectDrawClipper *iface) { struct ddraw_clipper *clipper = impl_from_IDirectDrawClipper(iface); ULONG refcount; @@ -196,7 +196,7 @@ static HRGN get_window_region(HWND window) * RETURNS * Either DD_OK or DDERR_* ************************************************************************/ -static HRESULT WINAPI ddraw_clipper_GetClipList(IDirectDrawClipper *iface, RECT *rect, +HRESULT WINAPI ddraw_clipper_GetClipList(IDirectDrawClipper *iface, RECT *rect, RGNDATA *clip_list, DWORD *clip_list_size) { struct ddraw_clipper *clipper = impl_from_IDirectDrawClipper(iface); @@ -312,7 +312,7 @@ static HRESULT WINAPI ddraw_clipper_SetClipList(IDirectDrawClipper *iface, RGNDA return DD_OK; } -static HRESULT WINAPI ddraw_clipper_GetHWnd(IDirectDrawClipper *iface, HWND *window) +HRESULT WINAPI ddraw_clipper_GetHWnd(IDirectDrawClipper *iface, HWND *window) { struct ddraw_clipper *clipper = impl_from_IDirectDrawClipper(iface); @@ -389,9 +389,15 @@ HRESULT ddraw_clipper_init(struct ddraw_clipper *clipper) struct ddraw_clipper *unsafe_impl_from_IDirectDrawClipper(IDirectDrawClipper *iface) { + struct ddraw_clipper *clipper; + if (!iface) return NULL; - assert(iface->lpVtbl == &ddraw_clipper_vtbl); - return impl_from_IDirectDrawClipper(iface); + clipper = impl_from_IDirectDrawClipper(iface); + + if (!clipper || iface->lpVtbl != &ddraw_clipper_vtbl) + WARN("The application passed us a bad clipper object.\n"); + + return clipper; } diff --git a/dlls/ddraw/ddraw_private.h b/dlls/ddraw/ddraw_private.h index 77e28662724..988d54b7a89 100644 --- a/dlls/ddraw/ddraw_private.h +++ b/dlls/ddraw/ddraw_private.h @@ -392,6 +392,15 @@ struct ddraw_clipper HRESULT ddraw_clipper_init(struct ddraw_clipper *clipper) DECLSPEC_HIDDEN; struct ddraw_clipper *unsafe_impl_from_IDirectDrawClipper(IDirectDrawClipper *iface) DECLSPEC_HIDDEN; +/* Invoke clipper methods directly instead of going through the vtable. Clipper methods have + * protections against invalid clipper objects, but that won't work if we crash when reading + * the vtable. */ +ULONG WINAPI ddraw_clipper_AddRef(IDirectDrawClipper *iface); +ULONG WINAPI ddraw_clipper_Release(IDirectDrawClipper *iface); +HRESULT WINAPI ddraw_clipper_GetClipList(IDirectDrawClipper *iface, RECT *rect, + RGNDATA *clip_list, DWORD *clip_list_size); +HRESULT WINAPI ddraw_clipper_GetHWnd(IDirectDrawClipper *iface, HWND *window); + /***************************************************************************** * IDirectDrawPalette implementation structure *****************************************************************************/ diff --git a/dlls/ddraw/surface.c b/dlls/ddraw/surface.c index 0063a1eddf8..2a5404f03fd 100644 --- a/dlls/ddraw/surface.c +++ b/dlls/ddraw/surface.c @@ -1552,7 +1552,7 @@ static HRESULT ddraw_surface_blt_clipped(struct ddraw_surface *dst_surface, cons scale_x = (float)(src_rect.right - src_rect.left) / (float)(dst_rect.right - dst_rect.left); scale_y = (float)(src_rect.bottom - src_rect.top) / (float)(dst_rect.bottom - dst_rect.top); - if (FAILED(hr = IDirectDrawClipper_GetClipList(&dst_surface->clipper->IDirectDrawClipper_iface, + if (FAILED(hr = ddraw_clipper_GetClipList(&dst_surface->clipper->IDirectDrawClipper_iface, &dst_rect, NULL, &clip_list_size))) { WARN("Failed to get clip list size, hr %#x.\n", hr); @@ -1565,7 +1565,7 @@ static HRESULT ddraw_surface_blt_clipped(struct ddraw_surface *dst_surface, cons return E_OUTOFMEMORY; } - if (FAILED(hr = IDirectDrawClipper_GetClipList(&dst_surface->clipper->IDirectDrawClipper_iface, + if (FAILED(hr = ddraw_clipper_GetClipList(&dst_surface->clipper->IDirectDrawClipper_iface, &dst_rect, clip_list, &clip_list_size))) { WARN("Failed to get clip list, hr %#x.\n", hr); @@ -4441,7 +4441,7 @@ static HRESULT WINAPI ddraw_surface7_GetClipper(IDirectDrawSurface7 *iface, IDir } *Clipper = &surface->clipper->IDirectDrawClipper_iface; - IDirectDrawClipper_AddRef(*Clipper); + ddraw_clipper_AddRef(*Clipper); wined3d_mutex_unlock(); return DD_OK; @@ -4515,16 +4515,15 @@ static HRESULT WINAPI ddraw_surface7_SetClipper(IDirectDrawSurface7 *iface, This->clipper = clipper; if (clipper != NULL) - IDirectDrawClipper_AddRef(iclipper); + ddraw_clipper_AddRef(iclipper); if (old_clipper) - IDirectDrawClipper_Release(&old_clipper->IDirectDrawClipper_iface); + ddraw_clipper_Release(&old_clipper->IDirectDrawClipper_iface); if ((This->surface_desc.ddsCaps.dwCaps & DDSCAPS_PRIMARYSURFACE) && This->ddraw->wined3d_swapchain) { clipWindow = NULL; - if(clipper) { - IDirectDrawClipper_GetHWnd(iclipper, &clipWindow); - } + if (clipper) + ddraw_clipper_GetHWnd(iclipper, &clipWindow); if (clipWindow) { @@ -5798,7 +5797,7 @@ static void STDMETHODCALLTYPE ddraw_surface_wined3d_object_destroyed(void *paren list_remove(&surface->surface_list_entry); if (surface->clipper) - IDirectDrawClipper_Release(&surface->clipper->IDirectDrawClipper_iface); + ddraw_clipper_Release(&surface->clipper->IDirectDrawClipper_iface); if (surface == surface->ddraw->primary) { -- 2.19.2
Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=49254 Your paranoid android. === debian9 (build log) === X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) === debian9 (build log) === X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig)
On Wed, 13 Mar 2019 at 15:48, Stefan Dösinger <stefan(a)codeweavers.com> wrote:
struct ddraw_clipper *unsafe_impl_from_IDirectDrawClipper(IDirectDrawClipper *iface) { + struct ddraw_clipper *clipper; + if (!iface) return NULL; - assert(iface->lpVtbl == &ddraw_clipper_vtbl);
- return impl_from_IDirectDrawClipper(iface); + clipper = impl_from_IDirectDrawClipper(iface); + + if (!clipper || iface->lpVtbl != &ddraw_clipper_vtbl) + WARN("The application passed us a bad clipper object.\n"); + + return clipper; } "if (!ddraw_clipper_is_valid(clipper)) return NULL;", right?
+/* Invoke clipper methods directly instead of going through the vtable. Clipper methods have + * protections against invalid clipper objects, but that won't work if we crash when reading + * the vtable. */ Sure, but that protection involves pretty much just calling ddraw_clipper_is_valid(). Just calling that function in the appropriate place in e.g. ddraw_surface_blt_clipped() seems both nicer and more robust.
Signed-off-by: Stefan Dösinger <stefan(a)codeweavers.com> --- Version 2: Adjust for different Wine behavior due to using the vtable to identify broken clippers. Add a comment that surface->SetClipper with an unreadable pointer crashes. I don't know why I previously had the idea that this works. --- dlls/ddraw/tests/ddraw1.c | 151 ++++++++++++++++++++++++++++++++++++ dlls/ddraw/tests/ddraw2.c | 158 ++++++++++++++++++++++++++++++++++++++ dlls/ddraw/tests/ddraw4.c | 157 +++++++++++++++++++++++++++++++++++++ dlls/ddraw/tests/ddraw7.c | 157 +++++++++++++++++++++++++++++++++++++ 4 files changed, 623 insertions(+) diff --git a/dlls/ddraw/tests/ddraw1.c b/dlls/ddraw/tests/ddraw1.c index d0ed33dbc41..cf53cc1f945 100644 --- a/dlls/ddraw/tests/ddraw1.c +++ b/dlls/ddraw/tests/ddraw1.c @@ -12293,6 +12293,156 @@ static void test_alphatest(void) DestroyWindow(window); } +static void test_clipper_refcount(void) +{ + IDirectDrawSurface *surface; + IDirectDrawClipper *clipper, *clipper2; + DDSURFACEDESC surface_desc; + IDirectDraw *ddraw; + ULONG refcount; + HWND window; + HRESULT hr; + BOOL changed; + const IDirectDrawClipperVtbl *orig_vtbl; + + window = create_window(); + ddraw = create_ddraw(); + ok(!!ddraw, "Failed to create a ddraw object.\n"); + hr = IDirectDraw_SetCooperativeLevel(ddraw, window, DDSCL_NORMAL); + ok(hr == DD_OK, "Got unexpected hr %#x.\n", hr); + + memset(&surface_desc, 0, sizeof(surface_desc)); + surface_desc.dwSize = sizeof(surface_desc); + surface_desc.dwFlags = DDSD_CAPS; + surface_desc.ddsCaps.dwCaps = DDSCAPS_PRIMARYSURFACE; + hr = IDirectDraw_CreateSurface(ddraw, &surface_desc, &surface, NULL); + ok(hr == DD_OK, "Got unexpected hr %#x.\n", hr); + + hr = IDirectDraw_CreateClipper(ddraw, 0, &clipper, NULL); + ok(SUCCEEDED(hr), "Failed to create clipper, hr %#x.\n", hr); + refcount = get_refcount((IUnknown *)clipper); + ok(refcount == 1, "Got unexpected refcount %u.\n", refcount); + + /* Show that clipper validation doesn't somehow happen through per-clipper vtable + * pointers. */ + hr = IDirectDraw_CreateClipper(ddraw, 0, &clipper2, NULL); + ok(SUCCEEDED(hr), "Failed to create clipper, hr %#x.\n", hr); + ok(clipper->lpVtbl == clipper2->lpVtbl, "Got different clipper vtables %p and %p.\n", + clipper->lpVtbl, clipper2->lpVtbl); + orig_vtbl = clipper->lpVtbl; + IDirectDrawClipper_Release(clipper2); + + /* Surfaces hold a reference to clippers. No surprises there. */ + hr = IDirectDrawSurface_SetClipper(surface, clipper); + ok(SUCCEEDED(hr), "Failed to set clipper, hr %#x.\n", hr); + refcount = get_refcount((IUnknown *)clipper); + ok(refcount == 2, "Got unexpected refcount %u.\n", refcount); + + hr = IDirectDrawSurface_GetClipper(surface, &clipper2); + ok(SUCCEEDED(hr), "Failed to get clipper, hr %#x.\n", hr); + ok(clipper == clipper2, "Got clipper %p, expected %p.\n", clipper2, clipper); + refcount = IDirectDrawClipper_Release(clipper2); + ok(refcount == 2, "Got unexpected refcount %u.\n", refcount); + + hr = IDirectDrawSurface_SetClipper(surface, NULL); + ok(SUCCEEDED(hr), "Failed to set clipper, hr %#x.\n", hr); + refcount = get_refcount((IUnknown *)clipper); + ok(refcount == 1, "Got unexpected refcount %u.\n", refcount); + + hr = IDirectDrawSurface_SetClipper(surface, clipper); + ok(SUCCEEDED(hr), "Failed to set clipper, hr %#x.\n", hr); + refcount = get_refcount((IUnknown *)clipper); + ok(refcount == 2, "Got unexpected refcount %u.\n", refcount); + + refcount = IDirectDrawSurface_Release(surface); + ok(!refcount, "%u references left.\n", refcount); + refcount = get_refcount((IUnknown *)clipper); + ok(refcount == 1, "Got unexpected refcount %u.\n", refcount); + + /* SetClipper with an invalid pointer crashes. */ + + /* Clipper methods work with a broken vtable, with the exception of Release. */ + clipper->lpVtbl = (void *)0xdeadbeef; + refcount = orig_vtbl->AddRef(clipper); + todo_wine ok(refcount == 2, "Got unexpected refcount %u.\n", refcount); + refcount = orig_vtbl->Release(clipper); + ok(!refcount, "Got unexpected refcount %u.\n", refcount); + + clipper->lpVtbl = orig_vtbl; + refcount = orig_vtbl->Release(clipper); + todo_wine ok(refcount == 1, "Got unexpected refcount %u.\n", refcount); + + /* Fix the refcount difference because Wine did not increase the ref in the + * AddRef call above. */ + if (refcount) + { + refcount = IDirectDrawClipper_Release(clipper); + ok(!refcount, "Got unexpected refcount %u.\n", refcount); + } + + /* Steal the reference and see what happens - releasing the surface works fine. + * The clipper is destroyed and not kept alive by a hidden refcount - trying to + * release it after the GetClipper call is likely to crash, and certain to crash + * if we allocate and zero as much heap memory as we can get. */ + hr = IDirectDraw_CreateSurface(ddraw, &surface_desc, &surface, NULL); + ok(hr == DD_OK, "Got unexpected hr %#x.\n", hr); + hr = IDirectDraw_CreateClipper(ddraw, 0, &clipper, NULL); + ok(SUCCEEDED(hr), "Failed to create clipper, hr %#x.\n", hr); + hr = IDirectDrawSurface_SetClipper(surface, clipper); + ok(SUCCEEDED(hr), "Failed to set clipper, hr %#x.\n", hr); + + IDirectDrawClipper_Release(clipper); + IDirectDrawClipper_Release(clipper); + + hr = IDirectDrawSurface_GetClipper(surface, &clipper2); + ok(SUCCEEDED(hr), "Failed to get clipper, hr %#x.\n", hr); + ok(clipper == clipper2, "Got clipper %p, expected %p.\n", clipper2, clipper); + + /* Show that invoking the Release method does not crash, but don't get the + * vtable through the clipper pointer because it is no longer pointing to + * valid memory. */ + refcount = orig_vtbl->Release(clipper); + ok(!refcount, "%u references left.\n", refcount); + + refcount = IDirectDrawSurface_Release(surface); + ok(!refcount, "%u references left.\n", refcount); + + /* It looks like the protection against invalid thispointers is part of + * the IDirectDrawClipper method implementation, not IDirectDrawSurface. */ + clipper = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, 0x1000); + ok(!!clipper, "failed to allocate memory\n"); + + /* Assigning the vtable to our fake clipper does NOT make a difference on + * native - there is a different member of the clipper implementation struct + * that is used to determine if a clipper is valid. */ + clipper->lpVtbl = orig_vtbl; + + refcount = orig_vtbl->AddRef(clipper); + todo_wine ok(!refcount, "Got refcount %u.\n", refcount); + refcount = orig_vtbl->AddRef((IDirectDrawClipper *)(ULONG_PTR)0xdeadbeef); + ok(!refcount, "Got refcount %u.\n", refcount); + + changed = 0x1234; + hr = orig_vtbl->IsClipListChanged(clipper, &changed); + todo_wine ok(hr == DDERR_INVALIDPARAMS, "Got unexpected hr %#x.\n", hr); + todo_wine ok(changed == 0x1234, "'changed' changed: %x.\n", changed); + + changed = 0x1234; + hr = orig_vtbl->IsClipListChanged((IDirectDrawClipper *)(ULONG_PTR)0xdeadbeef, &changed); + ok(hr == DDERR_INVALIDPARAMS, "Got unexpected hr %#x.\n", hr); + ok(changed == 0x1234, "'changed' changed: %x.\n", changed); + + /* Nope, we can't initialize our fake clipper. */ + hr = orig_vtbl->Initialize(clipper, ddraw, 0); + todo_wine ok(hr == DDERR_INVALIDPARAMS, "Got unexpected hr %#x.\n", hr); + + HeapFree(GetProcessHeap(), 0, clipper); + + refcount = IDirectDraw_Release(ddraw); + ok(!refcount, "%u references left.\n", refcount); + DestroyWindow(window); +} + START_TEST(ddraw1) { DDDEVICEIDENTIFIER identifier; @@ -12401,4 +12551,5 @@ START_TEST(ddraw1) test_killfocus(); test_gdi_surface(); test_alphatest(); + test_clipper_refcount(); } diff --git a/dlls/ddraw/tests/ddraw2.c b/dlls/ddraw/tests/ddraw2.c index 37238e162fb..7bf6dca422a 100644 --- a/dlls/ddraw/tests/ddraw2.c +++ b/dlls/ddraw/tests/ddraw2.c @@ -13552,6 +13552,163 @@ static void test_alphatest(void) DestroyWindow(window); } +static void test_clipper_refcount(void) +{ + IDirectDrawSurface *surface; + IDirectDrawClipper *clipper, *clipper2; + DDSURFACEDESC surface_desc; + IDirectDraw2 *ddraw; + IDirectDraw *ddraw1; + ULONG refcount; + HWND window; + HRESULT hr; + BOOL changed; + const IDirectDrawClipperVtbl *orig_vtbl; + + window = create_window(); + ddraw = create_ddraw(); + ok(!!ddraw, "Failed to create a ddraw object.\n"); + hr = IDirectDraw2_SetCooperativeLevel(ddraw, window, DDSCL_NORMAL); + ok(hr == DD_OK, "Got unexpected hr %#x.\n", hr); + + memset(&surface_desc, 0, sizeof(surface_desc)); + surface_desc.dwSize = sizeof(surface_desc); + surface_desc.dwFlags = DDSD_CAPS; + surface_desc.ddsCaps.dwCaps = DDSCAPS_PRIMARYSURFACE; + hr = IDirectDraw2_CreateSurface(ddraw, &surface_desc, &surface, NULL); + ok(hr == DD_OK, "Got unexpected hr %#x.\n", hr); + + hr = IDirectDraw2_CreateClipper(ddraw, 0, &clipper, NULL); + ok(SUCCEEDED(hr), "Failed to create clipper, hr %#x.\n", hr); + refcount = get_refcount((IUnknown *)clipper); + ok(refcount == 1, "Got unexpected refcount %u.\n", refcount); + + /* Show that clipper validation doesn't somehow happen through per-clipper vtable + * pointers. */ + hr = IDirectDraw2_CreateClipper(ddraw, 0, &clipper2, NULL); + ok(SUCCEEDED(hr), "Failed to create clipper, hr %#x.\n", hr); + ok(clipper->lpVtbl == clipper2->lpVtbl, "Got different clipper vtables %p and %p.\n", + clipper->lpVtbl, clipper2->lpVtbl); + orig_vtbl = clipper->lpVtbl; + IDirectDrawClipper_Release(clipper2); + + /* Surfaces hold a reference to clippers. No surprises there. */ + hr = IDirectDrawSurface_SetClipper(surface, clipper); + ok(SUCCEEDED(hr), "Failed to set clipper, hr %#x.\n", hr); + refcount = get_refcount((IUnknown *)clipper); + ok(refcount == 2, "Got unexpected refcount %u.\n", refcount); + + hr = IDirectDrawSurface_GetClipper(surface, &clipper2); + ok(SUCCEEDED(hr), "Failed to get clipper, hr %#x.\n", hr); + ok(clipper == clipper2, "Got clipper %p, expected %p.\n", clipper2, clipper); + refcount = IDirectDrawClipper_Release(clipper2); + ok(refcount == 2, "Got unexpected refcount %u.\n", refcount); + + hr = IDirectDrawSurface_SetClipper(surface, NULL); + ok(SUCCEEDED(hr), "Failed to set clipper, hr %#x.\n", hr); + refcount = get_refcount((IUnknown *)clipper); + ok(refcount == 1, "Got unexpected refcount %u.\n", refcount); + + hr = IDirectDrawSurface_SetClipper(surface, clipper); + ok(SUCCEEDED(hr), "Failed to set clipper, hr %#x.\n", hr); + refcount = get_refcount((IUnknown *)clipper); + ok(refcount == 2, "Got unexpected refcount %u.\n", refcount); + + refcount = IDirectDrawSurface_Release(surface); + ok(!refcount, "%u references left.\n", refcount); + refcount = get_refcount((IUnknown *)clipper); + ok(refcount == 1, "Got unexpected refcount %u.\n", refcount); + + /* SetClipper with an invalid pointer crashes. */ + + /* Clipper methods work with a broken vtable, with the exception of Release. */ + clipper->lpVtbl = (void *)0xdeadbeef; + refcount = orig_vtbl->AddRef(clipper); + todo_wine ok(refcount == 2, "Got unexpected refcount %u.\n", refcount); + refcount = orig_vtbl->Release(clipper); + ok(!refcount, "Got unexpected refcount %u.\n", refcount); + + clipper->lpVtbl = orig_vtbl; + refcount = orig_vtbl->Release(clipper); + todo_wine ok(refcount == 1, "Got unexpected refcount %u.\n", refcount); + + /* Fix the refcount difference because Wine did not increase the ref in the + * AddRef call above. */ + if (refcount) + { + refcount = IDirectDrawClipper_Release(clipper); + ok(!refcount, "Got unexpected refcount %u.\n", refcount); + } + + /* Steal the reference and see what happens - releasing the surface works fine. + * The clipper is destroyed and not kept alive by a hidden refcount - trying to + * release it after the GetClipper call is likely to crash, and certain to crash + * if we allocate and zero as much heap memory as we can get. */ + hr = IDirectDraw2_CreateSurface(ddraw, &surface_desc, &surface, NULL); + ok(hr == DD_OK, "Got unexpected hr %#x.\n", hr); + hr = IDirectDraw2_CreateClipper(ddraw, 0, &clipper, NULL); + ok(SUCCEEDED(hr), "Failed to create clipper, hr %#x.\n", hr); + hr = IDirectDrawSurface_SetClipper(surface, clipper); + ok(SUCCEEDED(hr), "Failed to set clipper, hr %#x.\n", hr); + + IDirectDrawClipper_Release(clipper); + IDirectDrawClipper_Release(clipper); + + hr = IDirectDrawSurface_GetClipper(surface, &clipper2); + ok(SUCCEEDED(hr), "Failed to get clipper, hr %#x.\n", hr); + ok(clipper == clipper2, "Got clipper %p, expected %p.\n", clipper2, clipper); + + /* Show that invoking the Release method does not crash, but don't get the + * vtable through the clipper pointer because it is no longer pointing to + * valid memory. */ + refcount = orig_vtbl->Release(clipper); + ok(!refcount, "%u references left.\n", refcount); + + refcount = IDirectDrawSurface_Release(surface); + ok(!refcount, "%u references left.\n", refcount); + + /* It looks like the protection against invalid thispointers is part of + * the IDirectDrawClipper method implementation, not IDirectDrawSurface. */ + clipper = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, 0x1000); + ok(!!clipper, "failed to allocate memory\n"); + + /* Assigning the vtable to our fake clipper does NOT make a difference on + * native - there is a different member of the clipper implementation struct + * that is used to determine if a clipper is valid. */ + clipper->lpVtbl = orig_vtbl; + + refcount = orig_vtbl->AddRef(clipper); + todo_wine ok(!refcount, "Got refcount %u.\n", refcount); + refcount = orig_vtbl->AddRef((IDirectDrawClipper *)(ULONG_PTR)0xdeadbeef); + ok(!refcount, "Got refcount %u.\n", refcount); + + changed = 0x1234; + hr = orig_vtbl->IsClipListChanged(clipper, &changed); + todo_wine ok(hr == DDERR_INVALIDPARAMS, "Got unexpected hr %#x.\n", hr); + todo_wine ok(changed == 0x1234, "'changed' changed: %x.\n", changed); + + changed = 0x1234; + hr = orig_vtbl->IsClipListChanged((IDirectDrawClipper *)(ULONG_PTR)0xdeadbeef, &changed); + ok(hr == DDERR_INVALIDPARAMS, "Got unexpected hr %#x.\n", hr); + ok(changed == 0x1234, "'changed' changed: %x.\n", changed); + + /* Nope, we can't initialize our fake clipper. */ + hr = IDirectDraw2_QueryInterface(ddraw, &IID_IDirectDraw, (void **)&ddraw1); + ok(SUCCEEDED(hr), "Failed to get ddraw1 interface, hr %#x.\n", hr); + + hr = orig_vtbl->Initialize(clipper, ddraw1, 0); + todo_wine ok(hr == DDERR_INVALIDPARAMS, "Got unexpected hr %#x.\n", hr); + + IDirectDraw_Release(ddraw1); + + HeapFree(GetProcessHeap(), 0, clipper); + + refcount = IDirectDraw2_Release(ddraw); + ok(!refcount, "%u references left.\n", refcount); + DestroyWindow(window); +} + + START_TEST(ddraw2) { DDDEVICEIDENTIFIER identifier; @@ -13668,4 +13825,5 @@ START_TEST(ddraw2) test_killfocus(); test_gdi_surface(); test_alphatest(); + test_clipper_refcount(); } diff --git a/dlls/ddraw/tests/ddraw4.c b/dlls/ddraw/tests/ddraw4.c index b60606fca3a..51052fb3568 100644 --- a/dlls/ddraw/tests/ddraw4.c +++ b/dlls/ddraw/tests/ddraw4.c @@ -15798,6 +15798,162 @@ static void test_alphatest(void) DestroyWindow(window); } +static void test_clipper_refcount(void) +{ + IDirectDrawSurface4 *surface; + IDirectDrawClipper *clipper, *clipper2; + DDSURFACEDESC2 surface_desc; + IDirectDraw4 *ddraw; + IDirectDraw *ddraw1; + ULONG refcount; + HWND window; + HRESULT hr; + BOOL changed; + const IDirectDrawClipperVtbl *orig_vtbl; + + window = create_window(); + ddraw = create_ddraw(); + ok(!!ddraw, "Failed to create a ddraw object.\n"); + hr = IDirectDraw4_SetCooperativeLevel(ddraw, window, DDSCL_NORMAL); + ok(hr == DD_OK, "Got unexpected hr %#x.\n", hr); + + memset(&surface_desc, 0, sizeof(surface_desc)); + surface_desc.dwSize = sizeof(surface_desc); + surface_desc.dwFlags = DDSD_CAPS; + surface_desc.ddsCaps.dwCaps = DDSCAPS_PRIMARYSURFACE; + hr = IDirectDraw4_CreateSurface(ddraw, &surface_desc, &surface, NULL); + ok(hr == DD_OK, "Got unexpected hr %#x.\n", hr); + + hr = IDirectDraw4_CreateClipper(ddraw, 0, &clipper, NULL); + ok(SUCCEEDED(hr), "Failed to create clipper, hr %#x.\n", hr); + refcount = get_refcount((IUnknown *)clipper); + ok(refcount == 1, "Got unexpected refcount %u.\n", refcount); + + /* Show that clipper validation doesn't somehow happen through per-clipper vtable + * pointers. */ + hr = IDirectDraw4_CreateClipper(ddraw, 0, &clipper2, NULL); + ok(SUCCEEDED(hr), "Failed to create clipper, hr %#x.\n", hr); + ok(clipper->lpVtbl == clipper2->lpVtbl, "Got different clipper vtables %p and %p.\n", + clipper->lpVtbl, clipper2->lpVtbl); + orig_vtbl = clipper->lpVtbl; + IDirectDrawClipper_Release(clipper2); + + /* Surfaces hold a reference to clippers. No surprises there. */ + hr = IDirectDrawSurface4_SetClipper(surface, clipper); + ok(SUCCEEDED(hr), "Failed to set clipper, hr %#x.\n", hr); + refcount = get_refcount((IUnknown *)clipper); + ok(refcount == 2, "Got unexpected refcount %u.\n", refcount); + + hr = IDirectDrawSurface4_GetClipper(surface, &clipper2); + ok(SUCCEEDED(hr), "Failed to get clipper, hr %#x.\n", hr); + ok(clipper == clipper2, "Got clipper %p, expected %p.\n", clipper2, clipper); + refcount = IDirectDrawClipper_Release(clipper2); + ok(refcount == 2, "Got unexpected refcount %u.\n", refcount); + + hr = IDirectDrawSurface4_SetClipper(surface, NULL); + ok(SUCCEEDED(hr), "Failed to set clipper, hr %#x.\n", hr); + refcount = get_refcount((IUnknown *)clipper); + ok(refcount == 1, "Got unexpected refcount %u.\n", refcount); + + hr = IDirectDrawSurface4_SetClipper(surface, clipper); + ok(SUCCEEDED(hr), "Failed to set clipper, hr %#x.\n", hr); + refcount = get_refcount((IUnknown *)clipper); + ok(refcount == 2, "Got unexpected refcount %u.\n", refcount); + + refcount = IDirectDrawSurface4_Release(surface); + ok(!refcount, "%u references left.\n", refcount); + refcount = get_refcount((IUnknown *)clipper); + ok(refcount == 1, "Got unexpected refcount %u.\n", refcount); + + /* SetClipper with an invalid pointer crashes. */ + + /* Clipper methods work with a broken vtable, with the exception of Release. */ + clipper->lpVtbl = (void *)0xdeadbeef; + refcount = orig_vtbl->AddRef(clipper); + todo_wine ok(refcount == 2, "Got unexpected refcount %u.\n", refcount); + refcount = orig_vtbl->Release(clipper); + ok(!refcount, "Got unexpected refcount %u.\n", refcount); + + clipper->lpVtbl = orig_vtbl; + refcount = orig_vtbl->Release(clipper); + todo_wine ok(refcount == 1, "Got unexpected refcount %u.\n", refcount); + + /* Fix the refcount difference because Wine did not increase the ref in the + * AddRef call above. */ + if (refcount) + { + refcount = IDirectDrawClipper_Release(clipper); + ok(!refcount, "Got unexpected refcount %u.\n", refcount); + } + + /* Steal the reference and see what happens - releasing the surface works fine. + * The clipper is destroyed and not kept alive by a hidden refcount - trying to + * release it after the GetClipper call is likely to crash, and certain to crash + * if we allocate and zero as much heap memory as we can get. */ + hr = IDirectDraw4_CreateSurface(ddraw, &surface_desc, &surface, NULL); + ok(hr == DD_OK, "Got unexpected hr %#x.\n", hr); + hr = IDirectDraw4_CreateClipper(ddraw, 0, &clipper, NULL); + ok(SUCCEEDED(hr), "Failed to create clipper, hr %#x.\n", hr); + hr = IDirectDrawSurface4_SetClipper(surface, clipper); + ok(SUCCEEDED(hr), "Failed to set clipper, hr %#x.\n", hr); + + IDirectDrawClipper_Release(clipper); + IDirectDrawClipper_Release(clipper); + + hr = IDirectDrawSurface4_GetClipper(surface, &clipper2); + ok(SUCCEEDED(hr), "Failed to get clipper, hr %#x.\n", hr); + ok(clipper == clipper2, "Got clipper %p, expected %p.\n", clipper2, clipper); + + /* Show that invoking the Release method does not crash, but don't get the + * vtable through the clipper pointer because it is no longer pointing to + * valid memory. */ + refcount = orig_vtbl->Release(clipper); + ok(!refcount, "%u references left.\n", refcount); + + refcount = IDirectDrawSurface4_Release(surface); + ok(!refcount, "%u references left.\n", refcount); + + /* It looks like the protection against invalid thispointers is part of + * the IDirectDrawClipper method implementation, not IDirectDrawSurface. */ + clipper = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, 0x1000); + ok(!!clipper, "failed to allocate memory\n"); + + /* Assigning the vtable to our fake clipper does NOT make a difference on + * native - there is a different member of the clipper implementation struct + * that is used to determine if a clipper is valid. */ + clipper->lpVtbl = orig_vtbl; + + refcount = orig_vtbl->AddRef(clipper); + todo_wine ok(!refcount, "Got refcount %u.\n", refcount); + refcount = orig_vtbl->AddRef((IDirectDrawClipper *)(ULONG_PTR)0xdeadbeef); + ok(!refcount, "Got refcount %u.\n", refcount); + + changed = 0x1234; + hr = orig_vtbl->IsClipListChanged(clipper, &changed); + todo_wine ok(hr == DDERR_INVALIDPARAMS, "Got unexpected hr %#x.\n", hr); + todo_wine ok(changed == 0x1234, "'changed' changed: %x.\n", changed); + + changed = 0x1234; + hr = orig_vtbl->IsClipListChanged((IDirectDrawClipper *)(ULONG_PTR)0xdeadbeef, &changed); + ok(hr == DDERR_INVALIDPARAMS, "Got unexpected hr %#x.\n", hr); + ok(changed == 0x1234, "'changed' changed: %x.\n", changed); + + /* Nope, we can't initialize our fake clipper. */ + hr = IDirectDraw4_QueryInterface(ddraw, &IID_IDirectDraw, (void **)&ddraw1); + ok(SUCCEEDED(hr), "Failed to get ddraw1 interface, hr %#x.\n", hr); + + hr = orig_vtbl->Initialize(clipper, ddraw1, 0); + todo_wine ok(hr == DDERR_INVALIDPARAMS, "Got unexpected hr %#x.\n", hr); + + IDirectDraw_Release(ddraw1); + + HeapFree(GetProcessHeap(), 0, clipper); + + refcount = IDirectDraw4_Release(ddraw); + ok(!refcount, "%u references left.\n", refcount); + DestroyWindow(window); +} + START_TEST(ddraw4) { DDDEVICEIDENTIFIER identifier; @@ -15929,4 +16085,5 @@ START_TEST(ddraw4) test_sysmem_draw(); test_gdi_surface(); test_alphatest(); + test_clipper_refcount(); } diff --git a/dlls/ddraw/tests/ddraw7.c b/dlls/ddraw/tests/ddraw7.c index 3339b32d200..c199994d520 100644 --- a/dlls/ddraw/tests/ddraw7.c +++ b/dlls/ddraw/tests/ddraw7.c @@ -15599,6 +15599,162 @@ static void test_alphatest(void) DestroyWindow(window); } +static void test_clipper_refcount(void) +{ + IDirectDrawSurface7 *surface; + IDirectDrawClipper *clipper, *clipper2; + DDSURFACEDESC2 surface_desc; + IDirectDraw7 *ddraw; + IDirectDraw *ddraw1; + ULONG refcount; + HWND window; + HRESULT hr; + BOOL changed; + const IDirectDrawClipperVtbl *orig_vtbl; + + window = create_window(); + ddraw = create_ddraw(); + ok(!!ddraw, "Failed to create a ddraw object.\n"); + hr = IDirectDraw7_SetCooperativeLevel(ddraw, window, DDSCL_NORMAL); + ok(hr == DD_OK, "Got unexpected hr %#x.\n", hr); + + memset(&surface_desc, 0, sizeof(surface_desc)); + surface_desc.dwSize = sizeof(surface_desc); + surface_desc.dwFlags = DDSD_CAPS; + surface_desc.ddsCaps.dwCaps = DDSCAPS_PRIMARYSURFACE; + hr = IDirectDraw7_CreateSurface(ddraw, &surface_desc, &surface, NULL); + ok(hr == DD_OK, "Got unexpected hr %#x.\n", hr); + + hr = IDirectDraw7_CreateClipper(ddraw, 0, &clipper, NULL); + ok(SUCCEEDED(hr), "Failed to create clipper, hr %#x.\n", hr); + refcount = get_refcount((IUnknown *)clipper); + ok(refcount == 1, "Got unexpected refcount %u.\n", refcount); + + /* Show that clipper validation doesn't somehow happen through per-clipper vtable + * pointers. */ + hr = IDirectDraw7_CreateClipper(ddraw, 0, &clipper2, NULL); + ok(SUCCEEDED(hr), "Failed to create clipper, hr %#x.\n", hr); + ok(clipper->lpVtbl == clipper2->lpVtbl, "Got different clipper vtables %p and %p.\n", + clipper->lpVtbl, clipper2->lpVtbl); + orig_vtbl = clipper->lpVtbl; + IDirectDrawClipper_Release(clipper2); + + /* Surfaces hold a reference to clippers. No surprises there. */ + hr = IDirectDrawSurface7_SetClipper(surface, clipper); + ok(SUCCEEDED(hr), "Failed to set clipper, hr %#x.\n", hr); + refcount = get_refcount((IUnknown *)clipper); + ok(refcount == 2, "Got unexpected refcount %u.\n", refcount); + + hr = IDirectDrawSurface7_GetClipper(surface, &clipper2); + ok(SUCCEEDED(hr), "Failed to get clipper, hr %#x.\n", hr); + ok(clipper == clipper2, "Got clipper %p, expected %p.\n", clipper2, clipper); + refcount = IDirectDrawClipper_Release(clipper2); + ok(refcount == 2, "Got unexpected refcount %u.\n", refcount); + + hr = IDirectDrawSurface7_SetClipper(surface, NULL); + ok(SUCCEEDED(hr), "Failed to set clipper, hr %#x.\n", hr); + refcount = get_refcount((IUnknown *)clipper); + ok(refcount == 1, "Got unexpected refcount %u.\n", refcount); + + hr = IDirectDrawSurface7_SetClipper(surface, clipper); + ok(SUCCEEDED(hr), "Failed to set clipper, hr %#x.\n", hr); + refcount = get_refcount((IUnknown *)clipper); + ok(refcount == 2, "Got unexpected refcount %u.\n", refcount); + + refcount = IDirectDrawSurface7_Release(surface); + ok(!refcount, "%u references left.\n", refcount); + refcount = get_refcount((IUnknown *)clipper); + ok(refcount == 1, "Got unexpected refcount %u.\n", refcount); + + /* SetClipper with an invalid pointer crashes. */ + + /* Clipper methods work with a broken vtable, with the exception of Release. */ + clipper->lpVtbl = (void *)0xdeadbeef; + refcount = orig_vtbl->AddRef(clipper); + todo_wine ok(refcount == 2, "Got unexpected refcount %u.\n", refcount); + refcount = orig_vtbl->Release(clipper); + ok(!refcount, "Got unexpected refcount %u.\n", refcount); + + clipper->lpVtbl = orig_vtbl; + refcount = orig_vtbl->Release(clipper); + todo_wine ok(refcount == 1, "Got unexpected refcount %u.\n", refcount); + + /* Fix the refcount difference because Wine did not increase the ref in the + * AddRef call above. */ + if (refcount) + { + refcount = IDirectDrawClipper_Release(clipper); + ok(!refcount, "Got unexpected refcount %u.\n", refcount); + } + + /* Steal the reference and see what happens - releasing the surface works fine. + * The clipper is destroyed and not kept alive by a hidden refcount - trying to + * release it after the GetClipper call is likely to crash, and certain to crash + * if we allocate and zero as much heap memory as we can get. */ + hr = IDirectDraw7_CreateSurface(ddraw, &surface_desc, &surface, NULL); + ok(hr == DD_OK, "Got unexpected hr %#x.\n", hr); + hr = IDirectDraw7_CreateClipper(ddraw, 0, &clipper, NULL); + ok(SUCCEEDED(hr), "Failed to create clipper, hr %#x.\n", hr); + hr = IDirectDrawSurface7_SetClipper(surface, clipper); + ok(SUCCEEDED(hr), "Failed to set clipper, hr %#x.\n", hr); + + IDirectDrawClipper_Release(clipper); + IDirectDrawClipper_Release(clipper); + + hr = IDirectDrawSurface7_GetClipper(surface, &clipper2); + ok(SUCCEEDED(hr), "Failed to get clipper, hr %#x.\n", hr); + ok(clipper == clipper2, "Got clipper %p, expected %p.\n", clipper2, clipper); + + /* Show that invoking the Release method does not crash, but don't get the + * vtable through the clipper pointer because it is no longer pointing to + * valid memory. */ + refcount = orig_vtbl->Release(clipper); + ok(!refcount, "%u references left.\n", refcount); + + refcount = IDirectDrawSurface7_Release(surface); + ok(!refcount, "%u references left.\n", refcount); + + /* It looks like the protection against invalid thispointers is part of + * the IDirectDrawClipper method implementation, not IDirectDrawSurface. */ + clipper = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, 0x1000); + ok(!!clipper, "failed to allocate memory\n"); + + /* Assigning the vtable to our fake clipper does NOT make a difference on + * native - there is a different member of the clipper implementation struct + * that is used to determine if a clipper is valid. */ + clipper->lpVtbl = orig_vtbl; + + refcount = orig_vtbl->AddRef(clipper); + todo_wine ok(!refcount, "Got refcount %u.\n", refcount); + refcount = orig_vtbl->AddRef((IDirectDrawClipper *)(ULONG_PTR)0xdeadbeef); + ok(!refcount, "Got refcount %u.\n", refcount); + + changed = 0x1234; + hr = orig_vtbl->IsClipListChanged(clipper, &changed); + todo_wine ok(hr == DDERR_INVALIDPARAMS, "Got unexpected hr %#x.\n", hr); + todo_wine ok(changed == 0x1234, "'changed' changed: %x.\n", changed); + + changed = 0x1234; + hr = orig_vtbl->IsClipListChanged((IDirectDrawClipper *)(ULONG_PTR)0xdeadbeef, &changed); + ok(hr == DDERR_INVALIDPARAMS, "Got unexpected hr %#x.\n", hr); + ok(changed == 0x1234, "'changed' changed: %x.\n", changed); + + /* Nope, we can't initialize our fake clipper. */ + hr = IDirectDraw7_QueryInterface(ddraw, &IID_IDirectDraw, (void **)&ddraw1); + ok(SUCCEEDED(hr), "Failed to get ddraw1 interface, hr %#x.\n", hr); + + hr = orig_vtbl->Initialize(clipper, ddraw1, 0); + todo_wine ok(hr == DDERR_INVALIDPARAMS, "Got unexpected hr %#x.\n", hr); + + IDirectDraw_Release(ddraw1); + + HeapFree(GetProcessHeap(), 0, clipper); + + refcount = IDirectDraw7_Release(ddraw); + ok(!refcount, "%u references left.\n", refcount); + DestroyWindow(window); +} + START_TEST(ddraw7) { DDDEVICEIDENTIFIER2 identifier; @@ -15742,4 +15898,5 @@ START_TEST(ddraw7) test_gdi_surface(); test_multiply_transform(); test_alphatest(); + test_clipper_refcount(); } -- 2.19.2
Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=49255 Your paranoid android. === w2003std (task log) === Task errors: BotError: The VM is not powered on === w2003std (task log) === Task errors: The previous 1 run(s) terminated abnormally === debian9 (build log) === X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) === debian9 (build log) === X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig)
Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=49252 Your paranoid android. === debian9 (build log) === X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) === debian9 (build log) === X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig) X Error of failed request: BadValue (integer parameter out of range for operation) Major opcode of failed request: 140 (RANDR) Minor opcode of failed request: 21 (RRSetCrtcConfig)
participants (4)
-
Henri Verbeet -
Henri Verbeet -
Marvin -
Stefan Dösinger