[PATCH 0/1] MR8419: ddraw: Fix use-after-free of the ddraw object.
ddraw surfaces also hold references to their ddraw objects, but that's not cleared in ddraw_destroy. * * * not sure if other uses of surface->ddraw need to be guarded too, let me know if i need to change them too. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8419
From: Yuxuan Shui <yshui(a)codeweavers.com> ddraw surfaces also hold references to their ddraw objects, but that's not cleared in ddraw_destroy. --- dlls/ddraw/ddraw.c | 6 ++++++ dlls/ddraw/surface.c | 18 +++++++++++------- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/dlls/ddraw/ddraw.c b/dlls/ddraw/ddraw.c index ed342bfd708..5097bf72975 100644 --- a/dlls/ddraw/ddraw.c +++ b/dlls/ddraw/ddraw.c @@ -421,6 +421,7 @@ static void ddraw_destroy_swapchain(struct ddraw *ddraw) static void ddraw_destroy(struct ddraw *This) { struct d3d_device *device; + struct ddraw_surface *surface; IDirectDraw7_SetCooperativeLevel(&This->IDirectDraw7_iface, NULL, DDSCL_NORMAL); IDirectDraw7_RestoreDisplayMode(&This->IDirectDraw7_iface); @@ -446,6 +447,11 @@ static void ddraw_destroy(struct ddraw *This) device->ddraw = NULL; } + LIST_FOR_EACH_ENTRY(surface, &This->surface_list, struct ddraw_surface, surface_list_entry) + { + surface->ddraw = NULL; + } + /* Now free the object */ free(This); } diff --git a/dlls/ddraw/surface.c b/dlls/ddraw/surface.c index 545c76738cd..ba0af882959 100644 --- a/dlls/ddraw/surface.c +++ b/dlls/ddraw/surface.c @@ -2218,12 +2218,15 @@ static HRESULT ddraw_surface_delete_attached_surface(struct ddraw_surface *surfa * QueryInterface(). Some applications, SCP - Containment Breach in * particular, modify the QueryInterface() pointer in the surface vtbl * but don't cleanup properly after the relevant dll is unloaded. */ - dsv = wined3d_device_context_get_depth_stencil_view(surface->ddraw->immediate_context); - if (attachment->surface_desc.ddsCaps.dwCaps & DDSCAPS_ZBUFFER && dsv == attachment->wined3d_rtv) + if (surface->ddraw) { - wined3d_device_context_set_depth_stencil_view(surface->ddraw->immediate_context, NULL); - LIST_FOR_EACH_ENTRY(device, &surface->ddraw->d3ddevice_list, struct d3d_device, ddraw_entry) - wined3d_stateblock_depth_buffer_changed(device->state); + dsv = wined3d_device_context_get_depth_stencil_view(surface->ddraw->immediate_context); + if (attachment->surface_desc.ddsCaps.dwCaps & DDSCAPS_ZBUFFER && dsv == attachment->wined3d_rtv) + { + wined3d_device_context_set_depth_stencil_view(surface->ddraw->immediate_context, NULL); + LIST_FOR_EACH_ENTRY(device, &surface->ddraw->d3ddevice_list, struct d3d_device, ddraw_entry) + wined3d_stateblock_depth_buffer_changed(device->state); + } } wined3d_mutex_unlock(); @@ -6062,12 +6065,13 @@ static void STDMETHODCALLTYPE ddraw_surface_wined3d_object_destroyed(void *paren ddraw_free_handle(NULL, surface->Handle - 1, DDRAW_HANDLE_SURFACE); /* Reduce the ddraw surface count. */ - list_remove(&surface->surface_list_entry); + if (surface->ddraw) + list_remove(&surface->surface_list_entry); if (surface->clipper && ddraw_clipper_is_valid(surface->clipper)) IDirectDrawClipper_Release(&surface->clipper->IDirectDrawClipper_iface); - if (surface == surface->ddraw->primary) + if (surface->ddraw && surface == surface->ddraw->primary) { surface->ddraw->primary = NULL; surface->ddraw->gdi_surface = NULL; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8419
Is this triggered by a test? IIRC from the last time this came up, the idea was that native will actually crash similarly if you release ddraw objects in the wrong order, and that we should just fix any code that does so. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8419#note_107818
On Wed Jun 25 01:09:14 2025 +0000, Elizabeth Figura wrote:
Is this triggered by a test? IIRC from the last time this came up, the idea was that native will actually crash similarly if you release ddraw objects in the wrong order, and that we should just fix any code that does so. yep, here's the stack trace:
``` 01e4:01e8:err:asan:asan_report ASan: read of 8 bytes at 00007274657ED0A0, caller 00006FFFE254BE7C (__asan_report_load8_noabort, ../dlls/asan_dynamic_thunk/thunk.c:1020,1) 01e4:01e8:err:asan:asan_report stacktrace: 01e4:01e8:err:asan:asan_report 00006FFFE254BE7C (__asan_report_load8_noabort, ../dlls/asan_dynamic_thunk/thunk.c:1020,1) 01e4:01e8:err:asan:asan_report 00006FFFE25CC7C6 (ddraw_surface_delete_attached_surface, ../dlls/ddraw/surface.c:2221,71) 01e4:01e8:err:asan:asan_report 00006FFFE25D13BD (ddraw_surface_wined3d_object_destroyed, ../dlls/ddraw/surface.c:6056,12) 01e4:01e8:err:asan:asan_report 00006FFFDCC09F06 (wined3d_texture_sub_resources_destroyed, ../dlls/wined3d/texture.c:1124,34) 01e4:01e8:err:asan:asan_report 00006FFFDC612C93 (adapter_gl_destroy_texture, ../dlls/wined3d/adapter_gl.c:4548,22) 01e4:01e8:err:asan:asan_report 00006FFFDCBECC04 (wined3d_texture_decref, ../dlls/wined3d/texture.c:1642,13) 01e4:01e8:err:asan:asan_report 00006FFFE25C7DD5 (ddraw_surface_cleanup, ../dlls/ddraw/surface.c:614,1) 01e4:01e8:err:asan:asan_report 00006FFFE25CEAE0 (ddraw_surface_release_iface, ../dlls/ddraw/surface.c:646,9) 01e4:01e8:err:asan:asan_report 00006FFFE25B09C0 (ddraw_surface1_Release, ../dlls/ddraw/surface.c:759,12) 01e4:01e8:err:asan:asan_report 00006FFFE2577CE7 (d3d_device_inner_Release, ../dlls/ddraw/device.c:345,9) 01e4:01e8:err:asan:asan_report 00006FFFE255E144 (d3d_device2_Release, ../dlls/ddraw/device.c:381,1) 01e4:01e8:err:asan:asan_report 00007FFFFE1CD316 (test_d3d_state_reset, ../dlls/ddraw/tests/ddraw2.c:16927,5) 01e4:01e8:err:asan:asan_report 00007FFFFE078623 (func_ddraw2, ../dlls/ddraw/tests/ddraw2.c:17379,5) 01e4:01e8:err:asan:asan_report 00007FFFFE088B23 (run_test, ../include/wine/test.h:785,9) 01e4:01e8:err:asan:asan_report 00007FFFFE08123F (main, ../include/wine/test.h:903,1) 01e4:01e8:err:asan:asan_report 00007FFFFE08147E (mainCRTStartup, ../dlls/msvcrt/crt_main.c:60,11) 01e4:01e8:err:asan:asan_report 00006FFFFD075013 (BaseThreadInitThunk, ../dlls/kernel32/thread.c:61,5) 01e4:01e8:err:asan:asan_report 00006FFFFE3DFDC3 (RtlUserThreadStart) 01e4:01e8:err:asan:asan_report info: 01e4:01e8:err:asan:asan_report heap-use-after-free, addr 00007274657ED0A0 01e4:01e8:err:asan:asan_report allocated user region: [00007274657ED020, 00007274657ED168) 328 01e4:01e8:err:asan:asan_report allocated by 15f61 01e4:01e8:err:asan:asan_report 00006FFFFE3F7B52 (RtlAllocateHeap, ../dlls/ntdll/heap.c:2320,5) 01e4:01e8:err:asan:asan_report 00006FFFF794E803 (msvcrt_heap_alloc, ../dlls/msvcrt/heap.c:72,1) 01e4:01e8:err:asan:asan_report 00006FFFF7903820 (calloc, ../dlls/msvcrt/heap.c:397,1) 01e4:01e8:err:asan:asan_report 00006FFFE2542809 (DDRAW_Create, ../dlls/ddraw/main.c:325,19) 01e4:01e8:err:asan:asan_report 00006FFFE2546E0F (DirectDrawCreate, ../dlls/ddraw/main.c:365,10) 01e4:01e8:err:asan:asan_report 00007FFFFE04DD37 (create_ddraw, ../dlls/ddraw/tests/ddraw2.c:455,8) 01e4:01e8:err:asan:asan_report 00007FFFFE1CBA06 (test_d3d_state_reset, ../dlls/ddraw/tests/ddraw2.c:16853,13) 01e4:01e8:err:asan:asan_report 00007FFFFE078623 (func_ddraw2, ../dlls/ddraw/tests/ddraw2.c:17379,5) 01e4:01e8:err:asan:asan_report 00007FFFFE088B23 (run_test, ../include/wine/test.h:785,9) 01e4:01e8:err:asan:asan_report 00007FFFFE08123F (main, ../include/wine/test.h:903,1) 01e4:01e8:err:asan:asan_report 00007FFFFE08147E (mainCRTStartup, ../dlls/msvcrt/crt_main.c:60,11) 01e4:01e8:err:asan:asan_report 00006FFFFD075013 (BaseThreadInitThunk, ../dlls/kernel32/thread.c:61,5) 01e4:01e8:err:asan:asan_report 00006FFFFE3DFDC3 (RtlUserThreadStart) 01e4:01e8:err:asan:asan_report freed by 161ba 01e4:01e8:err:asan:asan_report 00006FFFFE411A8D (RtlFreeHeap, ../dlls/ntdll/heap.c:2429,13) 01e4:01e8:err:asan:asan_report 00006FFFF794E8CB (msvcrt_heap_free, ../dlls/msvcrt/heap.c:115,1) 01e4:01e8:err:asan:asan_report 00006FFFF7932CC0 (free, ../dlls/msvcrt/heap.c:415,1) 01e4:01e8:err:asan:asan_report 00006FFFE25A3581 (ddraw_destroy, ../dlls/ddraw/ddraw.c:451,1) 01e4:01e8:err:asan:asan_report 00006FFFE2592569 (ddraw2_Release, ../dlls/ddraw/ddraw.c:496,12) 01e4:01e8:err:asan:asan_report 00007FFFFE1CD2BE (test_d3d_state_reset, ../dlls/ddraw/tests/ddraw2.c:16926,5) 01e4:01e8:err:asan:asan_report 00007FFFFE078623 (func_ddraw2, ../dlls/ddraw/tests/ddraw2.c:17379,5) 01e4:01e8:err:asan:asan_report 00007FFFFE088B23 (run_test, ../include/wine/test.h:785,9) 01e4:01e8:err:asan:asan_report 00007FFFFE08123F (main, ../include/wine/test.h:903,1) 01e4:01e8:err:asan:asan_report 00007FFFFE08147E (mainCRTStartup, ../dlls/msvcrt/crt_main.c:60,11) 01e4:01e8:err:asan:asan_report 00006FFFFD075013 (BaseThreadInitThunk, ../dlls/kernel32/thread.c:61,5) 01e4:01e8:err:asan:asan_report 00006FFFFE3DFDC3 (RtlUserThreadStart) ``` clearly the device held a reference to the surface so the surface was released after the ddraw. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8419#note_107860
On Wed Jun 25 01:09:14 2025 +0000, Yuxuan Shui wrote:
yep, here's the stack trace: ``` 01e4:01e8:err:asan:asan_report ASan: read of 8 bytes at 00007274657ED0A0, caller 00006FFFE254BE7C (__asan_report_load8_noabort, ../dlls/asan_dynamic_thunk/thunk.c:1020,1) 01e4:01e8:err:asan:asan_report stacktrace: 01e4:01e8:err:asan:asan_report 00006FFFE254BE7C (__asan_report_load8_noabort, ../dlls/asan_dynamic_thunk/thunk.c:1020,1) 01e4:01e8:err:asan:asan_report 00006FFFE25CC7C6 (ddraw_surface_delete_attached_surface, ../dlls/ddraw/surface.c:2221,71) 01e4:01e8:err:asan:asan_report 00006FFFE25D13BD (ddraw_surface_wined3d_object_destroyed, ../dlls/ddraw/surface.c:6056,12) 01e4:01e8:err:asan:asan_report 00006FFFDCC09F06 (wined3d_texture_sub_resources_destroyed, ../dlls/wined3d/texture.c:1124,34) 01e4:01e8:err:asan:asan_report 00006FFFDC612C93 (adapter_gl_destroy_texture, ../dlls/wined3d/adapter_gl.c:4548,22) 01e4:01e8:err:asan:asan_report 00006FFFDCBECC04 (wined3d_texture_decref, ../dlls/wined3d/texture.c:1642,13) 01e4:01e8:err:asan:asan_report 00006FFFE25C7DD5 (ddraw_surface_cleanup, ../dlls/ddraw/surface.c:614,1) 01e4:01e8:err:asan:asan_report 00006FFFE25CEAE0 (ddraw_surface_release_iface, ../dlls/ddraw/surface.c:646,9) 01e4:01e8:err:asan:asan_report 00006FFFE25B09C0 (ddraw_surface1_Release, ../dlls/ddraw/surface.c:759,12) 01e4:01e8:err:asan:asan_report 00006FFFE2577CE7 (d3d_device_inner_Release, ../dlls/ddraw/device.c:345,9) 01e4:01e8:err:asan:asan_report 00006FFFE255E144 (d3d_device2_Release, ../dlls/ddraw/device.c:381,1) 01e4:01e8:err:asan:asan_report 00007FFFFE1CD316 (test_d3d_state_reset, ../dlls/ddraw/tests/ddraw2.c:16927,5) 01e4:01e8:err:asan:asan_report 00007FFFFE078623 (func_ddraw2, ../dlls/ddraw/tests/ddraw2.c:17379,5) 01e4:01e8:err:asan:asan_report 00007FFFFE088B23 (run_test, ../include/wine/test.h:785,9) 01e4:01e8:err:asan:asan_report 00007FFFFE08123F (main, ../include/wine/test.h:903,1) 01e4:01e8:err:asan:asan_report 00007FFFFE08147E (mainCRTStartup, ../dlls/msvcrt/crt_main.c:60,11) 01e4:01e8:err:asan:asan_report 00006FFFFD075013 (BaseThreadInitThunk, ../dlls/kernel32/thread.c:61,5) 01e4:01e8:err:asan:asan_report 00006FFFFE3DFDC3 (RtlUserThreadStart) 01e4:01e8:err:asan:asan_report info: 01e4:01e8:err:asan:asan_report heap-use-after-free, addr 00007274657ED0A0 01e4:01e8:err:asan:asan_report allocated user region: [00007274657ED020, 00007274657ED168) 328 01e4:01e8:err:asan:asan_report allocated by 15f61 01e4:01e8:err:asan:asan_report 00006FFFFE3F7B52 (RtlAllocateHeap, ../dlls/ntdll/heap.c:2320,5) 01e4:01e8:err:asan:asan_report 00006FFFF794E803 (msvcrt_heap_alloc, ../dlls/msvcrt/heap.c:72,1) 01e4:01e8:err:asan:asan_report 00006FFFF7903820 (calloc, ../dlls/msvcrt/heap.c:397,1) 01e4:01e8:err:asan:asan_report 00006FFFE2542809 (DDRAW_Create, ../dlls/ddraw/main.c:325,19) 01e4:01e8:err:asan:asan_report 00006FFFE2546E0F (DirectDrawCreate, ../dlls/ddraw/main.c:365,10) 01e4:01e8:err:asan:asan_report 00007FFFFE04DD37 (create_ddraw, ../dlls/ddraw/tests/ddraw2.c:455,8) 01e4:01e8:err:asan:asan_report 00007FFFFE1CBA06 (test_d3d_state_reset, ../dlls/ddraw/tests/ddraw2.c:16853,13) 01e4:01e8:err:asan:asan_report 00007FFFFE078623 (func_ddraw2, ../dlls/ddraw/tests/ddraw2.c:17379,5) 01e4:01e8:err:asan:asan_report 00007FFFFE088B23 (run_test, ../include/wine/test.h:785,9) 01e4:01e8:err:asan:asan_report 00007FFFFE08123F (main, ../include/wine/test.h:903,1) 01e4:01e8:err:asan:asan_report 00007FFFFE08147E (mainCRTStartup, ../dlls/msvcrt/crt_main.c:60,11) 01e4:01e8:err:asan:asan_report 00006FFFFD075013 (BaseThreadInitThunk, ../dlls/kernel32/thread.c:61,5) 01e4:01e8:err:asan:asan_report 00006FFFFE3DFDC3 (RtlUserThreadStart) 01e4:01e8:err:asan:asan_report freed by 161ba 01e4:01e8:err:asan:asan_report 00006FFFFE411A8D (RtlFreeHeap, ../dlls/ntdll/heap.c:2429,13) 01e4:01e8:err:asan:asan_report 00006FFFF794E8CB (msvcrt_heap_free, ../dlls/msvcrt/heap.c:115,1) 01e4:01e8:err:asan:asan_report 00006FFFF7932CC0 (free, ../dlls/msvcrt/heap.c:415,1) 01e4:01e8:err:asan:asan_report 00006FFFE25A3581 (ddraw_destroy, ../dlls/ddraw/ddraw.c:451,1) 01e4:01e8:err:asan:asan_report 00006FFFE2592569 (ddraw2_Release, ../dlls/ddraw/ddraw.c:496,12) 01e4:01e8:err:asan:asan_report 00007FFFFE1CD2BE (test_d3d_state_reset, ../dlls/ddraw/tests/ddraw2.c:16926,5) 01e4:01e8:err:asan:asan_report 00007FFFFE078623 (func_ddraw2, ../dlls/ddraw/tests/ddraw2.c:17379,5) 01e4:01e8:err:asan:asan_report 00007FFFFE088B23 (run_test, ../include/wine/test.h:785,9) 01e4:01e8:err:asan:asan_report 00007FFFFE08123F (main, ../include/wine/test.h:903,1) 01e4:01e8:err:asan:asan_report 00007FFFFE08147E (mainCRTStartup, ../dlls/msvcrt/crt_main.c:60,11) 01e4:01e8:err:asan:asan_report 00006FFFFD075013 (BaseThreadInitThunk, ../dlls/kernel32/thread.c:61,5) 01e4:01e8:err:asan:asan_report 00006FFFFE3DFDC3 (RtlUserThreadStart) ``` clearly the device held a reference to the surface so the surface was released after the ddraw. native doesn't crash on this test case right? so maybe we shouldn't either?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8419#note_107861
native doesn't crash on this test case right? so maybe we shouldn't either? (well i guess we didn't crash either, but hard to imagine releasing things in this order can cause uaf)
Maybe not this test case, but according to my undersatnding, doing more or less the same thing—releasing the ddraw before the device—has crashed native before. Since the device doesn't hold a reference to the ddraw I'm not convinced it makes sense to do so here. I think we just fix the relevant test to release the ddraw last. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8419#note_107866
This merge request was closed by Yuxuan Shui. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8419
participants (3)
-
Elizabeth Figura (＠zfigura) -
Yuxuan Shui
-
Yuxuan Shui (＠yshui)