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 <marc.mutz@qt.io>

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);
 

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;
 }

src/corelib/thread/qthread_p.h

@@ -97,6 +97,79 @@ private:
     using QList<QPostEvent>::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<QObject *>;
+
+    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<QBindingStatus *>(data);
+        else
+            return nullptr;
+    }
+
+    List *list() const noexcept
+    {
+        return decodeList(data);
+    }
+
+private:
+    static List *decodeList(quintptr ptr) noexcept
+    {
+        if (isList(ptr))
+            return reinterpret_cast<List *>(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))
+        QMutexLocker lock(&mutex);
-            return (QBindingStatus *) statusOrPendingObjects;
+        return m_statusOrPendingObjects.bindingStatus();
-        return nullptr;
     }
 
-    void addObjectWithPendingBindingStatusChange(QObject *obj)
+    /* Returns nullptr if the object has been added, or the binding status
-    {
+       if that one has been set in the meantime
-        Q_ASSERT(!bindingStatus());
+    */
-        auto pendingObjects = pendingObjectsWithBindingStatusChange();
+    QBindingStatus *addObjectWithPendingBindingStatusChange(QObject *obj);
-        if (!pendingObjects) {
-            pendingObjects = new std::vector<QObject *>();
-            m_statusOrPendingObjects = quintptr(pendingObjects) | 1;
-        }
-        pendingObjects->push_back(obj);
-    }
 
-    std::vector<QObject *> *pendingObjectsWithBindingStatusChange()
+    // manipulating m_statusOrPendingObjects requires mutex to be locked
-    {
+    QtPrivate::BindingStatusOrList m_statusOrPendingObjects = {};
-        auto statusOrPendingObjects = m_statusOrPendingObjects.loadAcquire();
-        if (statusOrPendingObjects & 1)
-            return reinterpret_cast<std::vector<QObject *> *>(statusOrPendingObjects - 1);
-        return nullptr;
-    }
-
-    QAtomicInteger<quintptr> m_statusOrPendingObjects = 0;
 #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 *) {}
+    QBindingStatus *addObjectWithPendingBindingStatusChange(QObject *) { return nullptr; }
-    std::vector<QObject *> * pendingObjectsWithBindingStatusChange() { return nullptr; }
 
     static void setCurrentThread(QThread *) { }
     static QAbstractEventDispatcher *createEventDispatcher(QThreadData *data);