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

[Derek Foreman]

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?