From 0649c8e51e874dc607daabf83ecf294611740c8f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Martin=20Gr=C3=A4=C3=9Flin?= <mgraesslin@kde.org>
Date: Mon, 10 Oct 2016 13:41:42 +0200
Subject: [PATCH] [server] Use deleteLater when a ClientConnection gets
 destroyed

Summary:
In the situation that a wl_client gets destroyed while still
wl_resources are around it can happen that one of them calls into the
ClientConnection during the cleanup handling which gets triggered at the
same time. This can then trigger a crash.

This change uses deleteLater for the ClientConnection and sets the hold
wl_client pointer to null instead of deleting directly. So the
ClientConnection is still around while the Resources gets cleaned up.
This is similar to the cleanup of Resource where on unbind the
wl_resource pointer is set to null and the Resource gets delete later.

BUG: 370232
FIXED-IN: 5.28

Reviewers: #plasma, bshah

Subscribers: plasma-devel

Tags: #plasma_on_wayland

Differential Revision: https://phabricator.kde.org/D3004
---
 .../autotests/client/test_wayland_shell.cpp   | 42 +++++++++++++++++++
 src/wayland/autotests/server/test_display.cpp |  3 ++
 src/wayland/clientconnection.cpp              | 23 ++++++++--
 3 files changed, 65 insertions(+), 3 deletions(-)

diff --git a/src/wayland/autotests/client/test_wayland_shell.cpp b/src/wayland/autotests/client/test_wayland_shell.cpp
index 1a1070818d..ea9ff912f4 100644
--- a/src/wayland/autotests/client/test_wayland_shell.cpp
+++ b/src/wayland/autotests/client/test_wayland_shell.cpp
@@ -65,6 +65,7 @@ private Q_SLOTS:
     void testResize();
     void testDisconnect();
     void testWhileDestroying();
+    void testClientDisconnecting();
 
 private:
     KWayland::Server::Display *m_display;
@@ -795,5 +796,46 @@ void TestWaylandShell::testWhileDestroying()
     QVERIFY(clientErrorSpy.isEmpty());
 }
 
+void TestWaylandShell::testClientDisconnecting()
+{
+    // this test tries to request a new surface size while the client is actually already destroyed
+    // see BUG: 370232
+    using namespace KWayland::Client;
+    using namespace KWayland::Server;
+    // create ShellSurface
+    QScopedPointer<Surface> s(m_compositor->createSurface());
+    QSignalSpy shellSurfaceCreatedSpy(m_shellInterface, &ShellInterface::surfaceCreated);
+    QVERIFY(shellSurfaceCreatedSpy.isValid());
+    QScopedPointer<ShellSurface> ps(m_shell->createSurface(s.data()));
+    QVERIFY(shellSurfaceCreatedSpy.wait());
+
+    auto serverShellSurface = shellSurfaceCreatedSpy.first().first().value<ShellSurfaceInterface*>();
+    QVERIFY(serverShellSurface);
+
+    QSignalSpy shellSurfaceUnboundSpy(serverShellSurface, &Resource::unbound);
+    QVERIFY(shellSurfaceUnboundSpy.isValid());
+
+    QScopedPointer<Surface> s2(m_compositor->createSurface());
+    QScopedPointer<ShellSurface> ps2(m_shell->createSurface(s2.data()));
+    QVERIFY(shellSurfaceCreatedSpy.wait());
+    auto serverShellSurface2 = shellSurfaceCreatedSpy.last().first().value<ShellSurfaceInterface*>();
+    QVERIFY(serverShellSurface2);
+
+    connect(serverShellSurface, &Resource::unbound, this,
+        [serverShellSurface, serverShellSurface2] {
+            serverShellSurface2->requestSize(QSize(100, 200));
+        }
+    );
+
+    m_connection->deleteLater();
+    m_connection = nullptr;
+    m_thread->quit();
+    m_thread->wait();
+    delete m_thread;
+    m_thread = nullptr;
+
+    QVERIFY(shellSurfaceUnboundSpy.wait());
+}
+
 QTEST_GUILESS_MAIN(TestWaylandShell)
 #include "test_wayland_shell.moc"
diff --git a/src/wayland/autotests/server/test_display.cpp b/src/wayland/autotests/server/test_display.cpp
index d3a595e7ba..3421577761 100644
--- a/src/wayland/autotests/server/test_display.cpp
+++ b/src/wayland/autotests/server/test_display.cpp
@@ -170,7 +170,10 @@ void TestWaylandServerDisplay::testClientConnection()
     QVERIFY(disconnectedSpy.isEmpty());
     wl_client_destroy(client);
     QCOMPARE(disconnectedSpy.count(), 1);
+    QSignalSpy clientDestroyedSpy(client2, &QObject::destroyed);
+    QVERIFY(clientDestroyedSpy.isValid());
     client2->destroy();
+    QVERIFY(clientDestroyedSpy.wait());
     QCOMPARE(disconnectedSpy.count(), 2);
     close(sv[0]);
     close(sv[1]);
diff --git a/src/wayland/clientconnection.cpp b/src/wayland/clientconnection.cpp
index 07e2b46780..2d0b019f86 100644
--- a/src/wayland/clientconnection.cpp
+++ b/src/wayland/clientconnection.cpp
@@ -66,7 +66,9 @@ ClientConnection::Private::Private(wl_client *c, Display *display, ClientConnect
 
 ClientConnection::Private::~Private()
 {
-    wl_list_remove(&listener.link);
+    if (client) {
+        wl_list_remove(&listener.link);
+    }
     s_allClients.removeAt(s_allClients.indexOf(this));
 }
 
@@ -80,9 +82,12 @@ void ClientConnection::Private::destroyListenerCallback(wl_listener *listener, v
         }
     );
     Q_ASSERT(it != s_allClients.constEnd());
-    auto q = (*it)->q;
+    auto p = (*it);
+    auto q = p->q;
+    p->client = nullptr;
+    wl_list_remove(&p->listener.link);
     emit q->disconnected(q);
-    delete q;
+    q->deleteLater();
 }
 
 ClientConnection::ClientConnection(wl_client *c, Display *parent)
@@ -95,21 +100,33 @@ ClientConnection::~ClientConnection() = default;
 
 void ClientConnection::flush()
 {
+    if (!d->client) {
+        return;
+    }
     wl_client_flush(d->client);
 }
 
 void ClientConnection::destroy()
 {
+    if (!d->client) {
+        return;
+    }
     wl_client_destroy(d->client);
 }
 
 wl_resource *ClientConnection::createResource(const wl_interface *interface, quint32 version, quint32 id)
 {
+    if (!d->client) {
+        return nullptr;
+    }
     return wl_resource_create(d->client, interface, version, id);
 }
 
 wl_resource *ClientConnection::getResource(quint32 id)
 {
+    if (!d->client) {
+        return nullptr;
+    }
     return wl_client_get_object(d->client, id);
 }