Re: [Mesa-dev] [PATCH v2] egl/wayland: Avoid race conditions when on non-main thread

[Daniel Stone]

Hi Jonas,

On 24 June 2016 at 03:46, Jonas Ådahl  wrote:
> @@ -74,9 +74,8 @@ roundtrip(struct dri2_egl_display *dri2_dpy)
> struct wl_callback *callback;
> int done = 0, ret = 0;
>
> -   callback = wl_display_sync(dri2_dpy->wl_dpy);
> +   callback = wl_display_sync(dri2_dpy->wl_dpy_wrapper);
> wl_callback_add_listener(callback, _listener, );
> -   wl_proxy_set_queue((struct wl_proxy *) callback, dri2_dpy->wl_queue);
> while (ret != -1 && !done)
>ret = wl_display_dispatch_queue(dri2_dpy->wl_dpy, dri2_dpy->wl_queue);

If we're bumping the libwayland-client dep anyway, might as well just
make this use wl_display_roundtrip_queue.

> @@ -1235,6 +1237,10 @@ dri2_initialize_wayland_drm(_EGLDriver *drv, 
> _EGLDisplay *disp)
> wl_drm_destroy(dri2_dpy->wl_drm);
>   cleanup_registry:
> wl_registry_destroy(dri2_dpy->wl_registry);
> +   wl_proxy_wrapper_destroy(dri2_dpy->wl_dpy_wrapper);
> + cleanup_dpy_wrapper:
> +   if (dri2_dpy->wl_dpy != disp->PlatformDisplay)
> +  wl_display_disconnect(dri2_dpy->wl_dpy);
> wl_event_queue_destroy(dri2_dpy->wl_queue);
>   cleanup_dpy:
> free(dri2_dpy);

Adding wl_display_disconnect should be a separate change, and in any
case needs to come after wl_event_queue_destroy to avoid a
use-after-free.

With those fixed, assuming we're fine with an increased hard dep:
Reviewed-by: Daniel Stone 

Cheers,
Daniel
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2] egl/wayland: Avoid race conditions when on non-main thread

[Emil Velikov]

[Cc-ing Daniel/Pekka]

Hi all,

On 24 June 2016 at 03:46, Jonas Ådahl  wrote:
> When EGL is used on some other thread than the thread that drives the
> main wl_display queue, the Wayland EGL dri2 implementation is
> vulnerable to a race condition related to display round trips and global
> object advertisements.
>
> The race that may happen is that after after a proxy is created, but
> before the queue is set, events meant to be emitted via the yet to be
> set queue may already have been queued on the wrong queue.
>
> In order to make it possible to avoid this race, wayland 1.11
> introduced new API that allows creating a proxy wrapper that may be used
> as the factory proxy when creating new proxies via Wayland requests. The
> queue of a proxy wrapper can be changed without effecting what queue
> events emitted by the actual proxy will be queued on, while still
> effecting what default queue proxies created from it will have.
>
> By introducing a wl_display proxy wrapper and using this when performing
> round trips (via wl_display_sync()) and retrieving the global objects (via
> wl_display_get_registry()), the mentioned race condition is avoided.
>
> Signed-off-by: Jonas Ådahl 
> ---
>
> Changes since v1:
>
>  - Added proxy clean up on tear down
>  - Changed required version to the stable wayland release (1.11)
>
>
>  configure.ac|  2 +-
>  src/egl/drivers/dri2/egl_dri2.c |  1 +
>  src/egl/drivers/dri2/egl_dri2.h |  1 +
>  src/egl/drivers/dri2/platform_wayland.c | 30 --
>  4 files changed, 19 insertions(+), 15 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index 023110e..d73af83 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -83,7 +83,7 @@ GLPROTO_REQUIRED=1.4.14
>  LIBOMXIL_BELLAGIO_REQUIRED=0.0
>  LIBVA_REQUIRED=0.38.0
>  VDPAU_REQUIRED=1.1
> -WAYLAND_REQUIRED=1.2.0
> +WAYLAND_REQUIRED=1.11
>  XCB_REQUIRED=1.9.3
>  XCBDRI2_REQUIRED=1.8
>  XCBGLX_REQUIRED=1.8.1
> diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
> index d8448f4..88191b4 100644
> --- a/src/egl/drivers/dri2/egl_dri2.c
> +++ b/src/egl/drivers/dri2/egl_dri2.c
> @@ -849,6 +849,7 @@ dri2_terminate(_EGLDriver *drv, _EGLDisplay *disp)
>wl_shm_destroy(dri2_dpy->wl_shm);
>wl_registry_destroy(dri2_dpy->wl_registry);
>wl_event_queue_destroy(dri2_dpy->wl_queue);
> +  wl_proxy_wrapper_destroy(dri2_dpy->wl_dpy_wrapper);
>if (dri2_dpy->own_device) {
>   wl_display_disconnect(dri2_dpy->wl_dpy);
>}
> diff --git a/src/egl/drivers/dri2/egl_dri2.h b/src/egl/drivers/dri2/egl_dri2.h
> index ddb5f39..075c2a4 100644
> --- a/src/egl/drivers/dri2/egl_dri2.h
> +++ b/src/egl/drivers/dri2/egl_dri2.h
> @@ -205,6 +205,7 @@ struct dri2_egl_display
>
>  #ifdef HAVE_WAYLAND_PLATFORM
> struct wl_display*wl_dpy;
> +   struct wl_display*wl_dpy_wrapper;
> struct wl_registry   *wl_registry;
> struct wl_drm*wl_server_drm;
> struct wl_drm*wl_drm;
> diff --git a/src/egl/drivers/dri2/platform_wayland.c 
> b/src/egl/drivers/dri2/platform_wayland.c
> index ff0d5c8..519469e 100644
> --- a/src/egl/drivers/dri2/platform_wayland.c
> +++ b/src/egl/drivers/dri2/platform_wayland.c
> @@ -74,9 +74,8 @@ roundtrip(struct dri2_egl_display *dri2_dpy)
> struct wl_callback *callback;
> int done = 0, ret = 0;
>
> -   callback = wl_display_sync(dri2_dpy->wl_dpy);
> +   callback = wl_display_sync(dri2_dpy->wl_dpy_wrapper);
> wl_callback_add_listener(callback, _listener, );
> -   wl_proxy_set_queue((struct wl_proxy *) callback, dri2_dpy->wl_queue);
> while (ret != -1 && !done)
>ret = wl_display_dispatch_queue(dri2_dpy->wl_dpy, dri2_dpy->wl_queue);
>
> @@ -765,11 +764,9 @@ dri2_wl_swap_buffers_with_damage(_EGLDriver *drv,
>  * handle the commit and send a release event before checking for a free
>  * buffer */
> if (dri2_surf->throttle_callback == NULL) {
> -  dri2_surf->throttle_callback = wl_display_sync(dri2_dpy->wl_dpy);
> +  dri2_surf->throttle_callback = 
> wl_display_sync(dri2_dpy->wl_dpy_wrapper);
>wl_callback_add_listener(dri2_surf->throttle_callback,
> _listener, dri2_surf);
> -  wl_proxy_set_queue((struct wl_proxy *) dri2_surf->throttle_callback,
> - dri2_dpy->wl_queue);
> }
>
> wl_display_flush(dri2_dpy->wl_dpy);
> @@ -1093,12 +1090,17 @@ dri2_initialize_wayland_drm(_EGLDriver *drv, 
> _EGLDisplay *disp)
>
> dri2_dpy->wl_queue = wl_display_create_queue(dri2_dpy->wl_dpy);
>
> +   dri2_dpy->wl_dpy_wrapper = wl_proxy_create_wrapper(dri2_dpy->wl_dpy);
> +   if (dri2_dpy->wl_dpy_wrapper == NULL)
> +  goto cleanup_dpy_wrapper;
> +
> +   wl_proxy_set_queue((struct wl_proxy *) dri2_dpy->wl_dpy_wrapper,
> +  dri2_dpy->wl_queue);
> +
> if (dri2_dpy->own_device)
>