From 442648edc09f3e8d39766eb4776de3534a477caa Mon Sep 17 00:00:00 2001
From: Vlad Zahorodnii <vlad.zahorodnii@kde.org>
Date: Wed, 20 Mar 2024 13:09:23 +0200
Subject: [PATCH] wayland: Remove zombie ClientConnection from Display later

Otherwise it's theoretically possible to create a new ClientConnection
object for the zombie wl_client when its resources are being destroyed.
For example

- process early wl_client destroy notification
- the ClientConnection objects gets removed from the client list in Display
- process wl_resource objects getting destroyed
- if some code calls display->getConnection(zombie_client), it's going to
  reintroduce the client in the client list
- process late wl_client destroy notification, it's going to destroy the
  original and the clone ClientConnection object

This change prevents reintoducing a clone client object, by keeping the
original for a bit longer until it's actually destroyed. In the future though,
it would be great to kill the client lists in Display and ClientConnection,
and just use `static_cast<ClientConnection *>(wl_client_get_user_data())`.
---
 autotests/wayland/server/test_display.cpp |  8 --------
 src/wayland/display.cpp                   | 13 ++++---------
 src/wayland/display.h                     |  1 -
 3 files changed, 4 insertions(+), 18 deletions(-)

diff --git a/autotests/wayland/server/test_display.cpp b/autotests/wayland/server/test_display.cpp
index 63ab73f86d..6887fb8cce 100644
--- a/autotests/wayland/server/test_display.cpp
+++ b/autotests/wayland/server/test_display.cpp
@@ -81,7 +81,6 @@ void TestWaylandServerDisplay::testClientConnection()
     QVERIFY(client);
 
     QVERIFY(connectedSpy.isEmpty());
-    QVERIFY(display.connections().isEmpty());
     ClientConnection *connection = display.getConnection(client);
     QVERIFY(connection);
     QCOMPARE(connection->client(), client);
@@ -101,8 +100,6 @@ void TestWaylandServerDisplay::testClientConnection()
     QCOMPARE((wl_client *)constRef, client);
     QCOMPARE(connectedSpy.count(), 1);
     QCOMPARE(connectedSpy.first().first().value<ClientConnection *>(), connection);
-    QCOMPARE(display.connections().count(), 1);
-    QCOMPARE(display.connections().first(), connection);
 
     QCOMPARE(connection, display.getConnection(client));
     QCOMPARE(connectedSpy.count(), 1);
@@ -119,10 +116,6 @@ void TestWaylandServerDisplay::testClientConnection()
     QCOMPARE(connectedSpy.first().first().value<ClientConnection *>(), connection);
     QCOMPARE(connectedSpy.last().first().value<ClientConnection *>(), connection2);
     QCOMPARE(connectedSpy.last().first().value<ClientConnection *>(), client2);
-    QCOMPARE(display.connections().count(), 2);
-    QCOMPARE(display.connections().first(), connection);
-    QCOMPARE(display.connections().last(), connection2);
-    QCOMPARE(display.connections().last(), client2);
 
     // and destroy
     QVERIFY(disconnectedSpy.isEmpty());
@@ -136,7 +129,6 @@ void TestWaylandServerDisplay::testClientConnection()
     close(sv[1]);
     close(sv2[0]);
     close(sv2[1]);
-    QVERIFY(display.connections().isEmpty());
 }
 
 void TestWaylandServerDisplay::testConnectNoSocket()
diff --git a/src/wayland/display.cpp b/src/wayland/display.cpp
index 7b2420ae77..8cf8b23794 100644
--- a/src/wayland/display.cpp
+++ b/src/wayland/display.cpp
@@ -199,6 +199,7 @@ QList<SeatInterface *> Display::seats() const
 
 ClientConnection *Display::getConnection(wl_client *client)
 {
+    // TODO: Use wl_client_set_user_data() when we start requiring libwayland-server that has it, and remove client lists here and in ClientConnection.
     Q_ASSERT(client);
     auto it = std::find_if(d->clients.constBegin(), d->clients.constEnd(), [client](ClientConnection *c) {
         return c->client() == client;
@@ -210,21 +211,15 @@ ClientConnection *Display::getConnection(wl_client *client)
     auto c = new ClientConnection(client, this);
     d->clients << c;
     connect(c, &ClientConnection::disconnected, this, [this](ClientConnection *c) {
-        const int index = d->clients.indexOf(c);
-        Q_ASSERT(index != -1);
-        d->clients.remove(index);
-        Q_ASSERT(d->clients.indexOf(c) == -1);
         Q_EMIT clientDisconnected(c);
     });
+    connect(c, &ClientConnection::destroyed, this, [this, c]() {
+        d->clients.removeOne(c);
+    });
     Q_EMIT clientConnected(c);
     return c;
 }
 
-QList<ClientConnection *> Display::connections() const
-{
-    return d->clients;
-}
-
 ClientConnection *Display::createClient(int fd)
 {
     Q_ASSERT(fd != -1);
diff --git a/src/wayland/display.h b/src/wayland/display.h
index d1e04f1faa..62be0cbee3 100644
--- a/src/wayland/display.h
+++ b/src/wayland/display.h
@@ -110,7 +110,6 @@ public:
      * @return The ClientConnection for the given native client
      */
     ClientConnection *getConnection(wl_client *client);
-    QList<ClientConnection *> connections() const;
 
     /**
      * Returns the graphics buffer for the given @a resource, or @c null if there's no buffer.