From 3c40b1af35319f1fd1b10c97b954adff3abbe9b0 Mon Sep 17 00:00:00 2001 From: Olivier Goffart Date: Fri, 15 May 2009 13:54:44 +0200 Subject: Fix race condition in ~QObject while using QOrderedMutexLocker::relock we might unlock our mutex protecting the 'senders' list for a short moment. Another thread may then modify or remove element on the list. Therefore, we need to recheck the consistency of the list once we did that. Also, we cannot call removeSender because that will remove every connections, making impossible for another object destroyed in the same time to clean up the senders list, so call derefSender instead. Reviewed-by: Brad --- src/corelib/kernel/qobject.cpp | 30 ++++------ src/corelib/kernel/qobject_p.h | 1 - tests/auto/qobjectrace/tst_qobjectrace.cpp | 89 ++++++++++++++++++++++++++++++ 3 files changed, 100 insertions(+), 20 deletions(-) diff --git a/src/corelib/kernel/qobject.cpp b/src/corelib/kernel/qobject.cpp index 05015c0..f1a1eb5 100644 --- a/src/corelib/kernel/qobject.cpp +++ b/src/corelib/kernel/qobject.cpp @@ -345,18 +345,6 @@ void QObjectPrivate::derefSender(QObject *sender, int signal) // Q_ASSERT_X(false, "QObjectPrivate::derefSender", "sender not found"); } -void QObjectPrivate::removeSender(QObject *sender, int signal) -{ - for (int i = 0; i < senders.count(); ++i) { - Sender &s = senders[i]; - if (s.sender == sender && s.signal == signal) { - senders.removeAt(i); - return; - } - } - // Q_ASSERT_X(false, "QObjectPrivate::removeSender", "sender not found"); -} - QObjectPrivate::Sender *QObjectPrivate::setCurrentSender(QObject *receiver, Sender *sender) { @@ -790,7 +778,7 @@ QObject::~QObject() bool needToUnlock = QOrderedMutexLocker::relock(locker.mutex(), m); c = &connectionList[i]; if (c->receiver) - c->receiver->d_func()->removeSender(this, signal); + c->receiver->d_func()->derefSender(this, signal); if (needToUnlock) m->unlock(); @@ -811,18 +799,22 @@ QObject::~QObject() } // disconnect all senders - for (int i = 0; i < d->senders.count(); ++i) { + for (int i = 0; i < d->senders.count(); ) { QObjectPrivate::Sender *s = &d->senders[i]; - if (!s->sender) - continue; QMutex *m = &s->sender->d_func()->threadData->mutex; bool needToUnlock = QOrderedMutexLocker::relock(locker.mutex(), m); - s = &d->senders[i]; - if (s->sender) - s->sender->d_func()->removeReceiver(s->signal, this); + if (m < locker.mutex()) { + if (i >= d->senders.count() || s != &d->senders[i]) { + if (needToUnlock) + m->unlock(); + continue; + } + } + s->sender->d_func()->removeReceiver(s->signal, this); if (needToUnlock) m->unlock(); + ++i; } d->senders.clear(); diff --git a/src/corelib/kernel/qobject_p.h b/src/corelib/kernel/qobject_p.h index b324334..0eed938 100644 --- a/src/corelib/kernel/qobject_p.h +++ b/src/corelib/kernel/qobject_p.h @@ -163,7 +163,6 @@ public: QList senders; void refSender(QObject *sender, int signal); void derefSender(QObject *sender, int signal); - void removeSender(QObject *sender, int signal); static Sender *setCurrentSender(QObject *receiver, Sender *sender); diff --git a/tests/auto/qobjectrace/tst_qobjectrace.cpp b/tests/auto/qobjectrace/tst_qobjectrace.cpp index 0c88f29..fcfd528 100644 --- a/tests/auto/qobjectrace/tst_qobjectrace.cpp +++ b/tests/auto/qobjectrace/tst_qobjectrace.cpp @@ -50,6 +50,7 @@ class tst_QObjectRace: public QObject Q_OBJECT private slots: void moveToThreadRace(); + void destroyRace(); }; class RaceObject : public QObject @@ -147,5 +148,93 @@ void tst_QObjectRace::moveToThreadRace() delete object; } + +class MyObject : public QObject +{ Q_OBJECT + public slots: + void slot1() { emit signal1(); } + void slot2() { emit signal2(); } + void slot3() { emit signal3(); } + void slot4() { emit signal4(); } + void slot5() { emit signal5(); } + void slot6() { emit signal6(); } + void slot7() { emit signal7(); } + signals: + void signal1(); + void signal2(); + void signal3(); + void signal4(); + void signal5(); + void signal6(); + void signal7(); +}; + + + +class DestroyThread : public QThread +{ + Q_OBJECT + QObject **objects; + int number; + +public: + void setObjects(QObject **o, int n) + { + objects = o; + number = n; + for(int i = 0; i < number; i++) + objects[i]->moveToThread(this); + } + + void run() { + for(int i = 0; i < number; i++) + delete objects[i]; + } +}; + +void tst_QObjectRace::destroyRace() +{ + enum { ThreadCount = 10, ObjectCountPerThread = 733, + ObjectCount = ThreadCount * ObjectCountPerThread }; + + const char *_slots[] = { SLOT(slot1()) , SLOT(slot2()) , SLOT(slot3()), + SLOT(slot4()) , SLOT(slot5()) , SLOT(slot6()), + SLOT(slot7()) }; + + const char *_signals[] = { SIGNAL(signal1()), SIGNAL(signal2()), SIGNAL(signal3()), + SIGNAL(signal4()), SIGNAL(signal5()), SIGNAL(signal6()), + SIGNAL(signal7()) }; + + QObject *objects[ObjectCount]; + for (int i = 0; i < ObjectCount; ++i) + objects[i] = new MyObject; + + + for (int i = 0; i < ObjectCount * 11; ++i) { + connect(objects[(i*13) % ObjectCount], _signals[(2*i)%7], + objects[((i+2)*17) % ObjectCount], _slots[(3*i+2)%7] ); + connect(objects[((i+6)*23) % ObjectCount], _signals[(5*i+4)%7], + objects[((i+8)*41) % ObjectCount], _slots[(i+6)%7] ); + } + + DestroyThread *threads[ThreadCount]; + for (int i = 0; i < ThreadCount; ++i) { + threads[i] = new DestroyThread; + threads[i]->setObjects(objects + i*ObjectCountPerThread, ObjectCountPerThread); + } + + for (int i = 0; i < ThreadCount; ++i) + threads[i]->start(); + + QVERIFY(threads[0]->wait(TwoMinutes)); + // the other threads should finish pretty quickly now + for (int i = 1; i < ThreadCount; ++i) + QVERIFY(threads[i]->wait(3000)); + + for (int i = 0; i < ThreadCount; ++i) + delete threads[i]; +} + + QTEST_MAIN(tst_QObjectRace) #include "tst_qobjectrace.moc" -- cgit v0.12