GNOME Bugzilla – Bug 770992
wayland/cursor-role: Handle premature wl_buffer destruction
Last modified: 2016-09-09 03:10:48 UTC
Broken/malicious clients can potentially destroy an attached wl_buffer before calling wl_pointer.set_cursor. This would cause mutter to crash. Xwayland seem to do this (which seems wrong but thats a separate issue), or at least I hit the crash when doing things with an X11 client connecting via Xwayland. The attached patch makes mutter handle this more gracefully.
Created attachment 334964 [details] [review] wayland/cursor-role: Handle premature wl_buffer destruction If a client would attach a buffer to a surface, commit, and then later set the surface as a cursor, there will be no wl_buffer available to be used by the cursor role. Instead of dereferencing the non-existing wl_buffer resource, handle this situation by logging a warning and treating a prematurely destroyd wl_buffer as if no buffer had been attached.
Review of attachment 334964 [details] [review]: LGTM
So, basically this is about clients that are violating the protocol by destroying a wl_buffer before getting a release event, right? (In reply to Jonas Ådahl from comment #1) > If a client would attach a buffer to a surface, commit, and then later This description is missing the crucial step, i.e. the client destroys the wl_buffer after committing and before setting the surface as a cursor. > set the surface as a cursor, there will be no wl_buffer available to be > used by the cursor role. Instead of dereferencing the non-existing > wl_buffer resource, handle this situation by logging a warning and > treating a prematurely destroyd wl_buffer as if no buffer had been > attached. I'm afraid we can also crash in meta_cursor_renderer_realize_cursor_from_wl_buffer() if a client calls wl_pointer.set_cursor() with a surface that was already being used as a cursor and, again, after destroying a committed and not yet released wl_buffer.
(In reply to Rui Matos from comment #3) > So, basically this is about clients that are violating the protocol by > destroying a wl_buffer before getting a release event, right? Yepp. > > (In reply to Jonas Ådahl from comment #1) > > If a client would attach a buffer to a surface, commit, and then later > > This description is missing the crucial step, i.e. the client destroys the > wl_buffer after committing and before setting the surface as a cursor. Ah, true. I'll amend. > > > set the surface as a cursor, there will be no wl_buffer available to be > > used by the cursor role. Instead of dereferencing the non-existing > > wl_buffer resource, handle this situation by logging a warning and > > treating a prematurely destroyd wl_buffer as if no buffer had been > > attached. > > I'm afraid we can also crash in > meta_cursor_renderer_realize_cursor_from_wl_buffer() if a client calls > wl_pointer.set_cursor() with a surface that was already being used as a > cursor and, again, after destroying a committed and not yet released > wl_buffer. I don't think we would. We only take that path if priv->buffer is set, and priv->buffer is only set (at wl_pointer.set_cursor) if buffer && buffer->resource (with this patch applied). Right?
I think it can happen with these steps: surface.attach(bufferA) surface.commit() pointer.set_cursor(surface, hotspotA) - surface gets a new cursor role instance assigned surface.attach(bufferB) surface.commit() bufferB.destroy() pointer.set_cursor(surface, hotspotB) - the existing cursor role instance is re-used meta_wayland_surface_role_cursor_set_hotspot() gets called, changing the hotspot and we end up in update_cursor_sprite_texture() with priv->buffer->resource being NULL.
(In reply to Rui Matos from comment #5) > I think it can happen with these steps: > > surface.attach(bufferA) > surface.commit() > pointer.set_cursor(surface, hotspotA) - surface gets a new cursor role > instance assigned First in constructed() early in the wl_pointer.set_cursor handler (when initially assigned): priv->buffer == bufferA Then later, still in the wl_pointer.set_cursor handler: cursor_role.set_renderer(...) which will indirectly call .._realize_..() eventually resulting in priv->buffer == NULL > > surface.attach(bufferB) > surface.commit() Will first result in priv->buffer == bufferB then .._realize_..() (late in the role commit handler before destroy) ending up with priv->buffer == NULL. > bufferB.destroy() > pointer.set_cursor(surface, hotspotB) - the existing cursor role instance is > re-used priv->buffer will already be NULL here (since it was realized at commit already) > > meta_wayland_surface_role_cursor_set_hotspot() gets called, changing the > hotspot and we end up in update_cursor_sprite_texture() with > priv->buffer->resource being NULL. Since (if I'm mentally executing the code correctly) priv->buffer will be NULL here, we won't access the resource. Does the above make any sense? I think we should handle that series of messages as well already.
(In reply to Jonas Ådahl from comment #6) > > surface.attach(bufferB) > > surface.commit() > > Will first result in > > priv->buffer == bufferB > > then > > .._realize_..() (late in the role commit handler before destroy) > > ending up with > > priv->buffer == NULL. Aha, you're right, this is what I was missing. Ok, so that path is fine.
Attachment 334964 [details] pushed as 640178a - wayland/cursor-role: Handle premature wl_buffer destruction