diff options
author | Marc Mutz <marc.mutz@kdab.com> | 2019-07-04 14:08:44 +0200 |
---|---|---|
committer | Marc Mutz <marc.mutz@kdab.com> | 2019-07-05 18:50:12 +0000 |
commit | 90a29a73f84ffd6386beb37690c328b039423fab (patch) | |
tree | eb7af8f930cfe073dad8ad7f82079d2f75ce53a1 /src/network | |
parent | 85dc392135feb72eed449e6eaf47cdd023879394 (diff) |
QHostInfo: fix a race condition on wasDeleted
The plain bool variable wasDeleted is set to true in the
QHostLookupManager dtor before the call to clear(), which calls
waitForDone() on the thread pool performing the lookups. All tasks on
the thread pool start by checking this variable so as to return early
when destruction is in progress. But the check was outside the
mutex-protected area, so as a non-atomic load, without a
happens-before relation to the write, this is a Data Race, thus UB.
Fix by moving the check past the mutex locking into the critical
section. This way, tasks that were waiting for the mutex after seeing
no wasDeleted before get the message reliably.
This does not introduce a dead-lock, since the call to waitForDone()
is outside any mutex protection leaving a window for the tasks to
obtain the mutex and react on wasDeleted.
Change-Id: Ied4b9daa7dc78295b0d36a536839845c4db2fb78
Reviewed-by: MÃ¥rten Nordheim <marten.nordheim@qt.io>
Reviewed-by: Timur Pocheptsov <timur.pocheptsov@qt.io>
Reviewed-by: David Faure <david.faure@kdab.com>
Diffstat (limited to 'src/network')
-rw-r--r-- | src/network/kernel/qhostinfo.cpp | 19 |
1 files changed, 12 insertions, 7 deletions
diff --git a/src/network/kernel/qhostinfo.cpp b/src/network/kernel/qhostinfo.cpp index 4d451dfdb7..e9ea6335d2 100644 --- a/src/network/kernel/qhostinfo.cpp +++ b/src/network/kernel/qhostinfo.cpp @@ -743,7 +743,9 @@ QHostInfoLookupManager::QHostInfoLookupManager() : mutex(QMutex::Recursive), was QHostInfoLookupManager::~QHostInfoLookupManager() { + QMutexLocker locker(&mutex); wasDeleted = true; + locker.unlock(); // don't qDeleteAll currentLookups, the QThreadPool has ownership clear(); @@ -771,6 +773,8 @@ void QHostInfoLookupManager::clear() void QHostInfoLookupManager::work() { + QMutexLocker locker(&mutex); + if (wasDeleted) return; @@ -778,8 +782,6 @@ void QHostInfoLookupManager::work() // - launch new lookups via the thread pool // - make sure only one lookup per host/IP is in progress - QMutexLocker locker(&mutex); - if (!finishedLookups.isEmpty()) { // remove ID from aborted if it is in there for (int i = 0; i < finishedLookups.length(); i++) { @@ -831,10 +833,11 @@ void QHostInfoLookupManager::work() // called by QHostInfo void QHostInfoLookupManager::scheduleLookup(QHostInfoRunnable *r) { + QMutexLocker locker(&this->mutex); + if (wasDeleted) return; - QMutexLocker locker(&this->mutex); scheduledLookups.enqueue(r); work(); } @@ -842,11 +845,11 @@ void QHostInfoLookupManager::scheduleLookup(QHostInfoRunnable *r) // called by QHostInfo void QHostInfoLookupManager::abortLookup(int id) { + QMutexLocker locker(&this->mutex); + if (wasDeleted) return; - QMutexLocker locker(&this->mutex); - #if QT_CONFIG(thread) // is postponed? delete and return for (int i = 0; i < postponedLookups.length(); i++) { @@ -872,20 +875,22 @@ void QHostInfoLookupManager::abortLookup(int id) // called from QHostInfoRunnable bool QHostInfoLookupManager::wasAborted(int id) { + QMutexLocker locker(&this->mutex); + if (wasDeleted) return true; - QMutexLocker locker(&this->mutex); return abortedLookups.contains(id); } // called from QHostInfoRunnable void QHostInfoLookupManager::lookupFinished(QHostInfoRunnable *r) { + QMutexLocker locker(&this->mutex); + if (wasDeleted) return; - QMutexLocker locker(&this->mutex); #if QT_CONFIG(thread) currentLookups.removeOne(r); #endif |