From 71b4d4f150bc3c904a5aceec37513ddc3cd1c150 Mon Sep 17 00:00:00 2001
From: Lars Knoll <lars.knoll@qt.io>
Date: Fri, 20 Nov 2020 13:51:16 +0100
Subject: Fix crash in concurrent disconnect

This does not fix all data races that we have in the system yet.
One major issue is the virtual disconnectNotify(), that can be
called from any thread and thus is inherently problematic, as it
can collide with the object getting destroyed at the same time
in another thread.

Pick-to: 6.2 6.1 5.15
Task-number: QTBUG-88248
Change-Id: I9d841eb363b7e4f0de1657aeb8f5340d0fd55190
Reviewed-by: Andrei Golubev <andrei.golubev@qt.io>
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
---
 src/corelib/kernel/qobject.cpp | 47 ++++++++++++++++++++++++++----------------
 1 file changed, 29 insertions(+), 18 deletions(-)

(limited to 'src/corelib/kernel/qobject.cpp')

diff --git a/src/corelib/kernel/qobject.cpp b/src/corelib/kernel/qobject.cpp
index 4c2a787173..b38f39e2c8 100644
--- a/src/corelib/kernel/qobject.cpp
+++ b/src/corelib/kernel/qobject.cpp
@@ -389,8 +389,14 @@ void QObjectPrivate::ConnectionData::removeConnection(QObjectPrivate::Connection
 
     Q_ASSERT(c != orphaned.loadRelaxed());
     // add c to orphanedConnections
-    c->nextInOrphanList = orphaned.loadRelaxed();
-    orphaned.storeRelaxed(c);
+    Connection *o = nullptr;
+    /* No ABA issue here: When adding a node, we only care about the list head, it doesn't
+     * matter if the tail changes.
+     */
+    do {
+        o = orphaned.loadRelaxed();
+        c->nextInOrphanList = o;
+    } while (!orphaned.testAndSetRelease(o, c));
 
 #ifndef QT_NO_DEBUG
     found = false;
@@ -419,8 +425,7 @@ void QObjectPrivate::ConnectionData::cleanOrphanedConnectionsImpl(QObject *sende
         // Since ref == 1, no activate() is in process since we locked the mutex. That implies,
         // that nothing can reference the orphaned connection objects anymore and they can
         // be safely deleted
-        c = orphaned.loadRelaxed();
-        orphaned.storeRelaxed(nullptr);
+        c = orphaned.fetchAndStoreRelaxed(nullptr);
     }
     if (c) {
         // Deleting c might run arbitrary user code, so we must not hold the lock
@@ -1035,7 +1040,7 @@ QObject::~QObject()
 
                 QBasicMutex *m = signalSlotLock(c->receiver.loadRelaxed());
                 bool needToUnlock = QOrderedMutexLocker::relock(signalSlotMutex, m);
-                if (c->receiver.loadAcquire()) {
+                if (c == connectionList.first.loadAcquire() && c->receiver.loadAcquire()) {
                     cd->removeConnection(c);
                     Q_ASSERT(connectionList.first.loadRelaxed() != c);
                 }
@@ -1076,11 +1081,11 @@ QObject::~QObject()
             if (needToUnlock)
                 m->unlock();
 
-            if (slotObj) {
-                locker.unlock();
+            locker.unlock();
+            if (slotObj)
                 slotObj->destroyIfLastRef();
-                locker.relock();
-            }
+            senderData->cleanOrphanedConnections(sender);
+            locker.relock();
         }
 
         // invalidate all connections on the object and make sure
@@ -5024,11 +5029,11 @@ QMetaObject::Connection QObjectPrivate::connectImpl(const QObject *sender, int s
 bool QObject::disconnect(const QMetaObject::Connection &connection)
 {
     QObjectPrivate::Connection *c = static_cast<QObjectPrivate::Connection *>(connection.d_ptr);
+    if (!c)
+        return false;
     const bool disconnected = QObjectPrivate::disconnect(c);
-    if (disconnected) {
-        const_cast<QMetaObject::Connection &>(connection).d_ptr = nullptr;
-        c->deref(); // has been removed from the QMetaObject::Connection object
-    }
+    const_cast<QMetaObject::Connection &>(connection).d_ptr = nullptr;
+    c->deref(); // has been removed from the QMetaObject::Connection object
     return disconnected;
 }
 
@@ -5222,12 +5227,18 @@ bool QObjectPrivate::disconnect(QObjectPrivate::Connection *c)
         connections = QObjectPrivate::get(c->sender)->connections.loadRelaxed();
         Q_ASSERT(connections);
         connections->removeConnection(c);
-    }
-
-    connections->cleanOrphanedConnections(c->sender);
 
-    c->sender->disconnectNotify(QMetaObjectPrivate::signal(c->sender->metaObject(),
-                                                           c->signal_index));
+        c->sender->disconnectNotify(QMetaObjectPrivate::signal(c->sender->metaObject(), c->signal_index));
+        // We must not hold the receiver mutex, else we risk dead-locking; we also only need the sender mutex
+        // It is however vital to hold the senderMutex before calling cleanOrphanedConnections, as otherwise
+        // another thread might modify/delete the connection
+        if (receiverMutex != senderMutex) {
+            receiverMutex->unlock();
+        }
+        connections->cleanOrphanedConnections(c->sender, ConnectionData::AlreadyLockedAndTemporarilyReleasingLock);
+        senderMutex->unlock(); // now both sender and receiver mutex have been manually unlocked
+        locker.dismiss(); // so we dismiss the QOrderedMutexLocker
+    }
 
     return true;
 }
-- 
cgit v1.2.3