[PATCH 1/5] ddraw: Protect against invalid clipper pointers.
Signed-off-by: Stefan Dösinger <stefan(a)codeweavers.com> --- dlls/ddraw/clipper.c | 66 +++++++++++++++++++++++++++++++++++--- dlls/ddraw/ddraw_private.h | 1 + 2 files changed, 63 insertions(+), 4 deletions(-) diff --git a/dlls/ddraw/clipper.c b/dlls/ddraw/clipper.c index 01cac40ec6e..95036538d8f 100644 --- a/dlls/ddraw/clipper.c +++ b/dlls/ddraw/clipper.c @@ -26,9 +26,28 @@ WINE_DEFAULT_DEBUG_CHANNEL(ddraw); +static const struct IDirectDrawClipperVtbl ddraw_clipper_vtbl; + +#define DDRAW_CLIPPER_MAGIC (MAKEFOURCC('C','L','I','P')) + static inline struct ddraw_clipper *impl_from_IDirectDrawClipper(IDirectDrawClipper *iface) { - return CONTAINING_RECORD(iface, struct ddraw_clipper, IDirectDrawClipper_iface); + /* Native is very lenient when you invoke the methods (clipper or surface) with a clipper + * pointer that points to something that is either not accessible or not a clipper. Deus + * Ex: Goty depends on this. */ + struct ddraw_clipper *clipper = CONTAINING_RECORD(iface, struct ddraw_clipper, IDirectDrawClipper_iface); + + if (IsBadReadPtr(clipper, sizeof(*clipper))) + { + WARN("The application gave us a bad clipper.\n"); + return NULL; + } + if (clipper->magic != DDRAW_CLIPPER_MAGIC) + { + WARN("The application gave us a bad clipper.\n"); + return NULL; + } + return clipper; } static HRESULT WINAPI ddraw_clipper_QueryInterface(IDirectDrawClipper *iface, REFIID iid, void **object) @@ -37,6 +56,9 @@ static HRESULT WINAPI ddraw_clipper_QueryInterface(IDirectDrawClipper *iface, RE TRACE("iface %p, iid %s, object %p.\n", iface, debugstr_guid(iid), object); + if (!clipper) + return DDERR_INVALIDPARAMS; + if (IsEqualGUID(&IID_IDirectDrawClipper, iid) || IsEqualGUID(&IID_IUnknown, iid)) { @@ -54,17 +76,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 (!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 (!clipper || iface->lpVtbl != &ddraw_clipper_vtbl) + { + WARN("Invalid clipper, returning 0.\n"); + return 0; + } + + refcount = InterlockedDecrement(&clipper->ref); TRACE("%p decreasing refcount to %u.\n", clipper, refcount); @@ -72,6 +108,7 @@ static ULONG WINAPI ddraw_clipper_Release(IDirectDrawClipper *iface) { if (clipper->region) DeleteObject(clipper->region); + clipper->magic = 0; /* Should help with detecting freed clippers. */ heap_free(clipper); } @@ -84,6 +121,9 @@ static HRESULT WINAPI ddraw_clipper_SetHWnd(IDirectDrawClipper *iface, DWORD fla TRACE("iface %p, flags %#x, window %p.\n", iface, flags, window); + if (!clipper) + return DDERR_INVALIDPARAMS; + if (flags) { FIXME("flags %#x, not supported.\n", flags); @@ -161,6 +201,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 (!clipper) + return DDERR_INVALIDPARAMS; + wined3d_mutex_lock(); if (clipper->window) @@ -238,6 +281,9 @@ static HRESULT WINAPI ddraw_clipper_SetClipList(IDirectDrawClipper *iface, RGNDA TRACE("iface %p, region %p, flags %#x.\n", iface, region, flags); + if (!clipper) + return DDERR_INVALIDPARAMS; + wined3d_mutex_lock(); if (clipper->window) @@ -268,6 +314,9 @@ static HRESULT WINAPI ddraw_clipper_GetHWnd(IDirectDrawClipper *iface, HWND *win TRACE("iface %p, window %p.\n", iface, window); + if (!clipper) + return DDERR_INVALIDPARAMS; + wined3d_mutex_lock(); *window = clipper->window; wined3d_mutex_unlock(); @@ -282,6 +331,9 @@ static HRESULT WINAPI ddraw_clipper_Initialize(IDirectDrawClipper *iface, TRACE("iface %p, ddraw %p, flags %#x.\n", iface, ddraw, flags); + if (!clipper) + return DDERR_INVALIDPARAMS; + wined3d_mutex_lock(); if (clipper->initialized) { @@ -297,8 +349,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 (!clipper) + return DDERR_INVALIDPARAMS; + /* XXX What is safest? */ *changed = FALSE; @@ -322,6 +379,7 @@ HRESULT ddraw_clipper_init(struct ddraw_clipper *clipper) { clipper->IDirectDrawClipper_iface.lpVtbl = &ddraw_clipper_vtbl; clipper->ref = 1; + clipper->magic = DDRAW_CLIPPER_MAGIC; return DD_OK; } diff --git a/dlls/ddraw/ddraw_private.h b/dlls/ddraw/ddraw_private.h index 77e28662724..e6971e9bc7f 100644 --- a/dlls/ddraw/ddraw_private.h +++ b/dlls/ddraw/ddraw_private.h @@ -387,6 +387,7 @@ struct ddraw_clipper HWND window; HRGN region; BOOL initialized; + DWORD magic; }; HRESULT ddraw_clipper_init(struct ddraw_clipper *clipper) DECLSPEC_HIDDEN; -- 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=48684 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 95036538d8f..e7e96eb643d 100644 --- a/dlls/ddraw/clipper.c +++ b/dlls/ddraw/clipper.c @@ -73,7 +73,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; @@ -89,7 +89,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; @@ -192,7 +192,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); @@ -308,7 +308,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); @@ -386,9 +386,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 e6971e9bc7f..4a3d3027249 100644 --- a/dlls/ddraw/ddraw_private.h +++ b/dlls/ddraw/ddraw_private.h @@ -393,6 +393,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=48685 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)
Signed-off-by: Stefan Dösinger <stefan(a)codeweavers.com> --- This gets rid of unsafe_impl_from_IDirectDrawClipper, which feels slightly out of place because impl_from_IDirectDrawClipper does plenty of checks. However, we can't use any random clipper implementation because we're not calling the external API through the vtable, so we may want to keep unsafe_impl_from_IDirectDrawClipper for the WARN it writes. Please ignore this patch in this case. --- dlls/ddraw/clipper.c | 15 --------------- dlls/ddraw/ddraw_private.h | 3 +-- dlls/ddraw/surface.c | 26 +++++++++++--------------- 3 files changed, 12 insertions(+), 32 deletions(-) diff --git a/dlls/ddraw/clipper.c b/dlls/ddraw/clipper.c index e7e96eb643d..aa76ac3d162 100644 --- a/dlls/ddraw/clipper.c +++ b/dlls/ddraw/clipper.c @@ -383,18 +383,3 @@ HRESULT ddraw_clipper_init(struct ddraw_clipper *clipper) return DD_OK; } - -struct ddraw_clipper *unsafe_impl_from_IDirectDrawClipper(IDirectDrawClipper *iface) -{ - struct ddraw_clipper *clipper; - - if (!iface) - return NULL; - - 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 4a3d3027249..7fb41d50e99 100644 --- a/dlls/ddraw/ddraw_private.h +++ b/dlls/ddraw/ddraw_private.h @@ -193,7 +193,7 @@ struct ddraw_surface DDSURFACEDESC2 surface_desc; /* Clipper objects */ - struct ddraw_clipper *clipper; + IDirectDrawClipper *clipper; struct ddraw_palette *palette; /* For the ddraw surface list */ @@ -391,7 +391,6 @@ 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 diff --git a/dlls/ddraw/surface.c b/dlls/ddraw/surface.c index 2a5404f03fd..e0214778bfe 100644 --- a/dlls/ddraw/surface.c +++ b/dlls/ddraw/surface.c @@ -1552,8 +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 = ddraw_clipper_GetClipList(&dst_surface->clipper->IDirectDrawClipper_iface, - &dst_rect, NULL, &clip_list_size))) + if (FAILED(hr = ddraw_clipper_GetClipList(dst_surface->clipper, &dst_rect, NULL, &clip_list_size))) { WARN("Failed to get clip list size, hr %#x.\n", hr); return hr; @@ -1565,8 +1564,7 @@ static HRESULT ddraw_surface_blt_clipped(struct ddraw_surface *dst_surface, cons return E_OUTOFMEMORY; } - if (FAILED(hr = ddraw_clipper_GetClipList(&dst_surface->clipper->IDirectDrawClipper_iface, - &dst_rect, clip_list, &clip_list_size))) + if (FAILED(hr = ddraw_clipper_GetClipList(dst_surface->clipper, &dst_rect, clip_list, &clip_list_size))) { WARN("Failed to get clip list, hr %#x.\n", hr); heap_free(clip_list); @@ -4440,7 +4438,7 @@ static HRESULT WINAPI ddraw_surface7_GetClipper(IDirectDrawSurface7 *iface, IDir return DDERR_NOCLIPPERATTACHED; } - *Clipper = &surface->clipper->IDirectDrawClipper_iface; + *Clipper = surface->clipper; ddraw_clipper_AddRef(*Clipper); wined3d_mutex_unlock(); @@ -4496,14 +4494,13 @@ static HRESULT WINAPI ddraw_surface1_GetClipper(IDirectDrawSurface *iface, IDire * *****************************************************************************/ static HRESULT WINAPI ddraw_surface7_SetClipper(IDirectDrawSurface7 *iface, - IDirectDrawClipper *iclipper) + IDirectDrawClipper *clipper) { struct ddraw_surface *This = impl_from_IDirectDrawSurface7(iface); - struct ddraw_clipper *clipper = unsafe_impl_from_IDirectDrawClipper(iclipper); - struct ddraw_clipper *old_clipper = This->clipper; + IDirectDrawClipper *old_clipper = This->clipper; HWND clipWindow; - TRACE("iface %p, clipper %p.\n", iface, iclipper); + TRACE("iface %p, clipper %p.\n", iface, clipper); wined3d_mutex_lock(); if (clipper == This->clipper) @@ -4514,16 +4511,15 @@ static HRESULT WINAPI ddraw_surface7_SetClipper(IDirectDrawSurface7 *iface, This->clipper = clipper; - if (clipper != NULL) - ddraw_clipper_AddRef(iclipper); + if (clipper) + ddraw_clipper_AddRef(clipper); if (old_clipper) - ddraw_clipper_Release(&old_clipper->IDirectDrawClipper_iface); + ddraw_clipper_Release(old_clipper); if ((This->surface_desc.ddsCaps.dwCaps & DDSCAPS_PRIMARYSURFACE) && This->ddraw->wined3d_swapchain) { clipWindow = NULL; - if (clipper) - ddraw_clipper_GetHWnd(iclipper, &clipWindow); + ddraw_clipper_GetHWnd(clipper, &clipWindow); if (clipWindow) { @@ -5797,7 +5793,7 @@ static void STDMETHODCALLTYPE ddraw_surface_wined3d_object_destroyed(void *paren list_remove(&surface->surface_list_entry); if (surface->clipper) - ddraw_clipper_Release(&surface->clipper->IDirectDrawClipper_iface); + ddraw_clipper_Release(surface->clipper); 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=48686 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)
Signed-off-by: Stefan Dösinger <stefan(a)codeweavers.com> --- dlls/ddraw/tests/ddraw1.c | 195 ++++++++++++++++++++++++++++++++++++ dlls/ddraw/tests/ddraw2.c | 201 ++++++++++++++++++++++++++++++++++++++ dlls/ddraw/tests/ddraw4.c | 201 ++++++++++++++++++++++++++++++++++++++ dlls/ddraw/tests/ddraw7.c | 201 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 798 insertions(+) diff --git a/dlls/ddraw/tests/ddraw1.c b/dlls/ddraw/tests/ddraw1.c index d0ed33dbc41..cf25bcbe653 100644 --- a/dlls/ddraw/tests/ddraw1.c +++ b/dlls/ddraw/tests/ddraw1.c @@ -12293,6 +12293,200 @@ 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 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); + 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); + + /* Break the clipper. Clipper-related ops don't crash. AddRef works, Release doesn't. */ + hr = IDirectDraw_CreateSurface(ddraw, &surface_desc, &surface, NULL); + ok(hr == DD_OK, "Got unexpected hr %#x.\n", hr); + hr = IDirectDrawSurface_SetClipper(surface, clipper); + ok(SUCCEEDED(hr), "Failed to set clipper, hr %#x.\n", hr); + + orig_vtbl = clipper->lpVtbl; + clipper->lpVtbl = (void *)0xdeadbeef; + + refcount = orig_vtbl->AddRef(clipper); + ok(refcount == 3, "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); + ok(refcount == 2, "Got unexpected refcount %u.\n", refcount); + + /* GetClipper works just fine. */ + clipper->lpVtbl = (void *)0xdeadbeef; + 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); + + clipper->lpVtbl = orig_vtbl; + refcount = get_refcount((IUnknown *)clipper); + ok(refcount == 3, "Got unexpected refcount %u.\n", refcount); + IDirectDrawClipper_Release(clipper2); + + /* So does SetClipper, except for the Release being a no-op. */ + clipper->lpVtbl = (void *)0xdeadbeef; + hr = IDirectDrawSurface_SetClipper(surface, NULL); + ok(SUCCEEDED(hr), "Failed to set clipper, hr %#x.\n", hr); + + clipper->lpVtbl = orig_vtbl; + refcount = get_refcount((IUnknown *)clipper); + ok(refcount == 2, "Got unexpected refcount %u.\n", refcount); + + hr = IDirectDrawSurface_GetClipper(surface, &clipper2); + ok(hr == DDERR_NOCLIPPERATTACHED, "Got unexpected hr %#x.\n", hr); + ok(!clipper2, "Got clipper %p, expected NULL.\n", clipper2); + + /* Set a broken clipper. This updates the reference and assigns the clipper */ + clipper->lpVtbl = (void *)0xdeadbeef; + hr = IDirectDrawSurface_SetClipper(surface, clipper); + ok(SUCCEEDED(hr), "Failed to set clipper, hr %#x.\n", hr); + + clipper->lpVtbl = orig_vtbl; + refcount = get_refcount((IUnknown *)clipper); + ok(refcount == 3, "Got unexpected refcount %u.\n", refcount); + + /* Refcounting works again with a proper vtbl - GetClipper adds, our release releases. */ + 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 = get_refcount((IUnknown *)clipper); + ok(refcount == 4, "Got unexpected refcount %u.\n", refcount); + refcount = IDirectDrawClipper_Release(clipper2); + ok(refcount == 3, "Got unexpected refcount %u.\n", refcount); + + /* Surface destruction with a broken clipper - Refcount is not changed. */ + clipper->lpVtbl = (void *)0xdeadbeef; + refcount = IDirectDrawSurface_Release(surface); + ok(!refcount, "%u references left.\n", refcount); + + clipper->lpVtbl = orig_vtbl; + refcount = get_refcount((IUnknown *)clipper); + ok(refcount == 3, "Got unexpected refcount %u.\n", refcount); + + IDirectDrawClipper_Release(clipper); /* For the non-released surface ref from unset. */ + IDirectDrawClipper_Release(clipper); /* For the non-released surface ref from release. */ + refcount = IDirectDrawClipper_Release(clipper); /* Our ref. */ + ok(!refcount, "%u references left.\n", refcount); + + 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); + + /* 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. */ + 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); + /* Don't attempt to release clipper2, it is not a valid interface pointer. */ + + refcount = IDirectDrawSurface_Release(surface); + ok(!refcount, "%u references left.\n", refcount); + + /* Trying to set a pointer that is valid read-only memory will crash though, + * e.g. surface->SetClipper((IDirectDrawClipper *)test_clipper_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"); + clipper->lpVtbl = orig_vtbl; + + refcount = orig_vtbl->AddRef(clipper); + ok(!refcount, "Got refcount %u.\n", refcount); + refcount = orig_vtbl->AddRef((IDirectDrawClipper *)0xdeadbeef); + ok(!refcount, "Got refcount %u.\n", refcount); + + changed = 0x1234; + hr = orig_vtbl->IsClipListChanged(clipper, &changed); + ok(hr == DDERR_INVALIDPARAMS, "Got unexpected hr %#x.\n", hr); + ok(changed == 0x1234, "'changed' changed: %x.\n", changed); + + hr = orig_vtbl->IsClipListChanged((IDirectDrawClipper *)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); + ok(hr == DDERR_INVALIDPARAMS, "Got unexpected hr %#x.\n", hr); + + HeapFree(GetProcessHeap(), 0, clipper); + + refcount = IDirectDraw2_Release(ddraw); + ok(!refcount, "%u references left.\n", refcount); + DestroyWindow(window); +} + START_TEST(ddraw1) { DDDEVICEIDENTIFIER identifier; @@ -12401,4 +12595,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..86205db8aa8 100644 --- a/dlls/ddraw/tests/ddraw2.c +++ b/dlls/ddraw/tests/ddraw2.c @@ -13552,6 +13552,206 @@ 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 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); + 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); + + /* Break the clipper. Clipper-related ops don't crash. AddRef works, Release doesn't. */ + hr = IDirectDraw2_CreateSurface(ddraw, &surface_desc, &surface, NULL); + ok(hr == DD_OK, "Got unexpected hr %#x.\n", hr); + hr = IDirectDrawSurface_SetClipper(surface, clipper); + ok(SUCCEEDED(hr), "Failed to set clipper, hr %#x.\n", hr); + + orig_vtbl = clipper->lpVtbl; + clipper->lpVtbl = (void *)0xdeadbeef; + + refcount = orig_vtbl->AddRef(clipper); + ok(refcount == 3, "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); + ok(refcount == 2, "Got unexpected refcount %u.\n", refcount); + + /* GetClipper works just fine. */ + clipper->lpVtbl = (void *)0xdeadbeef; + 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); + + clipper->lpVtbl = orig_vtbl; + refcount = get_refcount((IUnknown *)clipper); + ok(refcount == 3, "Got unexpected refcount %u.\n", refcount); + IDirectDrawClipper_Release(clipper2); + + /* So does SetClipper, except for the Release being a no-op. */ + clipper->lpVtbl = (void *)0xdeadbeef; + hr = IDirectDrawSurface_SetClipper(surface, NULL); + ok(SUCCEEDED(hr), "Failed to set clipper, hr %#x.\n", hr); + + clipper->lpVtbl = orig_vtbl; + refcount = get_refcount((IUnknown *)clipper); + ok(refcount == 2, "Got unexpected refcount %u.\n", refcount); + + hr = IDirectDrawSurface_GetClipper(surface, &clipper2); + ok(hr == DDERR_NOCLIPPERATTACHED, "Got unexpected hr %#x.\n", hr); + ok(!clipper2, "Got clipper %p, expected NULL.\n", clipper2); + + /* Set a broken clipper. This updates the reference and assigns the clipper */ + clipper->lpVtbl = (void *)0xdeadbeef; + hr = IDirectDrawSurface_SetClipper(surface, clipper); + ok(SUCCEEDED(hr), "Failed to set clipper, hr %#x.\n", hr); + + clipper->lpVtbl = orig_vtbl; + refcount = get_refcount((IUnknown *)clipper); + ok(refcount == 3, "Got unexpected refcount %u.\n", refcount); + + /* Refcounting works again with a proper vtbl - GetClipper adds, our release releases. */ + 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 = get_refcount((IUnknown *)clipper); + ok(refcount == 4, "Got unexpected refcount %u.\n", refcount); + refcount = IDirectDrawClipper_Release(clipper2); + ok(refcount == 3, "Got unexpected refcount %u.\n", refcount); + + /* Surface destruction with a broken clipper - Refcount is not changed. */ + clipper->lpVtbl = (void *)0xdeadbeef; + refcount = IDirectDrawSurface_Release(surface); + ok(!refcount, "%u references left.\n", refcount); + + clipper->lpVtbl = orig_vtbl; + refcount = get_refcount((IUnknown *)clipper); + ok(refcount == 3, "Got unexpected refcount %u.\n", refcount); + + IDirectDrawClipper_Release(clipper); /* For the non-released surface ref from unset. */ + IDirectDrawClipper_Release(clipper); /* For the non-released surface ref from release. */ + refcount = IDirectDrawClipper_Release(clipper); /* Our ref. */ + ok(!refcount, "%u references left.\n", refcount); + + 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); + + /* 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. */ + 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); + /* Don't attempt to release clipper2, it is not a valid interface pointer. */ + + refcount = IDirectDrawSurface_Release(surface); + ok(!refcount, "%u references left.\n", refcount); + + /* Trying to set a pointer that is valid read-only memory will crash though, + * e.g. surface->SetClipper((IDirectDrawClipper *)test_clipper_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"); + clipper->lpVtbl = orig_vtbl; + + refcount = orig_vtbl->AddRef(clipper); + ok(!refcount, "Got refcount %u.\n", refcount); + refcount = orig_vtbl->AddRef((IDirectDrawClipper *)0xdeadbeef); + ok(!refcount, "Got refcount %u.\n", refcount); + + changed = 0x1234; + hr = orig_vtbl->IsClipListChanged(clipper, &changed); + ok(hr == DDERR_INVALIDPARAMS, "Got unexpected hr %#x.\n", hr); + ok(changed == 0x1234, "'changed' changed: %x.\n", changed); + + hr = orig_vtbl->IsClipListChanged((IDirectDrawClipper *)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); + 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 +13868,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..f477599f974 100644 --- a/dlls/ddraw/tests/ddraw4.c +++ b/dlls/ddraw/tests/ddraw4.c @@ -15798,6 +15798,206 @@ 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 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); + 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); + + /* Break the clipper. Clipper-related ops don't crash. AddRef works, Release doesn't. */ + hr = IDirectDraw4_CreateSurface(ddraw, &surface_desc, &surface, NULL); + ok(hr == DD_OK, "Got unexpected hr %#x.\n", hr); + hr = IDirectDrawSurface4_SetClipper(surface, clipper); + ok(SUCCEEDED(hr), "Failed to set clipper, hr %#x.\n", hr); + + orig_vtbl = clipper->lpVtbl; + clipper->lpVtbl = (void *)0xdeadbeef; + + refcount = orig_vtbl->AddRef(clipper); + ok(refcount == 3, "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); + ok(refcount == 2, "Got unexpected refcount %u.\n", refcount); + + /* GetClipper works just fine. */ + clipper->lpVtbl = (void *)0xdeadbeef; + 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); + + clipper->lpVtbl = orig_vtbl; + refcount = get_refcount((IUnknown *)clipper); + ok(refcount == 3, "Got unexpected refcount %u.\n", refcount); + IDirectDrawClipper_Release(clipper2); + + /* So does SetClipper, except for the Release being a no-op. */ + clipper->lpVtbl = (void *)0xdeadbeef; + hr = IDirectDrawSurface4_SetClipper(surface, NULL); + ok(SUCCEEDED(hr), "Failed to set clipper, hr %#x.\n", hr); + + clipper->lpVtbl = orig_vtbl; + refcount = get_refcount((IUnknown *)clipper); + ok(refcount == 2, "Got unexpected refcount %u.\n", refcount); + + hr = IDirectDrawSurface4_GetClipper(surface, &clipper2); + ok(hr == DDERR_NOCLIPPERATTACHED, "Got unexpected hr %#x.\n", hr); + ok(!clipper2, "Got clipper %p, expected NULL.\n", clipper2); + + /* Set a broken clipper. This updates the reference and assigns the clipper */ + clipper->lpVtbl = (void *)0xdeadbeef; + hr = IDirectDrawSurface4_SetClipper(surface, clipper); + ok(SUCCEEDED(hr), "Failed to set clipper, hr %#x.\n", hr); + + clipper->lpVtbl = orig_vtbl; + refcount = get_refcount((IUnknown *)clipper); + ok(refcount == 3, "Got unexpected refcount %u.\n", refcount); + + /* Refcounting works again with a proper vtbl - GetClipper adds, our release releases. */ + 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 = get_refcount((IUnknown *)clipper); + ok(refcount == 4, "Got unexpected refcount %u.\n", refcount); + refcount = IDirectDrawClipper_Release(clipper2); + ok(refcount == 3, "Got unexpected refcount %u.\n", refcount); + + /* Surface destruction with a broken clipper - Refcount is not changed. */ + clipper->lpVtbl = (void *)0xdeadbeef; + refcount = IDirectDrawSurface4_Release(surface); + ok(!refcount, "%u references left.\n", refcount); + + clipper->lpVtbl = orig_vtbl; + refcount = get_refcount((IUnknown *)clipper); + ok(refcount == 3, "Got unexpected refcount %u.\n", refcount); + + IDirectDrawClipper_Release(clipper); /* For the non-released surface ref from unset. */ + IDirectDrawClipper_Release(clipper); /* For the non-released surface ref from release. */ + refcount = IDirectDrawClipper_Release(clipper); /* Our ref. */ + ok(!refcount, "%u references left.\n", refcount); + + 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); + + /* 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. */ + 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); + /* Don't attempt to release clipper2, it is not a valid interface pointer. */ + + refcount = IDirectDrawSurface4_Release(surface); + ok(!refcount, "%u references left.\n", refcount); + + /* Trying to set a pointer that is valid read-only memory will crash though, + * e.g. surface->SetClipper((IDirectDrawClipper *)test_clipper_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"); + clipper->lpVtbl = orig_vtbl; + + refcount = orig_vtbl->AddRef(clipper); + ok(!refcount, "Got refcount %u.\n", refcount); + refcount = orig_vtbl->AddRef((IDirectDrawClipper *)0xdeadbeef); + ok(!refcount, "Got refcount %u.\n", refcount); + + changed = 0x1234; + hr = orig_vtbl->IsClipListChanged(clipper, &changed); + ok(hr == DDERR_INVALIDPARAMS, "Got unexpected hr %#x.\n", hr); + ok(changed == 0x1234, "'changed' changed: %x.\n", changed); + + hr = orig_vtbl->IsClipListChanged((IDirectDrawClipper *)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); + 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 +16129,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..aaf7f651780 100644 --- a/dlls/ddraw/tests/ddraw7.c +++ b/dlls/ddraw/tests/ddraw7.c @@ -15599,6 +15599,206 @@ 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 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); + 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); + + /* Break the clipper. Clipper-related ops don't crash. AddRef works, Release doesn't. */ + hr = IDirectDraw7_CreateSurface(ddraw, &surface_desc, &surface, NULL); + ok(hr == DD_OK, "Got unexpected hr %#x.\n", hr); + hr = IDirectDrawSurface7_SetClipper(surface, clipper); + ok(SUCCEEDED(hr), "Failed to set clipper, hr %#x.\n", hr); + + orig_vtbl = clipper->lpVtbl; + clipper->lpVtbl = (void *)0xdeadbeef; + + refcount = orig_vtbl->AddRef(clipper); + ok(refcount == 3, "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); + ok(refcount == 2, "Got unexpected refcount %u.\n", refcount); + + /* GetClipper works just fine. */ + clipper->lpVtbl = (void *)0xdeadbeef; + 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); + + clipper->lpVtbl = orig_vtbl; + refcount = get_refcount((IUnknown *)clipper); + ok(refcount == 3, "Got unexpected refcount %u.\n", refcount); + IDirectDrawClipper_Release(clipper2); + + /* So does SetClipper, except for the Release being a no-op. */ + clipper->lpVtbl = (void *)0xdeadbeef; + hr = IDirectDrawSurface7_SetClipper(surface, NULL); + ok(SUCCEEDED(hr), "Failed to set clipper, hr %#x.\n", hr); + + clipper->lpVtbl = orig_vtbl; + refcount = get_refcount((IUnknown *)clipper); + ok(refcount == 2, "Got unexpected refcount %u.\n", refcount); + + hr = IDirectDrawSurface7_GetClipper(surface, &clipper2); + ok(hr == DDERR_NOCLIPPERATTACHED, "Got unexpected hr %#x.\n", hr); + ok(!clipper2, "Got clipper %p, expected NULL.\n", clipper2); + + /* Set a broken clipper. This updates the reference and assigns the clipper */ + clipper->lpVtbl = (void *)0xdeadbeef; + hr = IDirectDrawSurface7_SetClipper(surface, clipper); + ok(SUCCEEDED(hr), "Failed to set clipper, hr %#x.\n", hr); + + clipper->lpVtbl = orig_vtbl; + refcount = get_refcount((IUnknown *)clipper); + ok(refcount == 3, "Got unexpected refcount %u.\n", refcount); + + /* Refcounting works again with a proper vtbl - GetClipper adds, our release releases. */ + 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 = get_refcount((IUnknown *)clipper); + ok(refcount == 4, "Got unexpected refcount %u.\n", refcount); + refcount = IDirectDrawClipper_Release(clipper2); + ok(refcount == 3, "Got unexpected refcount %u.\n", refcount); + + /* Surface destruction with a broken clipper - Refcount is not changed. */ + clipper->lpVtbl = (void *)0xdeadbeef; + refcount = IDirectDrawSurface7_Release(surface); + ok(!refcount, "%u references left.\n", refcount); + + clipper->lpVtbl = orig_vtbl; + refcount = get_refcount((IUnknown *)clipper); + ok(refcount == 3, "Got unexpected refcount %u.\n", refcount); + + IDirectDrawClipper_Release(clipper); /* For the non-released surface ref from unset. */ + IDirectDrawClipper_Release(clipper); /* For the non-released surface ref from release. */ + refcount = IDirectDrawClipper_Release(clipper); /* Our ref. */ + ok(!refcount, "%u references left.\n", refcount); + + 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); + + /* 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. */ + 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); + /* Don't attempt to release clipper2, it is not a valid interface pointer. */ + + refcount = IDirectDrawSurface7_Release(surface); + ok(!refcount, "%u references left.\n", refcount); + + /* Trying to set a pointer that is valid read-only memory will crash though, + * e.g. surface->SetClipper((IDirectDrawClipper *)test_clipper_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"); + clipper->lpVtbl = orig_vtbl; + + refcount = orig_vtbl->AddRef(clipper); + ok(!refcount, "Got refcount %u.\n", refcount); + refcount = orig_vtbl->AddRef((IDirectDrawClipper *)0xdeadbeef); + ok(!refcount, "Got refcount %u.\n", refcount); + + changed = 0x1234; + hr = orig_vtbl->IsClipListChanged(clipper, &changed); + ok(hr == DDERR_INVALIDPARAMS, "Got unexpected hr %#x.\n", hr); + ok(changed == 0x1234, "'changed' changed: %x.\n", changed); + + hr = orig_vtbl->IsClipListChanged((IDirectDrawClipper *)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); + 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 +15942,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=48687 Your paranoid android. === wvistau64 (64 bit report) === ddraw: ddraw4.c:2997: Test failed: Expected message 0x46, but didn't receive it. ddraw4.c:3000: Test failed: Expected (0,0)-(640,480), got (0,0)-(1024,768). ddraw4.c:3004: Test failed: Got unexpect screen size 1024x768. === wvistau64 (64 bit report) === ddraw: ddraw7.c:2719: Test failed: Expected message 0x46, but didn't receive it. ddraw7.c:2721: Test failed: Expected screen size 1024x768, got 0x0. ddraw7.c:2727: Test failed: Expected (0,0)-(1024,768), got (-8,-8)-(1032,746). ddraw7.c:2757: Test failed: Expected (0,0)-(1024,768), got (-8,-8)-(1032,746). ddraw7.c:2764: Test failed: Expected (0,0)-(1024,768), got (-8,-8)-(1032,746). ddraw7.c:2790: Test failed: Expected (0,0)-(1024,768), got (-8,-8)-(1032,746). ddraw7.c:2813: Test failed: Expected (0,0)-(1024,768), got (-8,-8)-(1032,746). ddraw7.c:2835: Test failed: Expected (0,0)-(1024,768), got (-8,-8)-(1032,746). ddraw7.c:2861: Test failed: Expected (0,0)-(1024,768), got (-8,-8)-(1032,746). ddraw7.c:2881: Test failed: Expected (0,0)-(1024,768), got (-8,-8)-(1032,746). ddraw7.c:2917: Test failed: Expected (0,0)-(1024,768), got (-8,-8)-(1032,746). ddraw7.c:2927: Test failed: Expected (0,0)-(1024,768), got (-8,-8)-(1032,746). ddraw7.c:2953: Test failed: Expected (0,0)-(1024,768), got (-8,-8)-(1032,746). ddraw7.c:2976: Test failed: Expected (0,0)-(1024,768), got (-8,-8)-(1032,746). ddraw7.c:2998: Test failed: Expected (0,0)-(1024,768), got (-8,-8)-(1032,746). ddraw7.c:3024: Test failed: Expected (0,0)-(1024,768), got (-8,-8)-(1032,746). ddraw7.c:3044: Test failed: Expected (0,0)-(1024,768), got (-8,-8)-(1032,746). ddraw7.c:3081: Test failed: Expected (0,0)-(1024,768), got (-8,-8)-(1032,746). === 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=48683 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 Mon, 4 Mar 2019 at 23:29, Stefan Dösinger <stefan(a)codeweavers.com> wrote:
Signed-off-by: Stefan Dösinger <stefan(a)codeweavers.com> --- dlls/ddraw/clipper.c | 66 +++++++++++++++++++++++++++++++++++--- dlls/ddraw/ddraw_private.h | 1 + 2 files changed, 63 insertions(+), 4 deletions(-)
That's pretty ugly. Would it work to introduce a helper along the lines of the floowing? static BOOL ddraw_clipper_is_valid(struct ddraw_clipper *clipper) { return !IsBadReadPtr(clipper, sizeof(*clipper)) && clipper->IDirectDrawClipper_iface.lpVtbl == &ddraw_clipper_vtbl; }
Am 05.03.2019 um 23:16 schrieb Henri Verbeet <hverbeet(a)gmail.com>:
That's pretty ugly. I know. But which part do you think could be improved in particular? Moving the extra checks out of impl_from_IDirectDrawClipper to keep it in line with the standard way we do COM objects?
Would it work to introduce a helper along the lines of the floowing?
static BOOL ddraw_clipper_is_valid(struct ddraw_clipper *clipper) { return !IsBadReadPtr(clipper, sizeof(*clipper)) && clipper->IDirectDrawClipper_iface.lpVtbl == &ddraw_clipper_vtbl; } The vtable only matters for Release(), the rest of the methods don't care if you change the vtable, that's why I added the magic value.
If we only care about fixing Deus Ex for now I could set the vtable to NULL on release and add a try-catch around the clipper release in ddraw_surface_wined3d_object_destroyed. That would be less invasive, but we couldn't add the test because it would crash.
On Thu, 7 Mar 2019 at 22:42, Stefan Dösinger <stefan(a)codeweavers.com> wrote:
Am 05.03.2019 um 23:16 schrieb Henri Verbeet <hverbeet(a)gmail.com>: That's pretty ugly. I know. But which part do you think could be improved in particular? Moving the extra checks out of impl_from_IDirectDrawClipper to keep it in line with the standard way we do COM objects?
Mostly the fact that impl_from_IDirectDrawClipper() can return NULL, yes. I'm not entirely sure ddraw needs more magic either, but I could live with that part if needed.
Would it work to introduce a helper along the lines of the floowing?
static BOOL ddraw_clipper_is_valid(struct ddraw_clipper *clipper) { return !IsBadReadPtr(clipper, sizeof(*clipper)) && clipper->IDirectDrawClipper_iface.lpVtbl == &ddraw_clipper_vtbl; } The vtable only matters for Release(), the rest of the methods don't care if you change the vtable, that's why I added the magic value.
Do applications care though? In principle impl_from_IDirectDrawClipper() doesn't currently check the vtable, but in practice passing a clipper with a modified vtable to just about anything is going to trigger the assert in unsafe_impl_from_IDirectDrawClipper(). I.e., as long as applications aren't actually modifying the vtable and still expect things to work, I'd probably prefer using the vtable as "magic".
Am 08.03.2019 um 18:49 schrieb Henri Verbeet <hverbeet(a)gmail.com>:
Mostly the fact that impl_from_IDirectDrawClipper() can return NULL, yes. I can easily change that part.
The vtable only matters for Release(), the rest of the methods don't care if you change the vtable, that's why I added the magic value.
Do applications care though? In principle impl_from_IDirectDrawClipper() doesn't currently check the vtable, but in practice passing a clipper with a modified vtable to just about anything is going to trigger the assert in unsafe_impl_from_IDirectDrawClipper(). Deus Ex doesn't, it only cares that we don't crash when attempting to release an already destroyed clipper. I do not know if there are other applications affected by this, but I don't expect there are many, or we would have seen this issue already.
I.e., as long as applications aren't actually modifying the vtable and still expect things to work, I'd probably prefer using the vtable as "magic". I changed the patches in that regard and adjusted the tests. The tests still document that the vtable shouldn't be used to detect valid clippers, but are marked todo.
participants (3)
-
Henri Verbeet -
Marvin -
Stefan Dösinger