[Bug 58325] New: wined3d swapchain use-after-free
http://bugs.winehq.org/show_bug.cgi?id=58325 Bug ID: 58325 Summary: wined3d swapchain use-after-free Product: Wine Version: unspecified Hardware: x86-64 OS: Linux Status: UNCONFIRMED Severity: normal Priority: P2 Component: d3d Assignee: wine-bugs(a)winehq.org Reporter: yshuiv7(a)gmail.com Distribution: --- Releasing the swapchain brings us to wined3d_swapchain_cleanup, which destroys the front/back buffer textures. Texture destruction is async, happening on the device's command stream. This means these textures can be destroyed way after the swapchain itself. But we did set `texture->swapchain` to `NULL` before queuing their destruction, so that bit is fine. The problem is in texture_resource_unload. Here we try to acquire the context, which goes adapter_gl_acquire_context -> wined3d_context_gl_acquire -> wined3d_context_gl_activate -> wined3d_context_gl_update_window. And here we try to access `context_gl->c.swapchain->win_handle`, which unfortunately points to the swapchain that was just freed. I am unsure what's the fix for this. Do we hold a refcount of the swapchain in the context? Do we clear `context->swapchain` when a swapchain is being freed? Should we release the front/back buffer synchronously? Or something else? -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=58325 Bernhard Übelacker <bernhardu(a)mailbox.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |bernhardu(a)mailbox.org --- Comment #1 from Bernhard Übelacker <bernhardu(a)mailbox.org> --- Just in case this is of any help, this is the (regular) ASan report to it: https://gitlab.winehq.org/bernhardu/wine/-/blob/5c01131e61811f903f9857404319... -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=58325 --- Comment #2 from Bernhard Übelacker <bernhardu@mailbox.org> --- After seeing you closed your merge request [1], I tried to drop it it in my gitlab runs, but unfortunately it still pops out here [2]. [1] https://gitlab.winehq.org/wine/wine/-/merge_requests/8406 [2] https://gitlab.winehq.org/bernhardu/wine/-/jobs/230557#L4216 -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=58325 --- Comment #3 from Yuxuan Shui <yshuiv7@gmail.com> --- whoops, i guess i closed it prematurely. i haven't looked at it for a while. i still don't know the mechanism of this bug, my previous explanations were shot down by Zeb and she was right. must be some convoluted race conditions... -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=58325 --- Comment #4 from Yuxuan Shui <yshuiv7@gmail.com> --- i just tried it again and didn't see this anymore, do you see the same? -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=58325 Stian Low <wineryyyyy@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |wineryyyyy@gmail.com --- Comment #5 from Stian Low <wineryyyyy@gmail.com> --- (In reply to Yuxuan Shui from comment #4)
i just tried it again and didn't see this anymore, do you see the same?
Race condition behavior seemed to sometimes cause swapchain tests for d3d10/11 to lockup/crash for wine-11-11: https://gitlab.winehq.org/wine/wine/-/merge_requests/10567/#note_143871 Adding Sleep() to some parts of wined3d exposed race conditions. I plan to add tests to reproduce and id wined3d parts which may need extra safety mechanisms. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=58325 --- Comment #6 from Bernhard Übelacker <bernhardu@mailbox.org> --- (In reply to Yuxuan Shui from comment #4)
i just tried it again and didn't see this anymore, do you see the same?
I am still carrying your patch from [1] in my ASan gitlab pipeline. For this nights run I reverted this patch and received multiple ASan reports in this pipeline [2] in wined3d_context_gl_update_window for following tests: dxgi:dxgi d3d11:d3d11 d3d10core:d3d10core d2d1:d2d1 So yes, I still see it on my end. [1] https://gitlab.winehq.org/wine/wine/-/merge_requests/8406 [2] https://gitlab.winehq.org/bernhardu/wine/-/pipelines/69296 -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=58325 --- Comment #7 from Stian Low <wineryyyyy@gmail.com> --- d3d10/11 tests still intermittently hung and had to be killed which I suspect is the reason why pipeline #70444 passed by #71395 failed despite no changes between them: https://gitlab.winehq.org/wine/wine/-/merge_requests/10567/pipelines Running #71401 again without any changes from last 2 to produce another pass. I still need to reproduce without any of my commits to confirm. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=58325 --- Comment #8 from Stian Low <wineryyyyy@gmail.com> --- (In reply to Stian Low from comment #7)
Running #71401 again without any changes from last 2 to produce another pass.
Confirming #71401 passes inconsistent with prior #71395 which failed despite no changes made between the two. I'll try to reproduce for master without any of my changes which may help confirm/clarify d3d10/11 still has bugs that cause intermittent test failures. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=58325 --- Comment #9 from Bernhard Übelacker <bernhardu@mailbox.org> --- (In reply to Stian Low from comment #8)
(In reply to Stian Low from comment #7)
Running #71401 again without any changes from last 2 to produce another pass.
Confirming #71401 passes inconsistent with prior #71395 which failed despite no changes made between the two.
I'll try to reproduce for master without any of my changes which may help confirm/clarify d3d10/11 still has bugs that cause intermittent test failures.
Hello, unfortunately I only see the flaky user32:win test failing in pipeline 71395, which seems unrelated to wined3d? ``` win.c:10689:0.183 Test failed: transparent window didn't get WM_NCHITTEST message win.c:10690:0.183 Test failed: button under static window didn't get WM_LBUTTONUP user32:win:0e30 done (2) in 51s 17606B `` These can can be also observed in the test pattern page for the linux_gitlab lines: https://test.winehq.org/data/patterns.html#user32:win -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=58325 --- Comment #10 from Stian Low <wineryyyyy@gmail.com> --- (In reply to Bernhard Übelacker from comment #9)
Hello, unfortunately I only see the flaky user32:win test failing in pipeline 71395, which seems unrelated to wined3d?
Thanks Bernhard for taking a look and adding some clarity to the test fail logs. user32:win indeed seems unrelated to d3d10/11 tests that I touched so maybe d3d is not the cause afterall? d3d10/11 tests still intermittently lockup/hang occasionally for my local tests which requires killing the test processes and rerun which usually then passes. Still unclear what causes tests lockups/hangs but maybe it's user32:win related which d3d tests may depend upon at a deeper level than the files I touched. user32:win tests that fail seem to be handled gracefully with test results vs local d3d test lockups/hangs which fails to ever produce test results. I was less familiar with how tests were handled on Gitlab so your insights helped. MR-10567 heavily deals with d3d swapchains and tests seemed buggy so I figured it may be related to this bug. Maybe the hangs for my local tests are still related but it seems the intermittent server side tests failing may be due to flakiness for user32:win tests which may be expected as you suggest. I'll keep debugging to determine if tests failing locally are still related to this bug. Otherwise I may be confusing it with coincidence. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=58325 --- Comment #11 from Stian Low <wineryyyyy@gmail.com> --- (In reply to Stian Low from comment #10)
I'll keep debugging to determine if tests failing locally are still related to this bug. Otherwise I may be confusing it with coincidence.
I may have been running new tests too quickly after priors because after retesting more carefully and waiting for prior test processes to close before starting new tests, hangs previously reported seem mostly resolved. The time to close the processes is negligible so I just need to remember to wait a few seconds longer between successive tests. Therefore my reports of d3d tests hanging is probably invalid and I was confusing expected behavior when running overlapping tests for potential bugs. If anything else happens beyond this finding related to d3d/swapchains/race conditions then I'll report it. Otherwise all seems resolved for my part for now. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=58325 --- Comment #12 from Stian Low <wineryyyyy@gmail.com> --- Seems duplicate: - https://bugs.winehq.org/show_bug.cgi?id=58657 -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=58325 --- Comment #13 from Stian Low <wineryyyyy@gmail.com> --- Created attachment 81760 --> http://bugs.winehq.org/attachment.cgi?id=81760 Patch may reproduce swapchain use-after-free for dlls/d3d11/tests/d3d11.c:test_swapchain_flip Please let me know if this patch also causes test_swapchain_flip() to crash for dlls/d3d11/tests/d3d11.c Patch was extract as part of MR-10567. Crash may specific to tests because apps like Frostpunk are fixed via MR-10567 and do not reproduce crashes. WINE_D3D_CONFIG=csmt=0x1 vs csmt=0x2 may crash at different points. csmt=0x2 crash seems to be fixed by moving `struct list entry;` as first vs last member of `struct wined3d_rendertarget_view`: modified dlls/wined3d/wined3d_private.h @@ -3962,6 +3962,7 @@ HRESULT wined3d_buffer_no3d_init(struct wined3d_buffer *buffer_no3d, struct wine struct wined3d_rendertarget_view { + struct list entry; csmt=0x1 crash seems fixed by removing the list handling for `wined3d_rendertarget_view_decref` modified dlls/wined3d/view.c @@ -421,6 +421,21 @@ ULONG CDECL wined3d_rendertarget_view_decref(struct wined3d_rendertarget_view *v /* Release the resource after destroying the view. * See wined3d_shader_resource_view_decref(). */ wined3d_mutex_lock(); +#if 000 + if (resource->type == WINED3D_RTYPE_TEXTURE_2D && texture_from_resource(resource)->swapchain) + { + struct wined3d_rendertarget_view *swap_view; + LIST_FOR_EACH_ENTRY(swap_view, &texture_from_resource(resource)->swapchain->back_buffer_rendertarget_views, struct wined3d_rendertarget_view, entry) + { + if (swap_view != view) + continue; + + list_remove(&swap_view->entry); + free(swap_view); + break; + } + } +#endif resource->device->adapter->adapter_ops->adapter_destroy_rendertarget_view(view); wined3d_mutex_unlock(); wined3d_resource_decref(resource); I'm assuming crash is more related to d3d and perhaps specifically to this swapchain bug vs struct list in general which explicitly states it may not be first member which is the case ubiquitously across many critial systemic components: ./include/wine/list.h: /* Define a list like so: * * struct gadget * { * struct list entry; <-- doesn't have to be the first item in the struct * int a, b; * }; Examples of struct list not first members of structs: - ./server/device.c:46: struct list dev_entry; /* entry in device queue */ - ./server/registry.c:95: struct list notify_list; /* list of notifications */ - ./server/mapping.c:78: struct list entry; /* entry in global shared maps list */ - ./server/user.h:44: struct list desktops; /* list of desktops of this winstation */ - ./server/process.h:42: struct list thread_list; /* thread list */ Despite MR-10567 fixing long standing dark rendering bugs, passing wine tests has proven far less trivial than the original hack. Whatever is causing tests to crash for attached path may be the last remaining issue to allow MR-10567 to finally be approved for merge by passing all tests. Perhaps multiple iterations of separate merge requests is a better method for the future vs so many iterations per single merge request which has resulted in an over-bloated mess. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=58325 --- Comment #14 from Stian Low <wineryyyyy@gmail.com> --- (In reply to Stian Low from comment #13)
Created attachment 81760 [details] Patch may reproduce swapchain use-after-free for + free(swap_view);
Actually patch seems to have a bug for `free(swap_view)` left over from previous iteration which performed a free on a wrapper of swap_view vs wined3d_rendertarget_view directly so that seems likely the cause of the patch failing probably explains the odd inconsistencies between csmt=0x1 vs 0x2. I'll keep testing and report any other issues but this one seems probably resolved and may be ignored. Apologies for noise. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
participants (2)
-
WineHQ Bugzilla -
WineHQ Bugzilla