From aff270293ba72cd1794737c06f54f5c49e1aff21 Mon Sep 17 00:00:00 2001 From: Jonathan Leivent Date: Fri, 26 Jan 2024 19:37:55 -0500 Subject: [PATCH] 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",