client: Plug a race in proxy destruction vs. dispatch

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@collabora.com>
Reviewed-by: Derek Foreman <derekf@osg.samsung.com>
Cc: Jonas Ådahl <jadahl@gmail.com>
Cc: Pekka Paalanen <pekka.paalanen@collabora.co.uk>

1 files changed, 41 insertions, 16 deletions

diff --git a/src/wayland-client.c b/src/wayland-client.c
index f3d71b0..987418a 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);
 
 	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);
-
 	pthread_mutex_lock(&display->mutex);
+
+	destroy_queued_closure(closure);
 }
 
 static int