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

[Pekka Paalanen]

On Fri, 29 May 2015 09:49:52 -0500
Derek Foreman <derekf at osg.samsung.com> wrote:

> On 29/05/15 04:02 AM, Pekka Paalanen wrote:
> > 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.
> 
> Fair enough - how about if I just drop the shell client out of the
> linked list at this point instead?  I originally wrote it that way but
> it felt like a leak. :/
> 
> Then the rest of the clean up will proceed normally and nothing will try
> to frob the freed linked list node from struct shell_client.
> 
> The reason the destruction order matters here is that the surfaces are
> all in a linked list with the shell client at the head of it.  I'm not
> really sure it's an indication of wide scale object lifetime mismanagement?

Ah, it's that kind of case.

Dropping the "head" from the list sounds fine to me, with a big
comment explaining what is happening. It is equivalent to removing
everything from the list while keeping the links still removable
for clean-up, except it's very cheap.


Thanks,
pq