[PATCH weston] desktop-shell: destroy shell client surfaces in handle_shell_client_destroy

[Pekka Paalanen]

On Tue, 26 May 2015 14:24:37 -0500
Derek Foreman <derekf at osg.samsung.com> wrote:

> This prevents a use after free when the surfaces are automatically cleaned
> up later, as shell_client's entry was still in the surface list.
> 
> Signed-off-by: Derek Foreman <derekf at osg.samsung.com>
> ---
> 
> I'm not really well versed in this, so would appreciate extra critical
> review here, as this trivial patch may be trivially wrong. :)
> 
> This stops a use after free (as detected by valgrind) when closing, for
> example, weston-terminal.
> 
>  desktop-shell/shell.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
> index fe620cb..b1bba44 100644
> --- a/desktop-shell/shell.c
> +++ b/desktop-shell/shell.c
> @@ -5817,11 +5817,16 @@ launch_desktop_shell_process(void *data)
>  static void
>  handle_shell_client_destroy(struct wl_listener *listener, void *data)
>  {
> +	struct wl_resource *shsurf_resource, *next;
>  	struct shell_client *sc =
>  		container_of(listener, struct shell_client, destroy_listener);
>  
>  	if (sc->ping_timer)
>  		wl_event_source_remove(sc->ping_timer);
> +
> +	wl_resource_for_each_safe(shsurf_resource, next, &sc->surface_list)
> +		wl_resource_destroy(shsurf_resource);
> +
>  	free(sc);
>  }
>  

Yeah, you cannot go calling wl_resource_destroy() unless the protocol
specification explicitly states that the protocol object gets destroyed.

However, this is a client destroy handler, which means no requests from
this client will ever be processed anymore. In that sense this is safe.
Client destroy handlers get called before the client resources are
destroyed, that's why this works.

I think this patch is a hack, and may hide actual bugs. Such
use-after-free problems imply that something is not tracking object
lifetimes properly. There is a good chance client tear-down is not the
only case where the bug is hit. Therefore it would be much better to
fix the object tracking to deal with arbitrary wl_resource destruction
order, even if the protocol spec says a particular destruction order is
illegal for a client to do.

That's why: NAK.


Thanks,
pq