From 90a29a73f84ffd6386beb37690c328b039423fab Mon Sep 17 00:00:00 2001 From: Marc Mutz Date: Thu, 4 Jul 2019 14:08:44 +0200 Subject: QHostInfo: fix a race condition on wasDeleted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Timur Pocheptsov Reviewed-by: David Faure --- src/network/kernel/qhostinfo.cpp | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) (limited to 'src/network/kernel') 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 -- cgit v1.2.3 From 0a3107dd302c521c240b273229ba40a358cb0873 Mon Sep 17 00:00:00 2001 From: Marc Mutz Date: Thu, 4 Jul 2019 15:24:13 +0200 Subject: QHostInfo: fix a race condition on QHostInfoCache::enabled MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In auto-test-enabled builds, QHostInfoCache can be enabled and disabled using qt_qhostinfo_enable_cache() at any time. We cannot rule out that users use this function, or, indeed, that the auto-test never gets a threading stress-test. Under the assumption, then, that QHostInfoCache::enabled can be set by any thread at any time, and is read by any thread using QHostInfo::lookupHost(), we're presented with a data race, thus UB. Fix by making the accesses to QHostInfoCache::enabed atomic. Relaxed operations are suffcient, as the bool is the only data of interest in these situations. In particular, access to the cache itself is protected by the cache's mutex. We use std::atomic because QAtomicInteger doesn't exist on all implementations, but std::atomic must. Commit a0faf9e2366 set the precedent that it works. Change-Id: Ia1766753bb54c5fe8d8447b51a49a96a7a853eef Reviewed-by: Mårten Nordheim --- src/network/kernel/qhostinfo.cpp | 15 +-------------- src/network/kernel/qhostinfo_p.h | 9 ++++++--- 2 files changed, 7 insertions(+), 17 deletions(-) (limited to 'src/network/kernel') diff --git a/src/network/kernel/qhostinfo.cpp b/src/network/kernel/qhostinfo.cpp index e9ea6335d2..597c616fe9 100644 --- a/src/network/kernel/qhostinfo.cpp +++ b/src/network/kernel/qhostinfo.cpp @@ -952,23 +952,10 @@ void qt_qhostinfo_cache_inject(const QString &hostname, const QHostInfo &resolut QHostInfoCache::QHostInfoCache() : max_age(60), enabled(true), cache(128) { #ifdef QT_QHOSTINFO_CACHE_DISABLED_BY_DEFAULT - enabled = false; + enabled.store(false, std::memory_order_relaxed); #endif } -bool QHostInfoCache::isEnabled() -{ - return enabled; -} - -// this function is currently only used for the auto tests -// and not usable by public API -void QHostInfoCache::setEnabled(bool e) -{ - enabled = e; -} - - QHostInfo QHostInfoCache::get(const QString &name, bool *valid) { QMutexLocker locker(&this->mutex); diff --git a/src/network/kernel/qhostinfo_p.h b/src/network/kernel/qhostinfo_p.h index 8cce302166..8898d6ff50 100644 --- a/src/network/kernel/qhostinfo_p.h +++ b/src/network/kernel/qhostinfo_p.h @@ -73,6 +73,7 @@ #include #include +#include QT_BEGIN_NAMESPACE @@ -176,10 +177,12 @@ public: void put(const QString &name, const QHostInfo &info); void clear(); - bool isEnabled(); - void setEnabled(bool e); + bool isEnabled() { return enabled.load(std::memory_order_relaxed); } + // this function is currently only used for the auto tests + // and not usable by public API + void setEnabled(bool e) { enabled.store(e, std::memory_order_relaxed); } private: - bool enabled; + std::atomic enabled; struct QHostInfoCacheElement { QHostInfo info; QElapsedTimer age; -- cgit v1.2.3