Mitigate UAF crashes due to wl_client_destroy reentrancy

There are situations in which a call into wl_client_destroy() can
result in a reentrant call into wl_client_destroy() - which
results in UAF / double free crashes.

For example, this can occur in the following scenario.

1. Server receives a message notifying it that a client has
   disconnected (WL_EVENT_HANGUP [1])

2. This beings client destruction with a call to wl_client_destroy()

3. wl_client_destroy() kicks off callbacks as client-associated
   resources are cleaned up and their destructors and destruction
   signals are invoked.

4. These callbacks eventually lead to an explicit call to
   wl_display_flush_clients() as the server attempts to flush
   events to other connected clients.

5. Since the client has already begun destruction, when it is
   reached in the iteration the flush fails wl_client_destroy()
   is called again [2].

This patch guards against this reentrant condition by removing
the client from the display's client list when wl_client_destroy()
is first called. This prevents access / iteration over the client
after wl_client_destroy() is called.

In the example above, wl_display_flush_clients() will pass over
the client currently undergoing destruction and the reentrant
call is avoided.

[1] https://gitlab.freedesktop.org/wayland/wayland/-/blob/8f499bf4045f88f3a4b4b0a445befca467bebe20/src/wayland-server.c#L342

[2] https://gitlab.freedesktop.org/wayland/wayland/-/blob/8f499bf4045f88f3a4b4b0a445befca467bebe20/src/wayland-server.c#L1512

Change-Id: Ie1fc978f3c45e706b2f846428819852783020f69
Signed-off-by: Thomas Lukaszewicz [thomaslukaszewicz@gmail.com](mailto:thomaslukaszewicz@gmail.com)

diff --git a/src/wayland-server.c b/src/wayland-server.c
index 12db67b..6ed1558 100644 (file)
--- a/src/wayland-server.c
+++ b/src/wayland-server.c
@@ -1155,6 +1155,23 @@ wl_client_destroy(struct wl_client *client)
 {
        struct wl_display *display = client->display;
 
+       /* wl_client_destroy() should not be called twice for the same client. */
+       if (wl_list_empty(&client->link)) {
+               client->error = 1;
+               wl_log("wl_client_destroy: encountered re-entrant client destruction.\n");
+               return;
+       }
+
+//TIZEN_ONLY(20240626) : mutex lock/unlock when removing client from the client_list
+       pthread_mutex_lock(&display->client_list_mutex);
+//TIZEN_ONLY : END
+       wl_list_remove(&client->link);
+       /* Keep the client link safe to inspect. */
+       wl_list_init(&client->link);
+//TIZEN_ONLY(20240626) : mutex lock/unlock when removing client from the client_list
+       pthread_mutex_unlock(&display->client_list_mutex);
+//TIZEN_ONLY : END
+
        wl_priv_signal_final_emit(&client->destroy_signal, client);
 
        wl_client_flush(client);
@@ -1165,14 +1182,6 @@ wl_client_destroy(struct wl_client *client)
 
        wl_priv_signal_final_emit(&client->destroy_late_signal, client);
 
-//TIZEN_ONLY(20240626) : mutex lock/unlock when removing client from the client_list
-       pthread_mutex_lock(&display->client_list_mutex);
-//TIZEN_ONLY : END
-       wl_list_remove(&client->link);
-//TIZEN_ONLY(20240626) : mutex lock/unlock when removing client from the client_list
-       pthread_mutex_unlock(&display->client_list_mutex);
-//TIZEN_ONLY : END
-
        wl_list_remove(&client->resource_created_signal.listener_list);
 //TIZEN_ONLY(20230518) : Add mutex for wl_client's resources
        pthread_mutex_destroy(&client->objects_mutex);

diff --git a/tests/client-test.c b/tests/client-test.c
index 47be83f..40773c0 100644 (file)
--- a/tests/client-test.c
+++ b/tests/client-test.c
@@ -143,3 +143,50 @@ TEST(client_destroy_listener)
        wl_display_destroy(display);
 }
 
+static void
+client_destroy_remove_link_notify(struct wl_listener *l, void *data)
+{
+       struct wl_client *client = data;
+       struct client_destroy_listener *listener =
+               wl_container_of(l, listener, listener);
+
+       /* The client destruction signal should not be emitted more than once. */
+       assert(!listener->done);
+       listener->done = true;
+
+       /* The client should have been removed from the display's list. */
+       assert(wl_list_empty(wl_client_get_link(client)));
+}
+
+/*
+ * Tests that wl_client_destroy() will remove the client from the display's
+ * client list to prevent client access during destruction.
+ */
+TEST(client_destroy_removes_link)
+{
+       struct wl_display *display;
+       struct wl_client *client;
+       struct client_destroy_listener destroy_listener;
+       int s[2];
+
+       assert(socketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0, s) == 0);
+       display = wl_display_create();
+       assert(display);
+       client = wl_client_create(display, s[0]);
+       assert(client);
+
+       destroy_listener.listener.notify = client_destroy_remove_link_notify;
+       destroy_listener.done = false;
+       wl_client_add_destroy_listener(client, &destroy_listener.listener);
+
+       assert(wl_client_get_destroy_listener(client,
+               client_destroy_remove_link_notify) == &destroy_listener.listener);
+
+       wl_client_destroy(client);
+       assert(destroy_listener.done);
+
+       close(s[0]);
+       close(s[1]);
+
+       wl_display_destroy(display);
+}