[PATCH wayland v4 07/11] client: Plug a race in proxy destruction vs. dispatch

[Derek Foreman]

On 2017-12-28 01:53 PM, Daniel Stone wrote:
> Closures created to hold events which will be dispatched on the client,
> take a reference to the proxy for the object the event was sent to, as
> well as the proxies for all objects referenced in that event.
> 
> These references are dropped immediately before dispatch, with the
> display lock also being released. This leaves the potential for a
> vanishingly small race, where another thread drops the last reference
> on one of the proxies used in an event as it is being dispatched.
> 
> Fix this by splitting decrease_closure_args_refcount into two functions:
> one which validates the objects (to ensure that clients are not returned
> objects which they have destroyed), and another which unrefs all proxies
> on the closure (object event was sent to, all referenced objects) as
> well as the closure itself. For symmetry, increase_closure_args_refcount
> is now the place where the refcount for the proxy for the object the
> event was sent to, is increased.
> 
> This also happens to fix a bug: previously, if an event was sent to a
> client-destroyed object, and the event had object arguments, a reference
> would be leaked on the proxy for each of the object arguments.
> 
> Found by inspection whilst reviewing the zombie-FD-leak series.
> 
> Signed-off-by: Daniel Stone <daniels at collabora.com>
> Cc: Jonas Ådahl <jadahl at gmail.com>
> Cc: Derek Foreman <derekf at osg.samsung.com>
> Cc: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> ---
>   src/wayland-client.c | 57 +++++++++++++++++++++++++++++++++++++---------------
>   1 file changed, 41 insertions(+), 16 deletions(-)
> 
> diff --git a/src/wayland-client.c b/src/wayland-client.c
> index f3d71b0e..987418a1 100644
> --- a/src/wayland-client.c
> +++ b/src/wayland-client.c
> @@ -236,7 +236,7 @@ wl_proxy_unref(struct wl_proxy *proxy)
>   }
>   
>   static void
> -decrease_closure_args_refcount(struct wl_closure *closure)
> +validate_closure_objects(struct wl_closure *closure)
>   {
>   	const char *signature;
>   	struct argument_details arg;
> @@ -251,16 +251,44 @@ decrease_closure_args_refcount(struct wl_closure *closure)
>   		case 'n':
>   		case 'o':
>   			proxy = (struct wl_proxy *) closure->args[i].o;
> -			if (proxy) {
> -				if (proxy->flags & WL_PROXY_FLAG_DESTROYED)
> -					closure->args[i].o = NULL;
> +			if (proxy && proxy->flags & WL_PROXY_FLAG_DESTROYED)
> +				closure->args[i].o = NULL;
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +}
> +
> +/* Destroys a closure which was demarshaled for dispatch; unrefs all the
> + * proxies in its arguments, as well as its own proxy, and destroys the
> + * closure itself. */
> +static void
> +destroy_queued_closure(struct wl_closure *closure)
> +{
> +	const char *signature;
> +	struct argument_details arg;
> +	struct wl_proxy *proxy;
> +	int i, count;
> +
> +	signature = closure->message->signature;
> +	count = arg_count_for_signature(signature);
> +	for (i = 0; i < count; i++) {
> +		signature = get_next_argument(signature, &arg);
> +		switch (arg.type) {
> +		case 'n':
> +		case 'o':
> +			proxy = (struct wl_proxy *) closure->args[i].o;
> +			if (proxy)
>   				wl_proxy_unref(proxy);
> -			}
>   			break;
>   		default:
>   			break;
>   		}
>   	}
> +
> +	wl_proxy_unref(closure->proxy);
> +	wl_closure_destroy(closure);
>   }
>   
>   static void
> @@ -272,9 +300,7 @@ wl_event_queue_release(struct wl_event_queue *queue)
>   		closure = container_of(queue->event_list.next,
>   				       struct wl_closure, link);
>   		wl_list_remove(&closure->link);
> -		decrease_closure_args_refcount(closure);
> -		wl_proxy_unref(closure->proxy);
> -		wl_closure_destroy(closure);
> +		destroy_queued_closure(closure);
>   	}
>   }
>   
> @@ -1232,6 +1258,8 @@ increase_closure_args_refcount(struct wl_closure *closure)
>   			break;
>   		}
>   	}
> +
> +	closure->proxy->refcount++;
>   }
>   
>   static int
> @@ -1273,9 +1301,8 @@ queue_event(struct wl_display *display, int len)
>   		return -1;
>   	}
>   
> -	increase_closure_args_refcount(closure);
> -	proxy->refcount++;
>   	closure->proxy = proxy;
> +	increase_closure_args_refcount(closure);

Ok, queue_event() is always called with display lock held so there's no 
similar increase_closure_args race.

I guess I'd have done the proxy->refcount++ move into 
increase_closure_args_refcount() in a separate patch.

>   
>   	if (proxy == &display->proxy)
>   		queue = &display->display_queue;
> @@ -1302,13 +1329,11 @@ dispatch_event(struct wl_display *display, struct wl_event_queue *queue)
>   
>   	/* Verify that the receiving object is still valid by checking if has
>   	 * been destroyed by the application. */
> -
> -	decrease_closure_args_refcount(closure);
> +	validate_closure_objects(closure);
>   	proxy = closure->proxy;
>   	proxy_destroyed = !!(proxy->flags & WL_PROXY_FLAG_DESTROYED);
> -	wl_proxy_unref(proxy);
>   	if (proxy_destroyed) {
> -		wl_closure_destroy(closure);
> +		destroy_queued_closure(closure);
>   		return;
>   	}
>   
> @@ -1328,9 +1353,9 @@ dispatch_event(struct wl_display *display, struct wl_event_queue *queue)
>   				  &proxy->object, opcode, proxy->user_data);
>   	}
>   
> -	wl_closure_destroy(closure);
> -

This (wl_closure_destroy occurring with lock held) was bothering me when 
I noticed it while reviewing your other patches - though I'd probably 
have moved it in a separate patch.

Nice catch.

Reviewed-by: Derek Foreman <derekf at osg.samsung.com>

>   	pthread_mutex_lock(&display->mutex);
> +
> +	destroy_queued_closure(closure);
>   }
>   
>   static int
>