From c05f4f86ea2b4e647e2f6ee88f082a70456f747b Mon Sep 17 00:00:00 2001 From: Derek Foreman Date: Fri, 13 Aug 2021 13:29:22 -0500 Subject: [PATCH] client: Don't disconnect on receipt of object events destined for zombies Server side objects give protocol designers exciting ways to break clients. For example, if a client deletes an object at the same time the server is sending an event containing a new object to that object, then we currently silently drop that event. If a following event in the buffer from an object that has not yet been deleted also contains a new object, the wl_map constraint that new objects must be 1 higher than the current highest object count is violated. This results in a disconnect. Instead, let's augment the zombie accounting code to keep the entire proxy around on deletion, for both client and server generated objects. This way we can create and immediately delete objects that are destined for zombie proxies - thus creating zombie descendants. We can go no further to clean this up in the client library - we can't call a destructor because the protocol might dictate that child objects will be automatically destroyed on the destruction of the parent. So we turn a situation that would lead to an erroneous disconnect into one that may or may not leak object ids depending on protocol definition. Fixes #74 for some definition of "fix" anyway. Signed-off-by: Derek Foreman --- protocol/tests.xml | 31 +++++++++ src/connection.c | 16 +---- src/wayland-client.c | 144 ++++++++++++++++-------------------------- src/wayland-private.h | 3 - tests/display-test.c | 131 ++++++++++++++++++++++++++++++++++++++ 5 files changed, 218 insertions(+), 107 deletions(-) diff --git a/protocol/tests.xml b/protocol/tests.xml index 22d80a10..8f1b32ec 100644 --- a/protocol/tests.xml +++ b/protocol/tests.xml @@ -49,4 +49,35 @@ + + + + A trivial interface. + + + + + + + + Simple interface for generating server side objects to use in + zombie tests. + + + + + + + Create a new server side object. + + + + + + + + + + + diff --git a/src/connection.c b/src/connection.c index ad16eeef..310af52e 100644 --- a/src/connection.c +++ b/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 - * destroyed client side */ + /* references object we've already destroyed */ object = NULL; } else if (object == NULL && id != 0) { wl_log("unknown object (%u), message %s(%s)\n", diff --git a/src/wayland-client.c b/src/wayland-client.c index 63ce0d01..e0fb178e 100644 --- a/src/wayland-client.c +++ b/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) { - struct wl_zombie *zombie = prepare_zombie(proxy); - - /* The map now contains the zombie entry, until the delete_id - * event arrives. */ + wl_proxy_unref(proxy); + } else { + /* The map now contains the zombie entry. A delete_id + * event will clean up client generated ids eventually, + * server generated ids will linger until they're + * overwritten. */ wl_map_insert_at(&proxy->display->objects, WL_MAP_ENTRY_ZOMBIE, proxy->object.id, - zombie); - } else { - wl_map_insert_at(&proxy->display->objects, 0, - proxy->object.id, NULL); + proxy); } - - 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 - * event-information structure, which we can free. */ - free(proxy); + wl_proxy_unref(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 applicable). */ + /* If our proxy is gone, just eat the event. */ proxy = wl_map_lookup(&display->objects, id); - if (!proxy || wl_object_is_zombie(&display->objects, id)) { - struct wl_zombie *zombie = wl_map_lookup(&display->objects, id); - num_zombie_fds = (zombie && opcode < zombie->event_count) ? - zombie->fd_count[opcode] : 0; - + if (!proxy) { 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]" - "(%d fd, %d byte)\n", + fprintf(stderr, "[%7u.%03u] discarded @%d.[event %d]" + "(%d byte)\n", time / 1000, time % 1000, - zombie ? "zombie" : "unknown", - id, opcode, - num_zombie_fds, size); + id, opcode, 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); diff --git a/src/wayland-private.h b/src/wayland-private.h index 493e1bee..c3b28887 100644 --- a/src/wayland-private.h +++ b/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 diff --git a/tests/display-test.c b/tests/display-test.c index 3db7c95a..2a6864ca 100644 --- a/tests/display-test.c +++ b/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); +}