From 718d680579508284f7617a4e7db24d140438478d Mon Sep 17 00:00:00 2001 From: Fabian Kosmale Date: Thu, 10 Mar 2022 17:10:39 +0100 Subject: Fix race conditions in moveToThread Amends ba6c1d2785ca6d8a8b162abcd9d978ab0c52ea2d, which made m_statusOrPendingObjects already atomic, but did not handle concurrent deletion/push_back of the pendingObjects vector correctly. We use the existing lock in QThreadPrivate to prevent data races. Pick-to: 6.2 6.3 Fixes: QTBUG-101681 Change-Id: I0b440fee6ec270d762e6700a4fe74f28b19e75e8 Reviewed-by: Marc Mutz --- src/corelib/kernel/qobject.cpp | 2 +- src/corelib/thread/qthread.cpp | 60 +++++++++++++++++++---- src/corelib/thread/qthread_p.h | 106 +++++++++++++++++++++++++++++++---------- 3 files changed, 135 insertions(+), 33 deletions(-) diff --git a/src/corelib/kernel/qobject.cpp b/src/corelib/kernel/qobject.cpp index 63dfb1cba0..05a881b63c 100644 --- a/src/corelib/kernel/qobject.cpp +++ b/src/corelib/kernel/qobject.cpp @@ -1858,7 +1858,7 @@ void QObject::moveToThread(QThread *targetThread) ? threadPrivate->bindingStatus() : nullptr; if (threadPrivate && !bindingStatus) { - threadPrivate->addObjectWithPendingBindingStatusChange(this); + bindingStatus = threadPrivate->addObjectWithPendingBindingStatusChange(this); } d_func()->setThreadData_helper(currentData, targetData, bindingStatus); diff --git a/src/corelib/thread/qthread.cpp b/src/corelib/thread/qthread.cpp index 735167ac4e..d067bd06d3 100644 --- a/src/corelib/thread/qthread.cpp +++ b/src/corelib/thread/qthread.cpp @@ -160,7 +160,10 @@ QThreadPrivate::QThreadPrivate(QThreadData *d) QThreadPrivate::~QThreadPrivate() { - delete pendingObjectsWithBindingStatusChange(); + // access to m_statusOrPendingObjects cannot race with anything + // unless there is already a potential use-after-free bug, as the + // thread is in the process of being destroyed + delete m_statusOrPendingObjects.list(); data->deref(); } @@ -502,6 +505,23 @@ uint QThread::stackSize() const return d->stackSize; } +/*! + \internal + Transitions BindingStatusOrList to the binding status state. If we had a list of + pending objects, all objects get their reinitBindingStorageAfterThreadMove method + called, and afterwards, the list gets discarded. + */ +void QtPrivate::BindingStatusOrList::setStatusAndClearList(QBindingStatus *status) noexcept +{ + + if (auto pendingObjects = list()) { + for (auto obj: *pendingObjects) + QObjectPrivate::get(obj)->reinitBindingStorageAfterThreadMove(); + delete pendingObjects; + } + data = encodeBindingStatus(status); +} + /*! Enters the event loop and waits until exit() is called, returning the value that was passed to exit(). The value returned is 0 if exit() is called via @@ -519,13 +539,9 @@ int QThread::exec() { Q_D(QThread); const auto status = QtPrivate::getBindingStatus(QtPrivate::QBindingStatusAccessToken{}); - if (auto pendingObjects = d->pendingObjectsWithBindingStatusChange()) { - for (auto obj: *pendingObjects) - QObjectPrivate::get(obj)->reinitBindingStorageAfterThreadMove(); - delete pendingObjects; - } - d->m_statusOrPendingObjects.storeRelease(quintptr(status)); + QMutexLocker locker(&d->mutex); + d->m_statusOrPendingObjects.setStatusAndClearList(status); d->data->quitNow = false; if (d->exited) { d->exited = false; @@ -542,6 +558,35 @@ int QThread::exec() return returnCode; } + +/*! + \internal + If BindingStatusOrList is already in the binding status state, this will + return that BindingStatus pointer. + Otherwise, \a object is added to the list, and we return nullptr. + The list is allocated if it does not already exist. + */ +QBindingStatus *QtPrivate::BindingStatusOrList::addObjectUnlessAlreadyStatus(QObject *object) +{ + + if (auto status = bindingStatus()) + return status; + List *objectList = list(); + if (!objectList) { + objectList = new List(); + objectList->reserve(8); + data = encodeList(objectList); + } + objectList->push_back(object); + return nullptr; +} + +QBindingStatus *QThreadPrivate::addObjectWithPendingBindingStatusChange(QObject *obj) +{ + QMutexLocker lock(&mutex); + return m_statusOrPendingObjects.addObjectUnlessAlreadyStatus(obj); +} + /*! \threadsafe Tells the thread's event loop to exit with a return code. @@ -926,7 +971,6 @@ QThreadPrivate::QThreadPrivate(QThreadData *d) : data(d ? d : new QThreadData) QThreadPrivate::~QThreadPrivate() { - delete pendingObjectsWithBindingStatusChange(); data->thread.storeRelease(nullptr); // prevent QThreadData from deleting the QThreadPrivate (again). delete data; } diff --git a/src/corelib/thread/qthread_p.h b/src/corelib/thread/qthread_p.h index 4520be76eb..79476a940f 100644 --- a/src/corelib/thread/qthread_p.h +++ b/src/corelib/thread/qthread_p.h @@ -97,6 +97,79 @@ private: using QList::insert; }; +namespace QtPrivate { + +/* BindingStatusOrList is basically a QBiPointer (as found in declarative) + with some helper methods to manipulate the list. BindingStatusOrList starts + its life in a null state and supports the following transitions + + 0 state (initial) + / \ + / \ + v v + pending object list----------->binding status + Note that binding status is the final state, and we never transition away + from it +*/ +class BindingStatusOrList +{ + Q_DISABLE_COPY_MOVE(BindingStatusOrList) +public: + using List = std::vector; + + constexpr BindingStatusOrList() noexcept : data(0) {} + explicit BindingStatusOrList(QBindingStatus *status) noexcept : + data(encodeBindingStatus(status)) {} + explicit BindingStatusOrList(List *list) noexcept : data(encodeList(list)) {} + + QBindingStatus *addObjectUnlessAlreadyStatus(QObject *object); + void setStatusAndClearList(QBindingStatus *status) noexcept; + + + static bool isBindingStatus(quintptr data) noexcept + { + return !isNull(data) && !isList(data); + } + static bool isList(quintptr data) noexcept { return data & 1; } + static bool isNull(quintptr data) noexcept { return data == 0; } + + QBindingStatus *bindingStatus() const noexcept + { + if (isBindingStatus(data)) + return reinterpret_cast(data); + else + return nullptr; + } + + List *list() const noexcept + { + return decodeList(data); + } + +private: + static List *decodeList(quintptr ptr) noexcept + { + if (isList(ptr)) + return reinterpret_cast(ptr & ~1); + else + return nullptr; + } + + static quintptr encodeBindingStatus(QBindingStatus *status) noexcept + { + return quintptr(status); + } + + static quintptr encodeList(List *list) noexcept + { + return quintptr(list) | 1; + } + + quintptr data; +}; + +} // namespace QtPrivate + #if QT_CONFIG(thread) class Q_CORE_EXPORT QDaemonThread : public QThread @@ -168,32 +241,18 @@ public: QBindingStatus *bindingStatus() { - auto statusOrPendingObjects = m_statusOrPendingObjects.loadAcquire(); - if (!(statusOrPendingObjects & 1)) - return (QBindingStatus *) statusOrPendingObjects; - return nullptr; - } - void addObjectWithPendingBindingStatusChange(QObject *obj) - { - Q_ASSERT(!bindingStatus()); - auto pendingObjects = pendingObjectsWithBindingStatusChange(); - if (!pendingObjects) { - pendingObjects = new std::vector(); - m_statusOrPendingObjects = quintptr(pendingObjects) | 1; - } - pendingObjects->push_back(obj); + QMutexLocker lock(&mutex); + return m_statusOrPendingObjects.bindingStatus(); } - std::vector *pendingObjectsWithBindingStatusChange() - { - auto statusOrPendingObjects = m_statusOrPendingObjects.loadAcquire(); - if (statusOrPendingObjects & 1) - return reinterpret_cast *>(statusOrPendingObjects - 1); - return nullptr; - } + /* Returns nullptr if the object has been added, or the binding status + if that one has been set in the meantime + */ + QBindingStatus *addObjectWithPendingBindingStatusChange(QObject *obj); - QAtomicInteger m_statusOrPendingObjects = 0; + // manipulating m_statusOrPendingObjects requires mutex to be locked + QtPrivate::BindingStatusOrList m_statusOrPendingObjects = {}; #ifndef Q_OS_INTEGRITY private: // Used in QThread(Private)::start to avoid racy access to QObject::objectName, @@ -216,8 +275,7 @@ public: bool running = false; QBindingStatus* bindingStatus() { return m_bindingStatus; } - void addObjectWithPendingBindingStatusChange(QObject *) {} - std::vector * pendingObjectsWithBindingStatusChange() { return nullptr; } + QBindingStatus *addObjectWithPendingBindingStatusChange(QObject *) { return nullptr; } static void setCurrentThread(QThread *) { } static QAbstractEventDispatcher *createEventDispatcher(QThreadData *data); -- cgit v1.2.3