diff options
author | Olivier Goffart <ogoffart@woboq.com> | 2013-11-15 09:46:02 +0100 |
---|---|---|
committer | The Qt Project <gerrit-noreply@qt-project.org> | 2013-11-26 08:24:25 +0100 |
commit | f805020410c8ccb0cd223988565bcabde1c5806b (patch) | |
tree | 4c470de0c3705a5b2ef8da41e80e8f9b26a010a6 /src | |
parent | c2f08598e1cc3089505dd8037d333071da0f231f (diff) |
Fix a race that occurred as we unlock the mutex to destroy the functor in ~QObject
When we unlock the mutex, we need to take in account that the Connection
pointed by 'node' may be destroyed in another thread while it is unlocked
Doing 'node->prev = &node' will make sure that 'node' is actually
updated when it is destroyed.
Setting isSlotObject under the mutex is safer and ensure that no other
thread will attempt to deref the object.
The regression was introduced in 5885b8f775998c30d53f40b7f368c5f6364e6df4
tst_qobjectrace was updated to catch races arising when we are
connecting with function pointers.
Change-Id: Ia0d11ae8df563dad97eb86993a786b579b28cd03
Reviewed-by: Allan Sandfeld Jensen <allan.jensen@digia.com>
Reviewed-by: Lars Knoll <lars.knoll@digia.com>
Diffstat (limited to 'src')
-rw-r--r-- | src/corelib/kernel/qobject.cpp | 20 |
1 files changed, 16 insertions, 4 deletions
diff --git a/src/corelib/kernel/qobject.cpp b/src/corelib/kernel/qobject.cpp index 6ed3b30917..777f95ef6d 100644 --- a/src/corelib/kernel/qobject.cpp +++ b/src/corelib/kernel/qobject.cpp @@ -1,6 +1,7 @@ /**************************************************************************** ** ** Copyright (C) 2013 Digia Plc and/or its subsidiary(-ies). +** Copyright (C) 2013 Olivier Goffart <ogoffart@woboq.com> ** Contact: http://www.qt-project.org/legal ** ** This file is part of the QtCore module of the Qt Toolkit. @@ -852,9 +853,9 @@ QObject::~QObject() // The destroy operation must happen outside the lock if (c->isSlotObject) { + c->isSlotObject = false; locker.unlock(); c->slotObj->destroyIfLastRef(); - c->isSlotObject = false; locker.relock(); } c->deref(); @@ -869,7 +870,15 @@ QObject::~QObject() d->connectionLists = 0; } - // disconnect all senders + /* Disconnect all senders: + * This loop basically just does + * for (node = d->senders; node; node = node->next) { ... } + * + * We need to temporarily unlock the receiver mutex to destroy the functors or to lock the + * sender's mutex. And when the mutex is released, node->next might be destroyed by another + * thread. That's why we set node->prev to &node, that way, if node is destroyed, node will + * be updated. + */ QObjectPrivate::Connection *node = d->senders; while (node) { QObject *sender = node->sender; @@ -882,6 +891,8 @@ QObject::~QObject() bool needToUnlock = QOrderedMutexLocker::relock(signalSlotMutex, m); //the node has maybe been removed while the mutex was unlocked in relock? if (!node || node->sender != sender) { + // We hold the wrong mutex + Q_ASSERT(needToUnlock); m->unlock(); continue; } @@ -901,11 +912,12 @@ QObject::~QObject() m->unlock(); if (slotObj) { + if (node) + node->prev = &node; locker.unlock(); slotObj->destroyIfLastRef(); locker.relock(); } - } } @@ -3186,9 +3198,9 @@ bool QMetaObjectPrivate::disconnectHelper(QObjectPrivate::Connection *c, c->receiver = 0; if (c->isSlotObject) { + c->isSlotObject = false; senderMutex->unlock(); c->slotObj->destroyIfLastRef(); - c->isSlotObject = false; senderMutex->lock(); } |