From 0db836ebc867b98a984b983b0371d44deed14779 Mon Sep 17 00:00:00 2001 From: Jonathan Leivent Date: Fri, 26 Jan 2024 15:03:27 -0500 Subject: [PATCH 1/4] tests: server needs zombies and zombie domino effect 2 new tests added to display-test.c: server_needs_zombies demonstrates that the same reason for zombies in the client exist in the server: proper demarshalling of fds zombie_domino_effect demonstrates that messages sent to zombies need to create new zombies for new_id args Signed-off-by: Jonathan Leivent --- protocol/tests.xml | 50 +++++ tests/display-test.c | 456 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 506 insertions(+) diff --git a/protocol/tests.xml b/protocol/tests.xml index 22d80a10..8ca098f8 100644 --- a/protocol/tests.xml +++ b/protocol/tests.xml @@ -49,4 +49,54 @@ + + + + A way to get these tests started. + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/tests/display-test.c b/tests/display-test.c index bcb3267f..8b9f1906 100644 --- a/tests/display-test.c +++ b/tests/display-test.c @@ -41,6 +41,7 @@ #include #include +#include #include "wayland-private.h" #include "wayland-server.h" @@ -1710,3 +1711,458 @@ TEST(no_source_terminate) display_run(d); display_destroy(d); } + + +struct zombie_init *zinit_global = NULL; + +/* snz: server needs zombies test - demonstrate that the server has a + need for zombies for the same reason as the client: proper + demarshalling of fds in the wire protocol so that they continue to + appear as the args to requests they're supposed to be args to. */ + +static void +snz_have_an_fd_event(void *data, struct zombie_target *target, int32_t fd) +{ + assert(false); /* not used in snz test */ +} + +static void +snz_have_a_new_id_event(void *data, struct zombie_target *target, + struct zombie_target *the_id) +{ + assert(false); /* not used in snz test */ +} + +bool snz_test_done = false; + +static void +snz_destruct_event(void *data, struct zombie_target *target) +{ + zombie_target_destroy(target); + snz_test_done = true; + /* send a final fd that is writable to the server to try + writing. If it writes the previous one in the wire (sent + by snz_new_obj_event) by mistake, it will fail because that + one is RO. */ + zombie_init_try_this_fd_request(zinit_global, fileno(stderr)); +} + +static const struct zombie_target_listener snz_target_listener = { + snz_have_an_fd_event, + snz_have_a_new_id_event, + snz_destruct_event, +}; + +static void +snz_new_obj_event(void *data, struct zombie_init *zinit, struct zombie_target *target) +{ + zombie_init_got_it_request(zinit, target); + assert(zombie_target_add_listener(target, &snz_target_listener, NULL)==0); + /* Use the current dir as a read-only fd to send to the + zombie. This happens before we send a writable fd (stderr, + sent by snz_destruct_event), so if this fd is not + demarshalled properly, an attempt to write to the supposed + second fd would fail. */ + int rofd = open(".", O_RDONLY | O_CLOEXEC | O_DIRECTORY); + zombie_target_have_an_fd_request(target, rofd); + close(rofd); +} + +static void +snz_got_it_event(void *data, struct zombie_init *zinit, + struct zombie_target *target) +{ + /*noop for snz test*/ +} + +static void +snz_try_this_fd_event(void *data, struct zombie_init *zinit, int32_t fd) +{ + assert(false); /* not used for snz test */ +} + +static const struct zombie_init_listener snz_init_listener = { + snz_new_obj_event, + snz_got_it_event, + snz_try_this_fd_event, +}; + +static void +snz_handle_globals(void *data, struct wl_registry *registry, + uint32_t id, const char *intf, uint32_t ver) +{ + if (!strcmp(intf, "zombie_init")) { + zinit_global = wl_registry_bind(registry, id, &zombie_init_interface, 1); + zombie_init_add_listener(zinit_global, &snz_init_listener, NULL); + } +} + +static const struct wl_registry_listener snz_registry_listener = { + snz_handle_globals, + NULL, +}; + +static void +snz_client_loop(void *data) +{ + struct client *c = client_connect(); + struct wl_registry *registry; + + registry = wl_display_get_registry(c->wl_display); + wl_registry_add_listener(registry, &snz_registry_listener, NULL); + + while (!snz_test_done) + if (wl_display_roundtrip(c->wl_display) < 0) + assert(false && "snz client loop killed off"); + + assert(zinit_global); + zombie_init_destroy(zinit_global); + + wl_registry_destroy(registry); + + client_disconnect_nocheck(c); +} + +static void +snz_init_new_obj_request(struct wl_client *client, struct wl_resource *resource, + uint32_t the_object) +{ + assert(false); /* not used in snz test */ +} + +static void +snz_init_got_it_request(struct wl_client *client, struct wl_resource *resource, + struct wl_resource *target) +{ + /* this should make target a zombie, if the server had zombies */ + zombie_target_send_destruct_event(target); + wl_resource_destroy(target); +} + +bool snz_got_try_this_fd_request = false; + +static const char finalmsg[] = "This write proves we demarshalled fds properly\n"; + +static void +snz_try_this_fd_request(struct wl_client *client, struct wl_resource *resource, int32_t fd) +{ + /* If writing this fd fails, that's because it's the wrong fd + left on the wire from a previous request that wasn't + demarshalled properly */ + assert(write(fd, finalmsg, sizeof finalmsg) == sizeof finalmsg); + fsync(fd); + close(fd); + snz_got_try_this_fd_request = true; +} + +static const struct zombie_init_interface snz_init_interface = { + snz_init_new_obj_request, + snz_init_got_it_request, + snz_try_this_fd_request, +}; + +static void +snz_zombie_init_destroy(struct wl_resource *res) +{ +} + +static void +snz_target_have_an_fd_request(struct wl_client *client, struct wl_resource *resource, + int32_t fd) +{ + /* noop */ +} + +static void +snz_target_destruct_request(struct wl_client *client, struct wl_resource *resource) +{ + assert(false); /* not used in snz test */ +} + +static const struct zombie_target_interface snz_target_interface = { + snz_target_have_an_fd_request, + snz_target_destruct_request, +}; + +static void +snz_zombie_target_destroy(struct wl_resource *res) +{ +} + +static void +bind_snz(struct wl_client *client, void *data, + uint32_t vers, uint32_t id) +{ + struct wl_resource *init, *target; + init = wl_resource_create(client, &zombie_init_interface, vers, id); + wl_resource_set_implementation(init, &snz_init_interface, NULL, + snz_zombie_init_destroy); + assert(init); + target = wl_resource_create(client, &zombie_target_interface, 1, 0); + wl_resource_set_implementation(target, &snz_target_interface, NULL, + snz_zombie_target_destroy); + zombie_init_send_new_obj_event(init, target); +} + +TEST(server_needs_zombies) /*snz*/ +{ + struct display *d; + struct wl_global *g; + + snz_got_try_this_fd_request = false; + + d = display_create(); + + g = wl_global_create(d->wl_display, &zombie_init_interface, + 1, d, bind_snz); + + + client_create_noarg(d, snz_client_loop); + + display_run(d); + + assert(snz_got_try_this_fd_request); + + wl_global_destroy(g); + + display_destroy(d); +} + + +/*zde: zombie domino effect test - show that properly demarshalling fds + is not the only job that zombies should have - they also need to + make new_id args into zombies, else an fd sent to one of them will + get left in the wire and foul things up. */ + +static void +zde_have_an_fd_event(void *data, struct zombie_target *target, int32_t fd) +{ + /*if we get here, then the zde passes!*/ +} + +static void +zde_have_a_new_id_event(void *data, struct zombie_target *target, + struct zombie_target *the_id) +{ + /*noop - but demarshalling this must zombify the_id to avoid eventual failure */ +} + +static void +zde_destruct_event(void *data, struct zombie_target *target) +{ + assert(false); /*not used by zde test */ +} + +static const struct zombie_target_listener zde_target_listener = { + zde_have_an_fd_event, + zde_have_a_new_id_event, + zde_destruct_event, +}; + +static void +zde_new_obj_event(void *data, struct zombie_init *zinit, struct zombie_target *target) +{ + assert(false); /* not used in zde test */ +} + +static void +zde_got_it_event(void *data, struct zombie_init *zinit, struct zombie_target *target) +{ + /*noop*/ +} + +bool zde_test_done = false; + +static void +zde_try_this_fd_event(void *data, struct zombie_init *zinit, int32_t fd) +{ + zde_test_done = true; + /* if writing this fd fails, that's because it's the wrong fd + left in the wire by improperly demarshalling an event sent + to a new_id that should have become a zombie but didn't */ + assert(write(fd, finalmsg, sizeof finalmsg) == sizeof finalmsg); + zombie_init_try_this_fd_request(zinit_global, fd); + fsync(fd); + close(fd); +} + +static const struct zombie_init_listener zde_init_listener = { + zde_new_obj_event, + zde_got_it_event, + zde_try_this_fd_event, +}; + +static void +zde_handle_globals(void *data, struct wl_registry *registry, + uint32_t id, const char *intf, uint32_t ver) +{ + if (!strcmp(intf, "zombie_init")) { + zinit_global = wl_registry_bind(registry, id, &zombie_init_interface, 1); + zombie_init_add_listener(zinit_global, &zde_init_listener, NULL); + } +} + +static const struct wl_registry_listener zde_registry_listener = { + zde_handle_globals, + NULL, +}; + +static void +zde_client_loop(void *data) +{ + struct client *c = client_connect(); + struct wl_registry *registry; + zinit_global = NULL; + + registry = wl_display_get_registry(c->wl_display); + wl_registry_add_listener(registry, &zde_registry_listener, NULL); + + while (!zinit_global && + wl_display_roundtrip(c->wl_display) > 0); + + struct zombie_target *target = zombie_init_new_obj_request(zinit_global); + zombie_target_add_listener(target, &zde_target_listener, NULL); + + /*make target into a zombie */ + zombie_target_destruct_request(target); + zombie_target_destroy(target); + + while (!zde_test_done) + if (wl_display_roundtrip(c->wl_display) < 0) + assert(false && "zde client loop killed off"); + + /*once more, with feeling:*/ + assert(wl_display_roundtrip(c->wl_display) > 0); + + zombie_init_destroy(zinit_global); + + wl_registry_destroy(registry); + + client_disconnect_nocheck(c); +} + +static void +zde_target_have_an_fd_request(struct wl_client *client, struct wl_resource *resource, + int32_t fd) +{ + assert(false); /* not used in zde test */ +} + +static void +zde_zombie_target_destroy(struct wl_resource *res) +{ + /*noop*/ +} + +static void +zde_target_destruct_request(struct wl_client *client, struct wl_resource *resource) +{ + wl_resource_destroy(resource); +} + +static const struct zombie_target_interface zde_target_interface = { + zde_target_have_an_fd_request, + zde_target_destruct_request, +}; + +const int num_domino_new_ids = 3; + +static void +zde_init_new_obj_request(struct wl_client *client, struct wl_resource *resource, + uint32_t the_object) +{ + int i; + struct wl_resource *target = + wl_resource_create(client, &zombie_target_interface, 1, the_object); + struct wl_resource *next_target = NULL; + + assert(target); + wl_resource_set_implementation(target, &zde_target_interface, NULL, + zde_zombie_target_destroy); + zombie_init_send_got_it_event(resource, target); + + /* the dominos: send a chain of new_id events...*/ + for (i = 1; i <= num_domino_new_ids; i++) { + next_target = wl_resource_create(client, &zombie_target_interface, 1, 0); + wl_resource_set_implementation(next_target, &zde_target_interface, NULL, + zde_zombie_target_destroy); + zombie_target_send_have_a_new_id_event(target, next_target); + target = next_target; + } + + /* open the current dir read-only and send it as the fd to the + zombie. This happens before we send a writable fd + (stderr), so if this fd is not demarshalled properly, an + attempt to write to the supposed second fd would fail. */ + int rofd = open(".", O_RDONLY | O_CLOEXEC | O_DIRECTORY); + zombie_target_send_have_an_fd_event(target, rofd); + close(rofd); + + /* one last round-trip to make sure the client survived: */ + zombie_init_send_try_this_fd_event(resource, fileno(stderr)); +} + +static void +zde_init_got_it_request(struct wl_client *client, struct wl_resource *resource, + struct wl_resource *target) +{ + assert(false); /* not used in zde test */ +} + +bool zde_got_try_this_fd_request = false; + +static void +zde_try_this_fd_request(struct wl_client *client, struct wl_resource *resource, + int32_t fd) +{ + /* this is redundant with zde_try_this_fd_event, but why not try it as well */ + assert(write(fd, finalmsg, sizeof finalmsg) == sizeof finalmsg); + fsync(fd); + close(fd); + zde_got_try_this_fd_request = true; +} + +static const struct zombie_init_interface zde_init_interface = { + zde_init_new_obj_request, + zde_init_got_it_request, + zde_try_this_fd_request, +}; + +static void +zde_zombie_init_destroy(struct wl_resource *res) +{ +} + +static void +bind_zde(struct wl_client *client, void *data, + uint32_t vers, uint32_t id) +{ + struct wl_resource *init; + init = wl_resource_create(client, &zombie_init_interface, vers, id); + wl_resource_set_implementation(init, &zde_init_interface, NULL, + zde_zombie_init_destroy); + assert(init); +} + +TEST(zombie_domino_effect) /*zde*/ +{ + struct display *d; + struct wl_global *g; + + zde_got_try_this_fd_request = false; + + d = display_create(); + + g = wl_global_create(d->wl_display, &zombie_init_interface, + 1, d, bind_zde); + + client_create_noarg(d, zde_client_loop); + + display_run(d); + + assert(zde_got_try_this_fd_request); + + wl_global_destroy(g); + + display_destroy(d); +} From dcd5996175dc985f42f5307e865371cb8d4db0d4 Mon Sep 17 00:00:00 2001 From: Jonathan Leivent Date: Fri, 26 Jan 2024 18:42:14 -0500 Subject: [PATCH 2/4] util: unified and enhanced zombie handing in wl_map servers and clients now both use zombies. Zombies are interface pointers, so that they retain info for demarshalling fds AND new_id args, which must be turned into other zombies. Demarshalling messages to zombies is handled by the same wl_connection_demarshal code used for messages to live objects. Server zombies are eventually reaped after moving through FIFO zombie ring, because there is no delete_id request in the protocol. Signed-off-by: Jonathan Leivent --- src/connection.c | 66 +++++++++--- src/wayland-client.c | 199 ++++++++++++++++------------------- src/wayland-private.h | 38 ++++++- src/wayland-server.c | 76 +++++++++++--- src/wayland-util.c | 222 ++++++++++++++++++++++++++++++++++------ tests/connection-test.c | 3 + tests/map-test.c | 12 ++- 7 files changed, 436 insertions(+), 180 deletions(-) diff --git a/src/connection.c b/src/connection.c index b89166fb..ec1dd50b 100644 --- a/src/connection.c +++ b/src/connection.c @@ -689,10 +689,14 @@ wl_closure_vmarshal(struct wl_object *sender, uint32_t opcode, va_list ap, } struct wl_closure * -wl_connection_demarshal(struct wl_connection *connection, +wl_connection_demarshal_common(struct wl_connection *connection, uint32_t size, struct wl_map *objects, - const struct wl_message *message) + const struct wl_message *message, + int (*handle_new_id)(void *data, + uint32_t id, + const struct wl_interface *interface), + void *data) { uint32_t *p, *next, *end, length, length_in_u32, id; int fd; @@ -813,7 +817,7 @@ wl_connection_demarshal(struct wl_connection *connection, goto err; } - if (wl_map_reserve_new(objects, id) < 0) { + if (handle_new_id(data, id, message->types[i]) < 0) { if (errno == EINVAL) { wl_log("not a valid new object id (%u), " "message %s(%s)\n", id, @@ -876,21 +880,51 @@ wl_connection_demarshal(struct wl_connection *connection, return NULL; } +static int +reserve_new_id(void *data, uint32_t id, const struct wl_interface *interface) +{ + struct wl_map *objects = data; + return wl_map_reserve_new(objects, id); +} + +struct wl_closure * +wl_connection_demarshal(struct wl_connection *connection, + uint32_t size, + struct wl_map *objects, + const struct wl_message *message) +{ + return wl_connection_demarshal_common(connection, + size, + objects, + message, + &reserve_new_id, + objects); +} + +/* demarshal messages to zombies. handle_new_id should create a new zombie. */ +void +wl_connection_demarshal_zombie(struct wl_connection *connection, + uint32_t size, + struct wl_map *objects, + const struct wl_message *message, + int (*handle_new_id)(void* data, + uint32_t id, + const struct wl_interface *interface), + void *data) +{ + struct wl_closure *closure = wl_connection_demarshal_common(connection, + size, + objects, + message, + handle_new_id, + data); + wl_closure_destroy(closure); +} + bool wl_object_is_zombie(struct wl_map *map, uint32_t id) { - uint32_t flags; - - /* Zombie objects only exist on the client side. */ - 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); + return wl_map_lookup_zombie(map, id) != NULL; } int @@ -916,7 +950,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 */ + * 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 94438e30..6f01fa99 100644 --- a/src/wayland-client.c +++ b/src/wayland-client.c @@ -50,16 +50,10 @@ /** \cond */ enum wl_proxy_flag { - WL_PROXY_FLAG_ID_DELETED = (1 << 0), WL_PROXY_FLAG_DESTROYED = (1 << 1), 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; @@ -412,66 +406,6 @@ wl_display_create_queue_with_name(struct wl_display *display, const char *name) 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); - - return WL_ITERATOR_CONTINUE; -} - static struct wl_proxy * proxy_create(struct wl_proxy *factory, const struct wl_interface *interface, uint32_t version) @@ -564,21 +498,9 @@ wl_proxy_create_for_id(struct wl_proxy *factory, static void proxy_destroy(struct wl_proxy *proxy) { - 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_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); - } + wl_map_zombify(&proxy->display->objects, + proxy->object.id, + proxy->object.interface); proxy->flags |= WL_PROXY_FLAG_DESTROYED; @@ -1107,22 +1029,10 @@ display_handle_error(void *data, static void display_handle_delete_id(void *data, struct wl_display *display, uint32_t id) { - struct wl_proxy *proxy; - pthread_mutex_lock(&display->mutex); - 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_map_remove(&display->objects, id); - } else if (proxy) { - proxy->flags |= WL_PROXY_FLAG_ID_DELETED; - } else { + if (wl_map_mark_deleted(&display->objects, id) != 0) wl_log("error: received delete_id for unknown id (%u)\n", id); - } pthread_mutex_unlock(&display->mutex); } @@ -1359,7 +1269,6 @@ WL_EXPORT void wl_display_disconnect(struct wl_display *display) { wl_connection_destroy(display->connection); - wl_map_for_each(&display->objects, free_zombies, NULL); wl_map_release(&display->objects); wl_event_queue_release(&display->default_queue); free(display->default_queue.name); @@ -1537,6 +1446,40 @@ increase_closure_args_refcount(struct wl_closure *closure) closure->proxy->refcount++; } +/* zombify_new_id is only called by wl_connection_demarshal_zombie + * when the event it is demarhsalling has new_id argments, meaning + * they're server IDs. Those server IDs need to have zombies created + * for them in case the server subsequently sends an event to them, + * which might contain fds or more new_ids. */ +static int +zombify_new_id(void *displayp, uint32_t id, const struct wl_interface *interface) +{ + struct wl_display *display = displayp; + + if (id >= WL_SERVER_ID_START) { + /* Make the server ID a zombie in case it gets sent an + * event. This handles the "domino effect" case where + * new_id args in events sent to zombies are + * subsequently sent events, with new_id args, and so + * on. */ + if (wl_map_insert_at(&display->objects, 0, id, (void*)interface) != 0) { + errno = EINVAL; + return -1; + } + if (wl_map_zombify(&display->objects, id, interface) != 0) { + errno = EINVAL; + return -1; + } + assert(wl_map_lookup_zombie(&display->objects, id) == interface); + + return 0; + } else { + /* Unexpected: the id is a client ID */ + errno = EINVAL; + return -1; + } +} + static int queue_event(struct wl_display *display, int len) { @@ -1548,7 +1491,7 @@ queue_event(struct wl_display *display, int len) struct wl_event_queue *queue; struct timespec tp; unsigned int time; - int num_zombie_fds; + const struct wl_interface *interface; wl_connection_copy(display->connection, p, sizeof p); id = p[0]; @@ -1557,30 +1500,66 @@ 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). */ 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) { + interface = wl_map_lookup_zombie(&display->objects, id); 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 [%s]#%u.[event %d]" + "(%s, %d byte)\n", time / 1000, time % 1000, - zombie ? "zombie" : "unknown", + interface ? "zombie" : "unknown", id, opcode, - num_zombie_fds, size); + interface ? interface->name : "unknown interface", + size); } - if (num_zombie_fds > 0) - wl_connection_close_fds_in(display->connection, - num_zombie_fds); - wl_connection_consume(display->connection, size); + if (interface) { + if (opcode >= interface->event_count) { + wl_log("interface '%s' has no event %u\n", + interface->name, opcode); + return -1; + } + message = &interface->events[opcode]; + if (message == NULL) { + wl_log("interface '%s' opcode %u has no event\n", + interface->name, opcode); + return -1; + } + if (message->name == NULL) { + wl_log("interface '%s' opcode %u has noname event,\n", + interface->name, opcode); + return -1; + } + wl_connection_demarshal_zombie(display->connection, size, + &display->objects, + message, + &zombify_new_id, display); + } else { + /* what to do if there is no zombie interface? The + * original zombie code just consumed the size and + * continued on. The assumption was if there was no + * zombie, then the reason was that there's no fds to + * consume, because zombies were only created for proxies + * that have fd-bearing events. But, there may have been + * new_id args, so that could still have caused problems. + * Under the new zombie regime, which always creates + * zombies for proxies (because even an unpatched server + * sends delete_id events), the absence of a zombie is + * unexpected and means we need to kill the client. */ + wl_log("Missing zombie for ID %u, event %u\n", id, opcode); + return -1; + /* Consider the case in the old regime where the proxy had + * no fd-bearing events and no new_id-bearing events. In + * such cases, when there was no zombie, consuming the + * size and moving on was OK, but there was no way to tell + * it was OK. Now we know the difference, and will always + * log this error. + */ + } return size; } diff --git a/src/wayland-private.h b/src/wayland-private.h index 6b506584..eb67dc5e 100644 --- a/src/wayland-private.h +++ b/src/wayland-private.h @@ -59,19 +59,23 @@ wl_interface_equal(const struct wl_interface *iface1, const struct wl_interface *iface2); /* Flags for wl_map_insert_new and wl_map_insert_at. Flags can be queried with - * wl_map_lookup_flags. The current implementation has room for 1 bit worth of + * wl_map_lookup_flags. The current implementation has room for 32 bits worth of * flags. If more flags are ever added, the implementation of wl_map will have * to change to allow for new flags */ enum wl_map_entry_flags { WL_MAP_ENTRY_LEGACY = (1 << 0), /* Server side only */ - WL_MAP_ENTRY_ZOMBIE = (1 << 0) /* Client side only */ }; struct wl_map { struct wl_array client_entries; struct wl_array server_entries; uint32_t side; + /* the free_list is LIFO, the zombie list is FIFO */ uint32_t free_list; + uint32_t zombie_list_head; + uint32_t zombie_list_tail; + /* if zombie_list_count is < 0, then the zombie_list is disabled */ + int32_t zombie_list_count; }; typedef enum wl_iterator_result (*wl_iterator_func_t)(void *element, @@ -93,12 +97,18 @@ wl_map_insert_at(struct wl_map *map, uint32_t flags, uint32_t i, void *data); int wl_map_reserve_new(struct wl_map *map, uint32_t i); -void -wl_map_remove(struct wl_map *map, uint32_t i); +int +wl_map_zombify(struct wl_map *map, uint32_t i, const struct wl_interface *interface); + +int +wl_map_mark_deleted(struct wl_map *map, uint32_t i); void * wl_map_lookup(struct wl_map *map, uint32_t i); +const struct wl_interface * +wl_map_lookup_zombie(struct wl_map *map, uint32_t i); + uint32_t wl_map_lookup_flags(struct wl_map *map, uint32_t i); @@ -179,6 +189,26 @@ wl_closure_vmarshal(struct wl_object *sender, uint32_t opcode, va_list ap, const struct wl_message *message); +struct wl_closure * +wl_connection_demarshal_common(struct wl_connection *connection, + uint32_t size, + struct wl_map *objects, + const struct wl_message *message, + int (*handle_new_id)(void *data, + uint32_t id, + const struct wl_interface *interface), + void *data); + +void +wl_connection_demarshal_zombie(struct wl_connection *connection, + uint32_t size, + struct wl_map *objects, + const struct wl_message *message, + int (*handle_new_id)(void* data, + uint32_t id, + const struct wl_interface *interface), + void *data); + struct wl_closure * wl_connection_demarshal(struct wl_connection *connection, uint32_t size, diff --git a/src/wayland-server.c b/src/wayland-server.c index 78ff4055..38427f39 100644 --- a/src/wayland-server.c +++ b/src/wayland-server.c @@ -327,6 +327,38 @@ destroy_client_with_error(struct wl_client *client, const char *reason) wl_client_destroy(client); } +/* zombify_new_id_queue_delete_id_event is only called by + * wl_connection_demarshal_zombie when the request it is demarhsalling + * has new_id argments, meaning they're client IDs. Those client IDs + * need to have zombies created for them in case the client + * subsequently sends a request to them, which might contain fds or + * more new_ids. */ +static int +zombify_new_id_queue_delete_id_event(void *clientp, uint32_t id, + const struct wl_interface *interface) +{ + struct wl_client *client = clientp; + + if (id < WL_SERVER_ID_START) { + if (wl_map_insert_at(&client->objects, 0, id, (void*)interface) != 0) { + errno = EINVAL; + return -1; + } + if (wl_map_zombify(&client->objects, id, interface) != 0) { + errno = EINVAL; + return -1; + } + if (client->display_resource) + wl_resource_queue_event(client->display_resource, + WL_DISPLAY_DELETE_ID, id); + + return 0; + } else { + /* Unexpected: the id is a server ID. */ + return -1; + } +} + static int wl_client_connection_data(int fd, uint32_t mask, void *data) { @@ -340,6 +372,7 @@ wl_client_connection_data(int fd, uint32_t mask, void *data) uint32_t resource_flags; int opcode, size, since; int len; + const struct wl_interface *interface; if (mask & WL_EVENT_HANGUP) { wl_client_destroy(client); @@ -383,9 +416,29 @@ wl_client_connection_data(int fd, uint32_t mask, void *data) resource = wl_map_lookup(&client->objects, p[0]); resource_flags = wl_map_lookup_flags(&client->objects, p[0]); if (resource == NULL) { - wl_resource_post_error(client->display_resource, - WL_DISPLAY_ERROR_INVALID_OBJECT, - "invalid object %u", p[0]); + interface = wl_map_lookup_zombie(&client->objects, p[0]); + if (interface == NULL) { + wl_resource_post_error(client->display_resource, + WL_DISPLAY_ERROR_INVALID_OBJECT, + "invalid object %u", p[0]); + break; + } + + if (opcode >= interface->method_count) { + wl_resource_post_error(client->display_resource, + WL_DISPLAY_ERROR_INVALID_METHOD, + "invalid method %d, object %s#%u", + opcode, + interface->name, + p[0]); + break; + } + + message = &interface->methods[opcode]; + wl_connection_demarshal_zombie(client->connection, size, + &client->objects, message, + &zombify_new_id_queue_delete_id_event, + client); break; } @@ -735,7 +788,8 @@ remove_and_destroy_resource(void *element, void *data, uint32_t flags) { struct wl_resource *resource = element; struct wl_client *client = resource->client; - uint32_t id = resource->object.id;; + uint32_t id = resource->object.id; + const struct wl_interface *interface = resource->object.interface; wl_signal_emit(&resource->deprecated_destroy_signal, resource); /* Don't emit the new signal for deprecated resources, as that would @@ -746,16 +800,12 @@ remove_and_destroy_resource(void *element, void *data, uint32_t flags) if (resource->destroy) resource->destroy(resource); + if (id < WL_SERVER_ID_START && client->display_resource) + wl_resource_queue_event(client->display_resource, + WL_DISPLAY_DELETE_ID, id); + /* The resource should be cleared from the map before memory is freed. */ - if (id < WL_SERVER_ID_START) { - if (client->display_resource) { - wl_resource_queue_event(client->display_resource, - WL_DISPLAY_DELETE_ID, id); - } - wl_map_insert_at(&client->objects, 0, id, NULL); - } else { - wl_map_remove(&client->objects, id); - } + wl_map_zombify(&client->objects, id, interface); if (!(flags & WL_MAP_ENTRY_LEGACY)) free(resource); diff --git a/src/wayland-util.c b/src/wayland-util.c index bb2a1838..1fd4fcfc 100644 --- a/src/wayland-util.c +++ b/src/wayland-util.c @@ -30,6 +30,7 @@ #include #include #include +#include #include "wayland-util.h" #include "wayland-private.h" @@ -169,20 +170,48 @@ wl_interface_equal(const struct wl_interface *a, const struct wl_interface *b) return a == b || strcmp(a->name, b->name) == 0; } -union map_entry { - uintptr_t next; +struct map_entry { + uint32_t next; + uint32_t flags : 8*sizeof(uint32_t) - 3; + uint32_t zombie : 1; + uint32_t freelisted : 1; + uint32_t deleted : 1; void *data; }; -#define map_entry_is_free(entry) ((entry).next & 0x1) -#define map_entry_get_data(entry) ((void *)((entry).next & ~(uintptr_t)0x3)) -#define map_entry_get_flags(entry) (((entry).next >> 1) & 0x1) +int32_t max_zombie_list_count = 64; + +/* We cannot use NULL (0) to denote null links in the free list or + zombie list because index 0 is allowed on the server side: id == + WL_SERVER_ID_START --> index == 0. Instead, use: */ +static const uint32_t map_null_link = ~0; + +static inline int +map_entry_is_free(struct map_entry *entry) +{ + return entry->zombie || entry->freelisted; +} + +static inline void +map_entry_clear(struct map_entry *entry) +{ + entry->next = map_null_link; + entry->flags = 0; + entry->zombie = 0; + entry->freelisted = 0; + entry->deleted = 0; + entry->data = NULL; +} void wl_map_init(struct wl_map *map, uint32_t side) { memset(map, 0, sizeof *map); map->side = side; + map->free_list = map_null_link; + map->zombie_list_head = map_null_link; + map->zombie_list_tail = map_null_link; + map->zombie_list_count = 0; } void @@ -195,7 +224,7 @@ wl_map_release(struct wl_map *map) uint32_t wl_map_insert_new(struct wl_map *map, uint32_t flags, void *data) { - union map_entry *start, *entry; + struct map_entry *start, *entry; struct wl_array *entries; uint32_t base; uint32_t count; @@ -208,9 +237,10 @@ wl_map_insert_new(struct wl_map *map, uint32_t flags, void *data) base = WL_SERVER_ID_START; } - if (map->free_list) { + if (map->free_list != map_null_link) { start = entries->data; - entry = &start[map->free_list >> 1]; + entry = &start[map->free_list]; + assert(entry->freelisted); map->free_list = entry->next; } else { entry = wl_array_add(entries, sizeof *entry); @@ -235,20 +265,21 @@ wl_map_insert_new(struct wl_map *map, uint32_t flags, void *data) errno = ENOSPC; return 0; } + map_entry_clear(entry); entry->data = data; - entry->next |= (flags & 0x1) << 1; - + entry->flags = flags; return count + base; } int wl_map_insert_at(struct wl_map *map, uint32_t flags, uint32_t i, void *data) { - union map_entry *start; + struct map_entry *start; uint32_t count; struct wl_array *entries; if (i < WL_SERVER_ID_START) { + assert(i == 0 || map->side == WL_MAP_SERVER_SIDE); entries = &map->client_entries; } else { entries = &map->server_entries; @@ -272,8 +303,10 @@ wl_map_insert_at(struct wl_map *map, uint32_t flags, uint32_t i, void *data) } start = entries->data; + + map_entry_clear(&start[i]); start[i].data = data; - start[i].next |= (flags & 0x1) << 1; + start[i].flags = flags; return 0; } @@ -281,7 +314,7 @@ wl_map_insert_at(struct wl_map *map, uint32_t flags, uint32_t i, void *data) int wl_map_reserve_new(struct wl_map *map, uint32_t i) { - union map_entry *start; + struct map_entry *start; uint32_t count; struct wl_array *entries; @@ -318,10 +351,16 @@ wl_map_reserve_new(struct wl_map *map, uint32_t i) return -1; start = entries->data; - start[i].data = NULL; + map_entry_clear(&start[i]); } else { start = entries->data; - if (start[i].data != NULL) { + + assert(!start[i].freelisted); + + /* In the new zombie regime, there may be zombies in + * any map, even opposite side ones. So a test here + * for start[i].data != NULL is no longer right.*/ + if (!map_entry_is_free(&start[i])) { errno = EINVAL; return -1; } @@ -330,34 +369,128 @@ wl_map_reserve_new(struct wl_map *map, uint32_t i) return 0; } -void -wl_map_remove(struct wl_map *map, uint32_t i) +int +wl_map_zombify(struct wl_map *map, uint32_t i, const struct wl_interface *interface) { - union map_entry *start; + struct map_entry *start, *entry; struct wl_array *entries; + bool use_zombie_list; + uint32_t count; + static bool checked_env = false; + const char *max_var; + + assert(i != 0); + + if (i < WL_SERVER_ID_START) { + use_zombie_list = false; + entries = &map->client_entries; + } else { + use_zombie_list = (map->side == WL_MAP_SERVER_SIDE) && + (map->zombie_list_count >= 0); + entries = &map->server_entries; + i -= WL_SERVER_ID_START; + } + + start = entries->data; + count = entries->size / sizeof *start; + + if (i >= count) + return -1; + + if (start[i].deleted) { + /* No need to make this entry into a zombie if it has + already been in a delete_id message - put it on the + free list instead */ + start[i].next = map->free_list; + start[i].freelisted = 1; + map->free_list = i; + return 0; + } + + start[i].data = (void*)interface; + start[i].zombie = 1; + start[i].next = map_null_link; + + if (use_zombie_list) { + if (!checked_env) { + checked_env = true; + max_var = getenv("WAYLAND_MAX_ZOMBIE_LIST_COUNT"); + if (max_var && *max_var) + max_zombie_list_count = atoi(max_var); + } + + if (map->zombie_list_tail != map_null_link) + start[map->zombie_list_tail].next = i; + else + map->zombie_list_head = i; + map->zombie_list_tail = i; + map->zombie_list_count++; + + if (map->zombie_list_count > max_zombie_list_count) { + i = map->zombie_list_head; + entry = &start[i]; + map->zombie_list_head = entry->next; + if (map->zombie_list_head == map_null_link) + map->zombie_list_tail = map_null_link; + map->zombie_list_count--; + + entry->next = map->free_list; + entry->freelisted = 1; + entry->zombie = 0; + map->free_list = i; + } + } + return 0; +} + +int +wl_map_mark_deleted(struct wl_map *map, uint32_t i) +{ + struct map_entry *start; + struct wl_array *entries; + uint32_t count; + + assert(i != 0); if (i < WL_SERVER_ID_START) { if (map->side == WL_MAP_SERVER_SIDE) - return; + return 0; entries = &map->client_entries; } else { if (map->side == WL_MAP_CLIENT_SIDE) - return; + return 0; entries = &map->server_entries; i -= WL_SERVER_ID_START; } start = entries->data; - start[i].next = map->free_list; - map->free_list = (i << 1) | 1; + count = entries->size / sizeof *start; + + if (i >= count) + return -1; + + /* turn off the zombie list - because the zombie_list is not + needed if we are receiving delete_id messages, and is + incompatible with randomly moving zombies directly to the + free list */ + map->zombie_list_count = -1; + + start[i].deleted = 1; + if (start[i].zombie) { + start[i].next = map->free_list; + start[i].freelisted = 1; + start[i].zombie = 0; + map->free_list = i; + } + return 0; } void * wl_map_lookup(struct wl_map *map, uint32_t i) { - union map_entry *start; + struct map_entry *start; uint32_t count; struct wl_array *entries; @@ -371,8 +504,31 @@ wl_map_lookup(struct wl_map *map, uint32_t i) start = entries->data; count = entries->size / sizeof *start; - if (i < count && !map_entry_is_free(start[i])) - return map_entry_get_data(start[i]); + if (i < count && !map_entry_is_free(&start[i])) + return start[i].data; + + return NULL; +} + +const struct wl_interface * +wl_map_lookup_zombie(struct wl_map *map, uint32_t i) +{ + struct map_entry *start; + uint32_t count; + struct wl_array *entries; + + if (i < WL_SERVER_ID_START) { + entries = &map->client_entries; + } else { + entries = &map->server_entries; + i -= WL_SERVER_ID_START; + } + + start = entries->data; + count = entries->size / sizeof *start; + + if (i < count && start[i].zombie) + return start[i].data; return NULL; } @@ -380,7 +536,7 @@ wl_map_lookup(struct wl_map *map, uint32_t i) uint32_t wl_map_lookup_flags(struct wl_map *map, uint32_t i) { - union map_entry *start; + struct map_entry *start; uint32_t count; struct wl_array *entries; @@ -394,8 +550,8 @@ wl_map_lookup_flags(struct wl_map *map, uint32_t i) start = entries->data; count = entries->size / sizeof *start; - if (i < count && !map_entry_is_free(start[i])) - return map_entry_get_flags(start[i]); + if (i < count && !map_entry_is_free(&start[i])) + return start[i].flags; return 0; } @@ -404,16 +560,16 @@ static enum wl_iterator_result for_each_helper(struct wl_array *entries, wl_iterator_func_t func, void *data) { enum wl_iterator_result ret = WL_ITERATOR_CONTINUE; - union map_entry entry, *start; + struct map_entry entry, *start; size_t count; - start = (union map_entry *) entries->data; - count = entries->size / sizeof(union map_entry); + start = (struct map_entry *) entries->data; + count = entries->size / sizeof(struct map_entry); for (size_t idx = 0; idx < count; idx++) { entry = start[idx]; - if (entry.data && !map_entry_is_free(entry)) { - ret = func(map_entry_get_data(entry), data, map_entry_get_flags(entry)); + if (entry.data && !map_entry_is_free(&entry)) { + ret = func(entry.data, data, entry.flags); if (ret != WL_ITERATOR_CONTINUE) break; } diff --git a/tests/connection-test.c b/tests/connection-test.c index 9762e0da..f97936d8 100644 --- a/tests/connection-test.c +++ b/tests/connection-test.c @@ -393,6 +393,7 @@ demarshal(struct marshal_data *data, const char *format, assert(closure); wl_closure_invoke(closure, WL_CLOSURE_INVOKE_SERVER, &object, 0, data); wl_closure_destroy(closure); + wl_map_release(&objects); } TEST(connection_demarshal) @@ -476,6 +477,7 @@ marshal_demarshal(struct marshal_data *data, assert(closure); wl_closure_invoke(closure, WL_CLOSURE_INVOKE_SERVER, &object, 0, data); wl_closure_destroy(closure); + wl_map_release(&objects); } TEST(connection_marshal_demarshal) @@ -540,6 +542,7 @@ expected_fail_demarshal(struct marshal_data *data, const char *format, assert(closure == NULL); assert(errno == expected_error); + wl_map_release(&objects); } TEST(connection_demarshal_null_strings) diff --git a/tests/map-test.c b/tests/map-test.c index 03568ea7..77088c85 100644 --- a/tests/map-test.c +++ b/tests/map-test.c @@ -89,7 +89,8 @@ TEST(map_remove) assert(wl_map_lookup(&map, j) == &b); assert(wl_map_lookup(&map, k) == &c); - wl_map_remove(&map, j); + wl_map_mark_deleted(&map, j); + wl_map_zombify(&map, j, NULL); assert(wl_map_lookup(&map, j) == NULL); /* Verify that we insert d at the hole left by removing b */ @@ -107,15 +108,18 @@ TEST(map_flags) wl_map_init(&map, WL_MAP_SERVER_SIDE); i = wl_map_insert_new(&map, 0, &a); - j = wl_map_insert_new(&map, 1, &b); + uint32_t flag_value = 0xabcdef10; + /* 3 bits of flags are used internally, so we lose the high 3 here: */ + uint32_t high_truncated_flag_value = (flag_value << 3) >> 3; + j = wl_map_insert_new(&map, high_truncated_flag_value, &b); assert(i == WL_SERVER_ID_START); assert(j == WL_SERVER_ID_START + 1); assert(wl_map_lookup(&map, i) == &a); assert(wl_map_lookup(&map, j) == &b); - assert(wl_map_lookup_flags(&map, i) == 0); - assert(wl_map_lookup_flags(&map, j) == 1); + assert(wl_map_lookup_flags(&map, i) == 0); + assert(wl_map_lookup_flags(&map, j) == high_truncated_flag_value); wl_map_release(&map); } From 3cd9ca8a65d177e69697453f6b49c82967127d55 Mon Sep 17 00:00:00 2001 From: Jonathan Leivent Date: Fri, 26 Jan 2024 18:48:10 -0500 Subject: [PATCH 3/4] tests: new map_zombie_list and map_mark_deleted Signed-off-by: Jonathan Leivent --- tests/map-test.c | 107 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 107 insertions(+) diff --git a/tests/map-test.c b/tests/map-test.c index 77088c85..c8e1340e 100644 --- a/tests/map-test.c +++ b/tests/map-test.c @@ -30,6 +30,113 @@ #include "wayland-private.h" #include "test-runner.h" +TEST(map_zombie_list) +{ + struct wl_map map; + uint32_t i, j, k, l, m, n, a, b, c, d, e, f; + struct wl_interface az, bz, cz; + + /* This setenv has to happen before any wl_map_zombify calls: */ + assert(setenv("WAYLAND_MAX_ZOMBIE_LIST_COUNT","2",1) == 0); + + wl_map_init(&map, WL_MAP_SERVER_SIDE); + i = wl_map_insert_new(&map, 0, &a); + j = wl_map_insert_new(&map, 0, &b); + k = wl_map_insert_new(&map, 0, &c); + assert(i == WL_SERVER_ID_START); + assert(j == WL_SERVER_ID_START + 1); + assert(k == WL_SERVER_ID_START + 2); + + assert(wl_map_lookup(&map, i) == &a); + assert(wl_map_lookup_zombie(&map, i) == NULL); + assert(wl_map_lookup(&map, j) == &b); + assert(wl_map_lookup_zombie(&map, j) == NULL); + assert(wl_map_lookup(&map, k) == &c); + assert(wl_map_lookup_zombie(&map, k) == NULL); + + + assert(map.zombie_list_count == 0); + + assert(wl_map_zombify(&map, WL_SERVER_ID_START + 3, NULL) != 0); + + assert(wl_map_zombify(&map, i, &az) == 0); + assert(wl_map_lookup(&map, i) == NULL); + assert(wl_map_lookup_zombie(&map, i) == &az); + assert(map.zombie_list_count == 1); + + l = wl_map_insert_new(&map, 0, &d); + assert(l == WL_SERVER_ID_START + 3); + assert(wl_map_lookup(&map, l) == &d); + assert(map.zombie_list_count == 1); + + assert(wl_map_zombify(&map, j, &bz) == 0); + assert(wl_map_lookup(&map, j) == NULL); + assert(wl_map_lookup_zombie(&map, j) == &bz); + assert(map.zombie_list_count == 2); + + m = wl_map_insert_new(&map, 0, &e); + assert(m == WL_SERVER_ID_START + 4); + assert(wl_map_lookup(&map, m) == &e); + assert(map.zombie_list_count == 2); + + assert(wl_map_zombify(&map, k, &cz) == 0); + assert(wl_map_lookup(&map, k) == NULL); + assert(wl_map_lookup_zombie(&map, k) == &cz); + assert(map.zombie_list_count == 2); + + n = wl_map_insert_new(&map, 0, &f); + assert(n == WL_SERVER_ID_START); + assert(wl_map_lookup(&map, n) == &f); + assert(map.zombie_list_count == 2); + + wl_map_release(&map); +} + +TEST(map_mark_deleted) +{ + struct wl_map map; + uint32_t i, j, k, a, b, c; + struct wl_interface az, bz; + + wl_map_init(&map, WL_MAP_SERVER_SIDE); + assert(wl_map_mark_deleted(&map, WL_SERVER_ID_START) != 0); + i = wl_map_insert_new(&map, 0, &a); + assert(i == WL_SERVER_ID_START); + assert(map.zombie_list_count == 0); + + assert(wl_map_lookup(&map, i) == &a); + + assert(wl_map_mark_deleted(&map, i) == 0); + assert(map.zombie_list_count == -1); /* turned off by above */ + assert(wl_map_lookup(&map, i) == &a); + assert(wl_map_lookup_zombie(&map, i) == NULL); + + assert(wl_map_zombify(&map, i, &az) == 0); + assert(wl_map_lookup(&map, i) == NULL); + assert(wl_map_lookup_zombie(&map, i) == NULL); + assert(map.zombie_list_count == -1); + + + j = wl_map_insert_new(&map, 0, &b); + assert(j == WL_SERVER_ID_START); + assert(wl_map_lookup(&map, j) == &b); + + assert(wl_map_zombify(&map, j, &bz) == 0); + assert(map.zombie_list_count == -1); + assert(wl_map_lookup(&map, j) == NULL); + assert(wl_map_lookup_zombie(&map, j) == &bz); + + assert(wl_map_mark_deleted(&map, j) == 0); + assert(wl_map_lookup(&map, j) == NULL); + assert(wl_map_lookup_zombie(&map, j) == NULL); + + + k = wl_map_insert_new(&map, 0, &c); + assert(k == WL_SERVER_ID_START); + + wl_map_release(&map); +} + TEST(map_insert_new) { struct wl_map map; From aff270293ba72cd1794737c06f54f5c49e1aff21 Mon Sep 17 00:00:00 2001 From: Jonathan Leivent Date: Fri, 26 Jan 2024 19:37:55 -0500 Subject: [PATCH 4/4] client/server: add delete_id request as a fake sync Add a delete_id request so that the server can reap zombies once the client acks the deletion. As a result, the wl_map zombie_list would only be used for clients that do not have this change. To keep compatibility, the delete_id request is dressed as a sync request, but carries a server object ID instead. Update tests to compensate for additional handshake that sets up delete_id requests. Signed-off-by: Jonathan Leivent --- src/wayland-client.c | 89 ++++++++++++++++++++++++++++++++++-- src/wayland-private.h | 4 ++ src/wayland-server.c | 78 +++++++++++++++++++++++++++++-- src/wayland-util.c | 9 +++- tests/connection-test.c | 5 ++ tests/protocol-logger-test.c | 7 +++ 6 files changed, 185 insertions(+), 7 deletions(-) diff --git a/src/wayland-client.c b/src/wayland-client.c index 6f01fa99..94173f03 100644 --- a/src/wayland-client.c +++ b/src/wayland-client.c @@ -104,6 +104,7 @@ struct wl_display { int reader_count; uint32_t read_serial; pthread_cond_t reader_cond; + bool use_sync_for_delete_id_requests; }; /** \endcond */ @@ -495,11 +496,74 @@ wl_proxy_create_for_id(struct wl_proxy *factory, return proxy; } +/* In the current protocol, the client has no equivalent to a + * wl_display delete_id event. But it can benefit from one. Here we + * hijack the wl_display sync request such that the client will send a + * sync with a server id instead of a client id, which will indicate + * it is really being used as a delete_id request. The client will + * only do this if the server had previously sent a delete_id event + * for WL_DELETE_ID_HANDSHAKE, indicating it would accept such + * hijacked syncs. */ +static void +marshal_fake_sync_for_delete_id(struct wl_display *display, uint32_t id) +{ + if (id < WL_SERVER_ID_START) { + wl_log("error: marshal_fake_sync_for_delete_id on client id %u\n", id); + return; + } + + /* We have to call wl_closure_marshal directly because we + * already have the display mutex */ + + struct wl_proxy *proxy = &display->proxy; + const struct wl_message *message = + &proxy->object.interface->methods[WL_DISPLAY_SYNC]; + + union wl_argument arg; + struct wl_closure *closure; + /* Even though the sync request takes a new_id arg, + * wl_closure_marshal wants an obj arg for the new_id arg, + * meaning it wants an actual object instead of an id. So we + * fake up an object. Fortunately, it only uses the id + * field. */ + struct wl_object fake_obj; + fake_obj.interface = NULL; + fake_obj.implementation = NULL; + fake_obj.id = id; + arg.o = &fake_obj; + closure = wl_closure_marshal(&proxy->object, WL_DISPLAY_SYNC, &arg, message); + + if (closure == NULL) { + wl_log("Error marshalling request: %s\n", strerror(errno)); + display_fatal_error(display, errno); + return; + } + + if (debug_client) { + struct wl_event_queue *queue; + + queue = wl_proxy_get_queue(proxy); + wl_closure_print(closure, &proxy->object, true, false, NULL, + wl_event_queue_get_name(queue)); + } + + if (wl_closure_send(closure, display->connection)) { + wl_log("Error sending request: %s\n", strerror(errno)); + display_fatal_error(display, errno); + } + + wl_closure_destroy(closure); +} + static void proxy_destroy(struct wl_proxy *proxy) { - wl_map_zombify(&proxy->display->objects, - proxy->object.id, + struct wl_display *display = proxy->display; + uint32_t id = proxy->object.id; + if (display->use_sync_for_delete_id_requests && id >= WL_SERVER_ID_START) + marshal_fake_sync_for_delete_id(display, id); + + wl_map_zombify(&display->objects, id, proxy->object.interface); proxy->flags |= WL_PROXY_FLAG_DESTROYED; @@ -1031,8 +1095,23 @@ display_handle_delete_id(void *data, struct wl_display *display, uint32_t id) { pthread_mutex_lock(&display->mutex); - if (wl_map_mark_deleted(&display->objects, id) != 0) + if (id == WL_DELETE_ID_HANDSHAKE) { + /* The server has sent a special "impossible" delete_id event + * to indicate that it can accept delete_id requests. Note + * that in the display, and reply with a similar delete_id + * request */ + /* The client doesn't have to return the handshake. + * If it doesn't, the server will learn that the + * client can send delete_id requests the first time + * the client does so, if it does so. But that might + * be after the server has begun to recycle its IDs. + * Unless the server has a large enough zombie + * ring. */ + display->use_sync_for_delete_id_requests = true; + marshal_fake_sync_for_delete_id(display, id); + } else if (wl_map_mark_deleted(&display->objects, id) != 0) { wl_log("error: received delete_id for unknown id (%u)\n", id); + } pthread_mutex_unlock(&display->mutex); } @@ -1144,6 +1223,7 @@ wl_display_connect_to_fd(int fd) pthread_mutex_init(&display->mutex, NULL); pthread_cond_init(&display->reader_cond, NULL); display->reader_count = 0; + display->use_sync_for_delete_id_requests = false; if (wl_map_insert_at(&display->objects, 0, 0, NULL) == -1) goto err_connection; @@ -1472,6 +1552,9 @@ zombify_new_id(void *displayp, uint32_t id, const struct wl_interface *interface } assert(wl_map_lookup_zombie(&display->objects, id) == interface); + if (display->use_sync_for_delete_id_requests) + marshal_fake_sync_for_delete_id(display, id); + return 0; } else { /* Unexpected: the id is a client ID */ diff --git a/src/wayland-private.h b/src/wayland-private.h index eb67dc5e..0205b96d 100644 --- a/src/wayland-private.h +++ b/src/wayland-private.h @@ -47,6 +47,7 @@ #define WL_SERVER_ID_START 0xff000000 #define WL_MAP_MAX_OBJECTS 0x00f00000 #define WL_CLOSURE_MAX_ARGS 20 +#define WL_DELETE_ID_HANDSHAKE 0xffffffff struct wl_object { const struct wl_interface *interface; @@ -100,6 +101,9 @@ wl_map_reserve_new(struct wl_map *map, uint32_t i); int wl_map_zombify(struct wl_map *map, uint32_t i, const struct wl_interface *interface); +void +wl_map_disable_zombie_list(struct wl_map *map); + int wl_map_mark_deleted(struct wl_map *map, uint32_t i); diff --git a/src/wayland-server.c b/src/wayland-server.c index 38427f39..0321f8c7 100644 --- a/src/wayland-server.c +++ b/src/wayland-server.c @@ -359,6 +359,22 @@ zombify_new_id_queue_delete_id_event(void *clientp, uint32_t id, } } +/* Only used for demarshalling wl_display sync messages */ +static int +handle_new_id_for_sync(void *data, uint32_t id, const struct wl_interface *interface) +{ + struct wl_map *objects = data; + + if (id >= WL_SERVER_ID_START) + /* Assume this is being called do to a sync request + * that is really a delete_id request - which will be + * handled in display_sync. + */ + return 0; + + return wl_map_reserve_new(objects, id); +} + static int wl_client_connection_data(int fd, uint32_t mask, void *data) { @@ -467,9 +483,24 @@ wl_client_connection_data(int fd, uint32_t mask, void *data) break; } - - closure = wl_connection_demarshal(client->connection, size, - &client->objects, message); + if (resource == client->display_resource && + strcmp(message->name,"sync") == 0) + /* This is a wl_display::sync message, which + * might be being used as a delete_id request. + * Avoid declaring a server ID in the new_ID + * field as an error by using + * handle_new_id_for_sync as the new_id + * handler. Would be nice to use opcode == + * WL_DISPLAY_SYNC, but WL_DISPLAY_SYNC only + * gets defined for clients. + */ + closure = wl_connection_demarshal_common(client->connection, size, + &client->objects, message, + &handle_new_id_for_sync, + &client->objects); + else + closure = wl_connection_demarshal(client->connection, size, + &client->objects, message); if (closure == NULL && errno == ENOMEM) { wl_resource_post_no_memory(resource); @@ -575,6 +606,8 @@ WL_EXPORT struct wl_client * wl_client_create(struct wl_display *display, int fd) { struct wl_client *client; + bool do_delete_id_handshake = false; + const char *env; client = zalloc(sizeof *client); if (client == NULL) @@ -611,6 +644,28 @@ wl_client_create(struct wl_display *display, int fd) wl_priv_signal_emit(&display->create_client_signal, client); + env = getenv("WAYLAND_SERVER_DELETE_ID_HANDSHAKE"); + if (env && (strcmp(env,"0")==0 || strcasecmp(env,"no")==0 || + strcasecmp(env,"false")==0)) + do_delete_id_handshake = false; + else + do_delete_id_handshake = true; + + if (do_delete_id_handshake) { + /* Send the client a fake delete_id event with id + * WL_DELETE_ID_HANDSHAKE so that a patched client + * will know this is a patched server that can handle + * delete_id requests. Unpatched clients will log + * this but otherwise ignore it. Patched clients will + * reply with the same id in a delete_id request(!), + * to indicate that they will take the responsibility + * to send future delete_id requests. + */ + wl_resource_queue_event(client->display_resource, + WL_DISPLAY_DELETE_ID, + WL_DELETE_ID_HANDSHAKE); + } + return client; err_map: @@ -1079,6 +1134,23 @@ display_sync(struct wl_client *client, struct wl_resource *callback; uint32_t serial; + if (id >= WL_SERVER_ID_START) { + /* This is not a sync request. It's really a + delete_id request. A true sync request would have + a client id instead. */ + if (id == WL_DELETE_ID_HANDSHAKE) { + /* the id == WL_DELETE_ID_HANDSHAKE case is only to tell + * us that the client will send other delete_id requests + * later + */ + wl_map_disable_zombie_list(&client->objects); + } + else if (wl_map_mark_deleted(&client->objects, id) != 0) + wl_log("error: received delete_id for unknown id (%u)\n", id); + + return; + } + callback = wl_resource_create(client, &wl_callback_interface, 1, id); if (callback == NULL) { wl_client_post_no_memory(client); diff --git a/src/wayland-util.c b/src/wayland-util.c index 1fd4fcfc..0a5dbb23 100644 --- a/src/wayland-util.c +++ b/src/wayland-util.c @@ -443,6 +443,13 @@ wl_map_zombify(struct wl_map *map, uint32_t i, const struct wl_interface *interf return 0; } + +void +wl_map_disable_zombie_list(struct wl_map *map) +{ + map->zombie_list_count = -1; +} + int wl_map_mark_deleted(struct wl_map *map, uint32_t i) { @@ -475,7 +482,7 @@ wl_map_mark_deleted(struct wl_map *map, uint32_t i) needed if we are receiving delete_id messages, and is incompatible with randomly moving zombies directly to the free list */ - map->zombie_list_count = -1; + wl_map_disable_zombie_list(map); start[i].deleted = 1; if (start[i].zombie) { diff --git a/tests/connection-test.c b/tests/connection-test.c index f97936d8..35bab09c 100644 --- a/tests/connection-test.c +++ b/tests/connection-test.c @@ -818,6 +818,11 @@ TEST(request_bogus_size) test_set_timeout(1); + /* Because this test reads the expected error off the wire + directly without anticipating any prior event from the + server, turn off the delete_id handshake event: */ + assert(setenv("WAYLAND_SERVER_DELETE_ID_HANDSHAKE","0",1) == 0); + /* * The manufactured message has real size 12. Test all bogus sizes * smaller than that, and zero as the last one since wl_closure_init diff --git a/tests/protocol-logger-test.c b/tests/protocol-logger-test.c index a0ebd22a..dcf1a126 100644 --- a/tests/protocol-logger-test.c +++ b/tests/protocol-logger-test.c @@ -59,6 +59,13 @@ struct message { const char *message_name; int args_count; } messages[] = { + { /* this is the WAYLAND_SERVER_DELETE_ID_HANDSHAKE message */ + .type = WL_PROTOCOL_LOGGER_EVENT, + .class = "wl_display", + .opcode = 1, + .message_name = "delete_id", + .args_count = 1, + }, { .type = WL_PROTOCOL_LOGGER_REQUEST, .class = "wl_display",