[PATCH wayland v4 05/11] client: Use refcount exclusively for destruction

[Derek Foreman]

On 2017-12-28 01:53 PM, Daniel Stone wrote:
> Commit e273c7cde added a refcount to wl_proxy. The refcount is set to 1
> on creation, decreased when the client explicitly destroys the proxy,
> and is increased and decreased every time an event referencing that
> proxy is queued.
> 
> Assuming no bugs, this means the refcount cannot reach 0 without the
> proxy being explicitly destroyed. However, some (not all) of the
> proxy-unref paths were only destroying the proxy if it had already been
> deleted. This should already be enforced by refcounting, so remove the
> check and rely solely on the refcount as the arbiter of when to free a
> proxy.
> 
> Signed-off-by: Daniel Stone <daniels at collabora.com>
> Cc: Jonas Ådahl <jadahl at gmail.com>
> Cc: Derek Foreman <derekf at osg.samsung.com>
> ---
>   src/wayland-client.c | 11 ++++-------
>   1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/src/wayland-client.c b/src/wayland-client.c
> index 21ef5b04..55838cd9 100644
> --- a/src/wayland-client.c
> +++ b/src/wayland-client.c
> @@ -258,7 +258,6 @@ wl_event_queue_release(struct wl_event_queue *queue)
>   {
>   	struct wl_closure *closure;
>   	struct wl_proxy *proxy;
> -	bool proxy_destroyed;
>   
>   	while (!wl_list_empty(&queue->event_list)) {
>   		closure = container_of(queue->event_list.next,
> @@ -268,10 +267,8 @@ wl_event_queue_release(struct wl_event_queue *queue)
>   		decrease_closure_args_refcount(closure);
>   
>   		proxy = closure->proxy;
> -		proxy_destroyed = !!(proxy->flags & WL_PROXY_FLAG_DESTROYED);
> -
>   		proxy->refcount--;
> -		if (proxy_destroyed && !proxy->refcount)
> +		if (!proxy->refcount)
>   			free(proxy);
>   
>   		wl_closure_destroy(closure);
> @@ -1310,10 +1307,10 @@ dispatch_event(struct wl_display *display, struct wl_event_queue *queue)
>   	proxy_destroyed = !!(proxy->flags & WL_PROXY_FLAG_DESTROYED);
>   
>   	proxy->refcount--;
> -	if (proxy_destroyed) {
> -		if (!proxy->refcount)
> -			free(proxy);
> +	if (!proxy->refcount)
> +		free(proxy);

This descends into use after free in the case the patch is concerned with?

I see you've turned that into an assert in the follow up patch though, 
which looks like a solid idea to me.

This and the follow up are
Reviewed-by: Derek Foreman <derekf at osg.samsung.com>


> +	if (proxy_destroyed) {
>   		wl_closure_destroy(closure);
>   		return;
>   	}
>