summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorLars Knoll <lars.knoll@qt.io>2020-11-20 13:51:16 +0100
committerQt Cherry-pick Bot <cherrypick_bot@qt-project.org>2021-06-17 09:27:47 +0000
commite41c100460126d8cb044f11db73b52d6b80f9309 (patch)
treebe80d2ba9c18a3560539148d83a6ccc87c7b1667
parentb6323f0c7ba24a4ad5daa3535fcc9d6b285cfe28 (diff)
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. Task-number: QTBUG-88248 Change-Id: I9d841eb363b7e4f0de1657aeb8f5340d0fd55190 Reviewed-by: Andrei Golubev <andrei.golubev@qt.io> Reviewed-by: Thiago Macieira <thiago.macieira@intel.com> (cherry picked from commit 71b4d4f150bc3c904a5aceec37513ddc3cd1c150) Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>
-rw-r--r--src/corelib/kernel/qobject.cpp47
-rw-r--r--src/corelib/kernel/qobject_p.h16
-rw-r--r--tests/auto/other/qobjectrace/tst_qobjectrace.cpp49
3 files changed, 91 insertions, 21 deletions
diff --git a/src/corelib/kernel/qobject.cpp b/src/corelib/kernel/qobject.cpp
index 33afcaaa7e..326fba5be2 100644
--- a/src/corelib/kernel/qobject.cpp
+++ b/src/corelib/kernel/qobject.cpp
@@ -390,8 +390,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;
@@ -420,8 +426,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
@@ -1027,7 +1032,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);
}
@@ -1068,11 +1073,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
@@ -4978,11 +4983,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;
}
@@ -5176,12 +5181,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;
}
diff --git a/src/corelib/kernel/qobject_p.h b/src/corelib/kernel/qobject_p.h
index 8dd29e05ce..4d5543d34a 100644
--- a/src/corelib/kernel/qobject_p.h
+++ b/src/corelib/kernel/qobject_p.h
@@ -257,7 +257,10 @@ public:
~ConnectionData()
{
- deleteOrphaned(orphaned.loadRelaxed());
+ Q_ASSERT(ref.loadRelaxed() == 0);
+ auto *c = orphaned.fetchAndStoreRelaxed(nullptr);
+ if (c)
+ deleteOrphaned(c);
SignalVector *v = signalVector.loadRelaxed();
if (v)
free(v);
@@ -304,8 +307,15 @@ public:
signalVector.storeRelaxed(newVector);
if (vector) {
- vector->nextInOrphanList = orphaned.loadRelaxed();
- orphaned.storeRelaxed(ConnectionOrSignalVector::fromSignalVector(vector));
+ 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();
+ vector->nextInOrphanList = o;
+ } while (!orphaned.testAndSetRelease(o, ConnectionOrSignalVector::fromSignalVector(vector)));
+
}
}
int signalVectorCount() const
diff --git a/tests/auto/other/qobjectrace/tst_qobjectrace.cpp b/tests/auto/other/qobjectrace/tst_qobjectrace.cpp
index ac33fa3ec3..093b4d2476 100644
--- a/tests/auto/other/qobjectrace/tst_qobjectrace.cpp
+++ b/tests/auto/other/qobjectrace/tst_qobjectrace.cpp
@@ -49,6 +49,7 @@ private slots:
void destroyRace();
void blockingQueuedDestroyRace();
void disconnectRace();
+ void disconnectRace2();
};
class RaceObject : public QObject
@@ -562,5 +563,53 @@ void tst_QObjectRace::disconnectRace()
QCOMPARE(countedStructObjectsCount.loadRelaxed(), 0u);
}
+void tst_QObjectRace::disconnectRace2()
+{
+ enum { IterationCount = 100, ConnectionCount = 100, YieldCount = 100 };
+
+ QAtomicPointer<MyObject> ptr;
+ QSemaphore createSemaphore(0);
+ QSemaphore proceedSemaphore(0);
+
+ std::unique_ptr<QThread> t1(QThread::create([&]() {
+ for (int i = 0; i < IterationCount; ++i) {
+ MyObject sender;
+ ptr.storeRelease(&sender);
+ createSemaphore.release();
+ proceedSemaphore.acquire();
+ ptr.storeRelaxed(nullptr);
+ for (int i = 0; i < YieldCount; ++i)
+ QThread::yieldCurrentThread();
+ }
+ }));
+ t1->start();
+
+
+ std::unique_ptr<QThread> t2(QThread::create([&]() {
+ auto connections = std::make_unique<QMetaObject::Connection[]>(ConnectionCount);
+ for (int i = 0; i < IterationCount; ++i) {
+ MyObject receiver;
+ MyObject *sender = nullptr;
+
+ createSemaphore.acquire();
+
+ while (!(sender = ptr.loadAcquire()))
+ ;
+
+ for (int i = 0; i < ConnectionCount; ++i)
+ connections[i] = QObject::connect(sender, &MyObject::signal1, &receiver, &MyObject::slot1);
+
+ proceedSemaphore.release();
+
+ for (int i = 0; i < ConnectionCount; ++i)
+ QObject::disconnect(connections[i]);
+ }
+ }));
+ t2->start();
+
+ t1->wait();
+ t2->wait();
+}
+
QTEST_MAIN(tst_QObjectRace)
#include "tst_qobjectrace.moc"