From d1a09838e1791572aa2b38d8c3e740b59fb5f97c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Martin=20Gr=C3=A4=C3=9Flin?= <mgraesslin@kde.org>
Date: Wed, 25 May 2016 08:59:10 +0200
Subject: [PATCH] [server] Prevent double delete of callback resources in
 SurfaceInterface

Summary:
When destroying a SurfaceInterface all callbacks are getting destroyed.
This used to iterate over the callbacks and performing
wl_resource_destroy on them. This triggered the destroy handler which
removes the resource from the callback list. Which means removing from
the list we are iterating on. This could result in a double delete or
accessing invalid memory.

This change copies all callbacks to a temporary list and clears the
normal lists. So the destroy handler does no longer modify the lists
currently being iterated on.

Test Plan: Added a test case which crashed with previous code

Reviewers: #plasma

Subscribers: plasma-devel

Tags: #plasma

Differential Revision: https://phabricator.kde.org/D1677
---
 .../autotests/client/test_wayland_surface.cpp | 39 +++++++++++++++++++
 src/wayland/surface_interface.cpp             | 20 ++++++----
 2 files changed, 51 insertions(+), 8 deletions(-)

diff --git a/src/wayland/autotests/client/test_wayland_surface.cpp b/src/wayland/autotests/client/test_wayland_surface.cpp
index 890ec43c6f..1c4f3d40d5 100644
--- a/src/wayland/autotests/client/test_wayland_surface.cpp
+++ b/src/wayland/autotests/client/test_wayland_surface.cpp
@@ -58,6 +58,7 @@ private Q_SLOTS:
     void testDamageTracking();
     void testSurfaceAt();
     void testDestroyAttachedBuffer();
+    void testDestroyWithPendingCallback();
     void testDisconnect();
 
 private:
@@ -874,6 +875,44 @@ void TestWaylandSurface::testDestroyAttachedBuffer()
     QVERIFY(!serverSurface->buffer());
 }
 
+
+void TestWaylandSurface::testDestroyWithPendingCallback()
+{
+    // this test tries to verify that destroying a surface with a pending callback works correctly
+    // first create surface
+    using namespace KWayland::Client;
+    using namespace KWayland::Server;
+    QScopedPointer<Surface> s(m_compositor->createSurface());
+    QVERIFY(!s.isNull());
+    QVERIFY(s->isValid());
+    QSignalSpy surfaceCreatedSpy(m_compositorInterface, &CompositorInterface::surfaceCreated);
+    QVERIFY(surfaceCreatedSpy.isValid());
+    QVERIFY(surfaceCreatedSpy.wait());
+    auto serverSurface = surfaceCreatedSpy.first().first().value<SurfaceInterface*>();
+    QVERIFY(serverSurface);
+
+    // now render to it
+    QImage img(QSize(10, 10), QImage::Format_ARGB32);
+    img.fill(Qt::black);
+    auto b = m_shm->createBuffer(img);
+    s->attachBuffer(b);
+    s->damage(QRect(0, 0, 10, 10));
+    // add some frame callbacks
+    for (int i = 0; i < 1000; i++) {
+        wl_surface_frame(*s);
+    }
+    s->commit(KWayland::Client::Surface::CommitFlag::FrameCallback);
+    QSignalSpy damagedSpy(serverSurface, &SurfaceInterface::damaged);
+    QVERIFY(damagedSpy.isValid());
+    QVERIFY(damagedSpy.wait());
+
+    // now try to destroy the Surface again
+    QSignalSpy destroyedSpy(serverSurface, &QObject::destroyed);
+    QVERIFY(destroyedSpy.isValid());
+    s.reset();
+    QVERIFY(destroyedSpy.wait());
+}
+
 void TestWaylandSurface::testDisconnect()
 {
     // this test verifies that the server side correctly tears down the resources when the client disconnects
diff --git a/src/wayland/surface_interface.cpp b/src/wayland/surface_interface.cpp
index 1784b6f8f7..cc3d030ccb 100644
--- a/src/wayland/surface_interface.cpp
+++ b/src/wayland/surface_interface.cpp
@@ -219,14 +219,18 @@ void SurfaceInterface::frameRendered(quint32 msec)
 
 void SurfaceInterface::Private::destroy()
 {
-    for (wl_resource *c : current.callbacks) {
-        wl_resource_destroy(c);
-    }
-    for (wl_resource *c : pending.callbacks) {
-        wl_resource_destroy(c);
-    }
-    for (wl_resource *c : subSurfacePending.callbacks) {
-        wl_resource_destroy(c);
+    // copy all existing callbacks to new list and clear existing lists
+    // the wl_resource_destroy on the callback resource goes into destroyFrameCallback
+    // which would modify the list we are iterating on
+    QList<wl_resource *> callbacksToDestroy;
+    callbacksToDestroy << current.callbacks;
+    current.callbacks.clear();
+    callbacksToDestroy << pending.callbacks;
+    pending.callbacks.clear();
+    callbacksToDestroy << subSurfacePending.callbacks;
+    subSurfacePending.callbacks.clear();
+    for (auto it = callbacksToDestroy.constBegin(), end = callbacksToDestroy.constEnd(); it != end; it++) {
+        wl_resource_destroy(*it);
     }
     if (current.buffer) {
         current.buffer->unref();