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>

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