[PATCH 0/1] MR7690: winewayland: Don't use a destroyed surface in text input.
Don't assert or get the user data of null surface arguments. When the callbacks are run with a null surface it means the surface was destroyed client-side but the compositor has yet to process the destroy request and has sent events referencing the surface which no longer exists on the client. The wl_surface pointer also needs to be removed during wayland_surface_destroy to prevent it from becoming a dangling pointer which may be used if text_input_done or text_input_leave is run after the surface is destroyed. While doing so, track the hwnd instead to be consistent with keyboard and pointer. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7690
From: Attila Fidan <dev(a)print0.net> Don't assert or get the user data of null surface arguments. When the callbacks are run with a null surface it means the surface was destroyed client-side but the compositor has yet to process the destroy request and has sent events referencing the surface which no longer exists on the client. The wl_surface pointer also needs to be removed during wayland_surface_destroy to prevent it from becoming a dangling pointer which may be used if text_input_done or text_input_leave is run after the surface is destroyed. While doing so, track the hwnd instead to be consistent with keyboard and pointer. --- dlls/winewayland.drv/wayland_surface.c | 5 ++++ dlls/winewayland.drv/wayland_text_input.c | 35 ++++++++++++----------- dlls/winewayland.drv/waylanddrv.h | 2 +- 3 files changed, 25 insertions(+), 17 deletions(-) diff --git a/dlls/winewayland.drv/wayland_surface.c b/dlls/winewayland.drv/wayland_surface.c index 2178f5431cb..701085847a4 100644 --- a/dlls/winewayland.drv/wayland_surface.c +++ b/dlls/winewayland.drv/wayland_surface.c @@ -204,6 +204,11 @@ void wayland_surface_destroy(struct wayland_surface *surface) process_wayland.keyboard.focused_hwnd = NULL; pthread_mutex_unlock(&process_wayland.keyboard.mutex); + pthread_mutex_lock(&process_wayland.text_input.mutex); + if (process_wayland.text_input.focused_hwnd == surface->hwnd) + process_wayland.text_input.focused_hwnd = NULL; + pthread_mutex_unlock(&process_wayland.text_input.mutex); + wayland_surface_clear_role(surface); if (surface->wp_viewport) diff --git a/dlls/winewayland.drv/wayland_text_input.c b/dlls/winewayland.drv/wayland_text_input.c index e0181eb8240..f8df5fe09cb 100644 --- a/dlls/winewayland.drv/wayland_text_input.c +++ b/dlls/winewayland.drv/wayland_text_input.c @@ -61,16 +61,21 @@ static void text_input_enter(void *data, struct zwp_text_input_v3 *zwp_text_inpu struct wl_surface *surface) { struct wayland_text_input *text_input = data; - TRACE("data %p, text_input %p, surface %p.\n", data, zwp_text_input_v3, surface); + HWND hwnd; + + if (!surface) return; + + hwnd = wl_surface_get_user_data(surface); + TRACE("data %p, text_input %p, hwnd %p.\n", data, zwp_text_input_v3, hwnd); pthread_mutex_lock(&text_input->mutex); + text_input->focused_hwnd = hwnd; zwp_text_input_v3_enable(text_input->zwp_text_input_v3); zwp_text_input_v3_set_content_type(text_input->zwp_text_input_v3, ZWP_TEXT_INPUT_V3_CONTENT_HINT_NONE, ZWP_TEXT_INPUT_V3_CONTENT_PURPOSE_NORMAL); zwp_text_input_v3_set_cursor_rectangle(text_input->zwp_text_input_v3, 0, 0, 0, 0); zwp_text_input_v3_commit(text_input->zwp_text_input_v3); - text_input->wl_surface = surface; pthread_mutex_unlock(&text_input->mutex); } @@ -78,16 +83,16 @@ static void text_input_leave(void *data, struct zwp_text_input_v3 *zwp_text_inpu struct wl_surface *surface) { struct wayland_text_input *text_input = data; - HWND hwnd; - TRACE("data %p, text_input %p, surface %p.\n", data, zwp_text_input_v3, surface); + TRACE("data %p, text_input %p.\n", data, zwp_text_input_v3); pthread_mutex_lock(&text_input->mutex); zwp_text_input_v3_disable(text_input->zwp_text_input_v3); zwp_text_input_v3_commit(text_input->zwp_text_input_v3); - assert(text_input->wl_surface); - hwnd = wl_surface_get_user_data(text_input->wl_surface); - post_ime_update(hwnd, 0, NULL, NULL); - text_input->wl_surface = NULL; + if (text_input->focused_hwnd) + { + post_ime_update(text_input->focused_hwnd, 0, NULL, NULL); + text_input->focused_hwnd = NULL; + } pthread_mutex_unlock(&text_input->mutex); } @@ -127,18 +132,16 @@ static void text_input_done(void *data, struct zwp_text_input_v3 *zwp_text_input uint32_t serial) { struct wayland_text_input *text_input = data; - HWND hwnd; TRACE("data %p, text_input %p, serial %u.\n", data, zwp_text_input_v3, serial); pthread_mutex_lock(&text_input->mutex); /* Some compositors will send a done event for every commit, regardless of * the focus state of the text input. This behavior is arguably out of spec, * but otherwise harmless, so just ignore the new state in such cases. */ - if (text_input->wl_surface) + if (text_input->focused_hwnd) { - hwnd = wl_surface_get_user_data(text_input->wl_surface); - post_ime_update(hwnd, text_input->preedit_cursor_pos, text_input->preedit_string, - text_input->commit_string); + post_ime_update(text_input->focused_hwnd, text_input->preedit_cursor_pos, + text_input->preedit_string, text_input->commit_string); } free(text_input->preedit_string); @@ -177,7 +180,7 @@ void wayland_text_input_deinit(void) pthread_mutex_lock(&text_input->mutex); zwp_text_input_v3_destroy(text_input->zwp_text_input_v3); text_input->zwp_text_input_v3 = NULL; - text_input->wl_surface = NULL; + text_input->focused_hwnd = NULL; pthread_mutex_unlock(&text_input->mutex); }; @@ -194,13 +197,13 @@ BOOL WAYLAND_SetIMECompositionRect(HWND hwnd, RECT rect) pthread_mutex_lock(&text_input->mutex); - if (!text_input->zwp_text_input_v3) + if (!text_input->zwp_text_input_v3 || hwnd != text_input->focused_hwnd) goto err; if (!(data = wayland_win_data_get(hwnd))) goto err; - if (!(surface = data->wayland_surface) || surface->wl_surface != text_input->wl_surface) + if (!(surface = data->wayland_surface)) { wayland_win_data_release(data); goto err; diff --git a/dlls/winewayland.drv/waylanddrv.h b/dlls/winewayland.drv/waylanddrv.h index 065d4d31873..ba1e715f38e 100644 --- a/dlls/winewayland.drv/waylanddrv.h +++ b/dlls/winewayland.drv/waylanddrv.h @@ -120,7 +120,7 @@ struct wayland_text_input WCHAR *preedit_string; DWORD preedit_cursor_pos; WCHAR *commit_string; - struct wl_surface *wl_surface; + HWND focused_hwnd; pthread_mutex_t mutex; }; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7690
This merge request was approved by Alexandros Frantzis. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7690
This merge request was approved by Alexandros Frantzis. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7690
Alexandros Frantzis (@afrantzis) commented about dlls/winewayland.drv/wayland_text_input.c:
pthread_mutex_lock(&text_input->mutex);
- if (!text_input->zwp_text_input_v3) + if (!text_input->zwp_text_input_v3 || hwnd != text_input->focused_hwnd) goto err;
if (!(data = wayland_win_data_get(hwnd)))
Not blocking for this MR: Although not a problem at the moment (AFAICT), we try not to acquire the `win_data_mutex` while holding other mutexes (keyboard, pointer etc), to avoid potential deadlocks in the future. We can avoid the nested locking in this case too, by first performing the cursor transforms using the provided hwnd and then locking text_input and checking whether it matches the hwnd. In the worst case we will have performed an unnecessary transform. If we find that there is some performance implication we can do a quick lock/check before the transforms, or also check `process_wayland.zwp_text_input_manager_v3` which doesn't need a lock. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7690#note_99369
participants (3)
-
Alexandros Frantzis (＠afrantzis) -
Attila Fidan
-
Attila Fidan (＠atticf)