[PATCH v2] winex11.drv: Destroy windows associated with X11 display when the display is closed.
Paul Gofman
pgofman at codeweavers.com
Thu Oct 14 09:29:52 CDT 2021
On 10/14/21 17:12, Rémi Bernon wrote:
> On 10/14/21 4:10 PM, Paul Gofman wrote:
>> On 10/14/21 17:04, Rémi Bernon wrote:
>>> On 10/14/21 3:56 PM, Paul Gofman wrote:
>>>> Signed-off-by: Paul Gofman <pgofman at codeweavers.com>
>>>> ---
>>>> Supersedes 217174.
>>>>
>>>> X11DRV_ThreadDetach() destroys thread data part of which
>>>> (e. g., display) is still present in window data in winex11.drv
>>>> and accessible through hwnd. That causes all sort of hangs
>>>> and crashes when, for instance, the window is still used for
>>>> Vulkan
>>>> rendering (even if only to tear down the device and swapchain).
>>>>
>>>> dlls/winex11.drv/window.c | 66
>>>> ++++++++++++++++++++++++++++++++++
>>>> dlls/winex11.drv/x11drv.h | 6 ++++
>>>> dlls/winex11.drv/x11drv_main.c | 3 ++
>>>> 3 files changed, 75 insertions(+)
>>>>
>>>
>>> I think the right way to fix this is to fix the user32 window leaks,
>>> as complicated and intricated as it may be. Adding another internal
>>> thread window list just makes things even more complicated.
>>
>> My reasoning under this variant of fixing the present problem is that
>> if winex11.drv can delete the display which is still used by winex11
>> window data, it is a sort of use after free winex11 bug on its own
>> and not necessarily related to what user32 is doing. I am not sure
>> winemac.drv has the same problem. Relying on the proper driver
>> windows cleanup in winex11.drv from user32 side to avoid use after
>> free doesn't seem right by itself.
>>
>> Apart from that, do you have any plans for resending your child
>> window deletion race fixing patches? If fixing the present issue from
>> user32 side will be deemed the only correct way that will probably
>> depend on those.
>>
>
> Sure, I'll try to clean them up and have another try. Having another
> pair of eyes on them might help too :)
Also please note that X11DRV_ThreadDetach() closes the thread's display,
which means that all the windows created with this display get destroyed
anyway and we only have leftover x11drv_win_data structures referencing
non-existent X11 objects after that. It is probably not the case in
winemac driver. I am not sure anymore that explicitly destroying the
windows on the driver side from user32 before exiting thread is
necessarily assumed valid usage.
More information about the wine-devel
mailing list