Merge branch 'spawn_zombies' into 'main'

Draft: client: Don't disconnect on receipt of object events destined for zombies

Closes #74

See merge request wayland/wayland!173

protocol/tests.xml

@@ -49,4 +49,35 @@
       <arg name="passer" type="object" interface="fd_passer"/>
     </request>
   </interface>
+
+  <interface name="noop" version="1">
+    <description summary="An interface that does nothing">
+      A trivial interface.
+    </description>
+
+    <request name="destroy" type="destructor"/>
+  </interface>
+
+  <interface name="necromancer" version="1">
+    <description summary="Helper for creating zombie objects">
+      Simple interface for generating server side objects to use in
+      zombie tests.
+    </description>
+
+    <request name="destroy" type="destructor"/>
+
+    <request name="create_server_object">
+      <description summary="create a new server side object">
+        Create a new server side object.
+      </description>
+    </request>
+
+    <event name="server_object">
+      <description summary="server side object">
+      </description>
+      <arg name="id" type="new_id" interface="noop" summary="server side object"/>
+    </event>
+
+
+  </interface>
 </protocol>

src/connection.c

@@ -200,12 +200,6 @@ close_fds(struct wl_ring_buffer *buffer, int max)
 	buffer->tail += size;
 }
 
-void
-wl_connection_close_fds_in(struct wl_connection *connection, int max)
-{
-	close_fds(&connection->fds_in, max);
-}
-
 int
 wl_connection_destroy(struct wl_connection *connection)
 {
@@ -809,7 +803,8 @@ wl_connection_demarshal(struct wl_connection *connection,
 				goto err;
 			}
 
-			if (wl_map_reserve_new(objects, id) < 0) {
+			if (wl_map_reserve_new(objects, id) < 0 &&
+			    !wl_object_is_zombie(objects, id)) {
 				wl_log("not a valid new object id (%u), "
 				       "message %s(%s)\n",
 				       id, message->name, message->signature);
@@ -879,10 +874,6 @@ wl_object_is_zombie(struct wl_map *map, uint32_t id)
 	if (map->side == WL_MAP_SERVER_SIDE)
 		return false;
 
-	/* Zombie objects can only have been created by the client. */
-	if (id >= WL_SERVER_ID_START)
-		return false;
-
 	flags = wl_map_lookup_flags(map, id);
 	return !!(flags & WL_MAP_ENTRY_ZOMBIE);
 }
@@ -909,8 +900,7 @@ wl_closure_lookup_objects(struct wl_closure *closure, struct wl_map *objects)
 
 			object = wl_map_lookup(objects, id);
 			if (wl_object_is_zombie(objects, id)) {
-				/* references object we've already
+				/* references object we've already destroyed */
-				 * destroyed client side */
 				object = NULL;
 			} else if (object == NULL && id != 0) {
 				wl_log("unknown object (%u), message %s(%s)\n",

src/wayland-client.c

@@ -55,11 +55,6 @@ enum wl_proxy_flag {
 	WL_PROXY_FLAG_WRAPPER = (1 << 2),
 };
 
-struct wl_zombie {
-	int event_count;
-	int *fd_count;
-};
-
 struct wl_proxy {
 	struct wl_object object;
 	struct wl_display *display;
@@ -352,62 +347,11 @@ wl_display_create_queue(struct wl_display *display)
 	return queue;
 }
 
-static int
-message_count_fds(const char *signature)
-{
-	unsigned int count, i, fds = 0;
-	struct argument_details arg;
-
-	count = arg_count_for_signature(signature);
-	for (i = 0; i < count; i++) {
-		signature = get_next_argument(signature, &arg);
-		if (arg.type == 'h')
-			fds++;
-	}
-
-	return fds;
-}
-
-static struct wl_zombie *
-prepare_zombie(struct wl_proxy *proxy)
-{
-	const struct wl_interface *interface = proxy->object.interface;
-	const struct wl_message *message;
-	int i, count;
-	struct wl_zombie *zombie = NULL;
-
-	/* If we hit an event with an FD, ensure we have a zombie object and
-	 * fill the fd_count slot for that event with the number of FDs for
-	 * that event. Interfaces with no events containing FDs will not have
-	 * zombie objects created. */
-	for (i = 0; i < interface->event_count; i++) {
-		message = &interface->events[i];
-		count = message_count_fds(message->signature);
-
-		if (!count)
-			continue;
-
-		if (!zombie) {
-			zombie = zalloc(sizeof(*zombie) +
-				        (interface->event_count * sizeof(int)));
-			if (!zombie)
-				return NULL;
-
-			zombie->event_count = interface->event_count;
-			zombie->fd_count = (int *) &zombie[1];
-		}
-
-		zombie->fd_count[i] = count;
-	}
-
-	return zombie;
-}
-
 static enum wl_iterator_result
 free_zombies(void *element, void *data, uint32_t flags)
 {
 	if (flags & WL_MAP_ENTRY_ZOMBIE)
-		free(element);
+		wl_proxy_unref(element);
 
 	return WL_ITERATOR_CONTINUE;
 }
@@ -471,7 +415,7 @@ static struct wl_proxy *
 wl_proxy_create_for_id(struct wl_proxy *factory,
 		       uint32_t id, const struct wl_interface *interface)
 {
-	struct wl_proxy *proxy;
+	struct wl_proxy *proxy, *zombie;
 	struct wl_display *display = factory->display;
 
 	proxy = zalloc(sizeof *proxy);
@@ -484,6 +428,9 @@ wl_proxy_create_for_id(struct wl_proxy *factory,
 	proxy->queue = factory->queue;
 	proxy->refcount = 1;
 	proxy->version = factory->version;
+	zombie = wl_map_lookup(&display->objects, id);
+	if (zombie && wl_object_is_zombie(&display->objects, id))
+		wl_proxy_unref(zombie);
 
 	wl_map_insert_at(&display->objects, 0, id, proxy);
 
@@ -493,25 +440,21 @@ wl_proxy_create_for_id(struct wl_proxy *factory,
 static void
 proxy_destroy(struct wl_proxy *proxy)
 {
+	proxy->flags |= WL_PROXY_FLAG_DESTROYED;
+
 	if (proxy->flags & WL_PROXY_FLAG_ID_DELETED) {
 		wl_map_remove(&proxy->display->objects, proxy->object.id);
-	} else if (proxy->object.id < WL_SERVER_ID_START) {
+		wl_proxy_unref(proxy);
-		struct wl_zombie *zombie = prepare_zombie(proxy);
+	} else {
-
+		/* The map now contains the zombie entry. A delete_id
-		/* The map now contains the zombie entry, until the delete_id
+		 * event will clean up client generated ids eventually,
-		 * event arrives. */
+		 * server generated ids will linger until they're
+		 * overwritten. */
 		wl_map_insert_at(&proxy->display->objects,
 				 WL_MAP_ENTRY_ZOMBIE,
 				 proxy->object.id,
-				 zombie);
+				 proxy);
-	} else {
-		wl_map_insert_at(&proxy->display->objects, 0,
-				 proxy->object.id, NULL);
 	}
-
-	proxy->flags |= WL_PROXY_FLAG_DESTROYED;
-
-	wl_proxy_unref(proxy);
 }
 
 static void
@@ -1034,9 +977,7 @@ display_handle_delete_id(void *data, struct wl_display *display, uint32_t id)
 	proxy = wl_map_lookup(&display->objects, id);
 
 	if (wl_object_is_zombie(&display->objects, id)) {
-		/* For zombie objects, the 'proxy' is actually the zombie
+		wl_proxy_unref(proxy);
-		 * event-information structure, which we can free. */
-		free(proxy);
 		wl_map_remove(&display->objects, id);
 	} else if (proxy) {
 		proxy->flags |= WL_PROXY_FLAG_ID_DELETED;
@@ -1411,6 +1352,7 @@ create_proxies(struct wl_proxy *sender, struct wl_closure *closure)
 			if (proxy == NULL)
 				return -1;
 			closure->args[i].o = (struct wl_object *)proxy;
+
 			break;
 		default:
 			break;
@@ -1447,6 +1389,30 @@ increase_closure_args_refcount(struct wl_closure *closure)
 	closure->proxy->refcount++;
 }
 
+static void
+inter_zombie_closure(struct wl_closure *closure)
+{
+	const char *signature;
+	struct argument_details arg;
+	int i, count;
+	struct wl_proxy *proxy;
+
+	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':
+			proxy = (struct wl_proxy *) closure->args[i].o;
+			if (proxy)
+				proxy_destroy(proxy);
+			break;
+		default:
+			break;
+		}
+	}
+}
+
 static int
 queue_event(struct wl_display *display, int len)
 {
@@ -1458,7 +1424,7 @@ queue_event(struct wl_display *display, int len)
 	struct wl_event_queue *queue;
 	struct timespec tp;
 	unsigned int time;
-	int num_zombie_fds;
+	bool zombie = false;
 
 	wl_connection_copy(display->connection, p, sizeof p);
 	id = p[0];
@@ -1467,29 +1433,18 @@ queue_event(struct wl_display *display, int len)
 	if (len < size)
 		return 0;
 
-	/* If our proxy is gone or a zombie, just eat the event (and any FDs,
+	/* If our proxy is gone, just eat the event. */
-	 * if applicable). */
 	proxy = wl_map_lookup(&display->objects, id);
-	if (!proxy || wl_object_is_zombie(&display->objects, id)) {
+	if (!proxy) {
-		struct wl_zombie *zombie = wl_map_lookup(&display->objects, id);
-		num_zombie_fds = (zombie && opcode < zombie->event_count) ?
-			zombie->fd_count[opcode] : 0;
-
 		if (debug_client) {
 			clock_gettime(CLOCK_REALTIME, &tp);
 			time = (tp.tv_sec * 1000000L) + (tp.tv_nsec / 1000);
 
-			fprintf(stderr, "[%7u.%03u] discarded [%s]@%d.[event %d]"
+			fprintf(stderr, "[%7u.%03u] discarded @%d.[event %d]"
-				"(%d fd, %d byte)\n",
+				"(%d byte)\n",
 				time / 1000, time % 1000,
-				zombie ? "zombie" : "unknown",
+				id, opcode, size);
-				id, opcode,
-				num_zombie_fds, size);
 		}
-		if (num_zombie_fds > 0)
-			wl_connection_close_fds_in(display->connection,
-						   num_zombie_fds);
-
 		wl_connection_consume(display->connection, size);
 		return size;
 	}
@@ -1516,6 +1471,13 @@ queue_event(struct wl_display *display, int len)
 		return -1;
 	}
 
+	zombie = wl_object_is_zombie(&display->objects, id);
+	if (zombie) {
+		inter_zombie_closure(closure);
+		wl_closure_destroy(closure);
+		return size;
+	}
+
 	closure->proxy = proxy;
 	increase_closure_args_refcount(closure);
 

src/wayland-private.h

@@ -232,7 +232,4 @@ zalloc(size_t s)
 	return calloc(1, s);
 }
 
-void
-wl_connection_close_fds_in(struct wl_connection *connection, int max);
-
 #endif

tests/display-test.c

@@ -1629,3 +1629,134 @@ TEST(global_remove)
 
 	display_destroy(d);
 }
+
+struct new_zombie_data {
+	struct necromancer *necromancer;
+	struct necromancer *necromancer_two;
+	struct noop *temp_noop;
+};
+
+static void
+server_object(void *data, struct necromancer *necro, struct noop *noop)
+{
+	struct new_zombie_data *nzd = data;
+
+	/* We should only see this once, since one of the two noop
+	 * objects we create should be created as a zombie */
+	assert(!nzd->temp_noop);
+	nzd->temp_noop = noop;
+}
+
+struct necromancer_listener necromancer_listener = {
+	server_object,
+};
+
+static void
+new_zombie_handle_globals(void *data, struct wl_registry *registry,
+			  uint32_t id, const char *intf, uint32_t ver)
+{
+	struct new_zombie_data *rg = data;
+
+	if (!strcmp(intf, "necromancer")) {
+		rg->necromancer = wl_registry_bind(registry, id, &necromancer_interface, 1);
+		rg->necromancer_two = wl_registry_bind(registry, id, &necromancer_interface, 1);
+		necromancer_add_listener(rg->necromancer, &necromancer_listener, NULL);
+		necromancer_add_listener(rg->necromancer_two, &necromancer_listener, rg);
+	}
+}
+
+static const struct wl_registry_listener new_zombie_registry_listener = {
+	new_zombie_handle_globals,
+	NULL
+};
+
+static void noop_destructor(struct wl_client *client, struct wl_resource *res)
+{
+	wl_resource_destroy(res);
+}
+
+static const struct noop_interface noop_server_interface = {
+	noop_destructor
+};
+
+static void
+send_new_object(struct wl_client *client, struct wl_resource *res)
+{
+	struct wl_resource *new_res;
+
+	new_res = wl_resource_create(client, &noop_interface, 1, 0);
+	wl_resource_set_implementation(new_res, &noop_server_interface, NULL, NULL);
+	necromancer_send_server_object(res, new_res);
+}
+
+static void necromancer_destructor(struct wl_client *client, struct wl_resource *res)
+{
+	wl_resource_destroy(res);
+}
+
+static const struct necromancer_interface necro_interface = {
+	necromancer_destructor,
+	send_new_object
+};
+
+static void
+bind_necromancer(struct wl_client *client, void *data,
+		 uint32_t vers, uint32_t id)
+{
+	struct wl_resource *res;
+
+	res = wl_resource_create(client, &necromancer_interface, vers, id);
+	wl_resource_set_implementation(res, &necro_interface, NULL, NULL);
+}
+
+static void
+spawn_new_zombie(void *data)
+{
+	struct new_zombie_data rg = {};
+	struct client *c = client_connect();
+	struct wl_registry *reg;
+
+	reg = wl_display_get_registry(c->wl_display);
+	wl_registry_add_listener(reg, &new_zombie_registry_listener, &rg);
+
+	/* Get the globals */
+	wl_display_roundtrip(c->wl_display);
+
+	wl_registry_destroy(reg);
+
+	necromancer_create_server_object(rg.necromancer);
+	necromancer_create_server_object(rg.necromancer_two);
+	necromancer_destroy(rg.necromancer);
+	/* We should now have two new_id events on the way,
+	 * and the first one will be destined for a zombie
+	 * when it arrives. That would violate the wl_map
+	 * requirement that a new id can only be one higher
+	 * than the current max id in the map if we simply
+	 * dropped the first inbound object.
+	 */
+
+	wl_display_roundtrip(c->wl_display);
+	assert(rg.temp_noop);
+	noop_destroy(rg.temp_noop);
+	necromancer_destroy(rg.necromancer_two);
+	wl_display_roundtrip(c->wl_display);
+	client_disconnect(c);
+}
+
+TEST(new_zombie)
+{
+	struct display *d;
+	struct wl_global *g;
+
+	d = display_create();
+	g = wl_global_create(d->wl_display, &necromancer_interface,
+			     1, d, bind_necromancer);
+
+	client_create_noarg(d, spawn_new_zombie);
+
+	display_run(d);
+
+	wl_global_destroy(g);
+
+	display_destroy(d);
+}