QObjectPrivate: fix data race on ConnectionData contents

The atomic pointer "connections" is always populated under
mutex in QObjectPrivate::ensureConnectionData() but isn't necessarily
read under mutex protection (e.g. in maybeSignalConnected()).
This caused a data race, fixed by using storeRelease and loadAcquired.

Task-number: QTBUG-100336
Pick-to: 6.7 6.6 6.5
Change-Id: Ifd605e65122248eb08f49e036fdda6e6564226bc
Reviewed-by: Marc Mutz <marc.mutz@qt.io>
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>

2 files changed, 29 insertions, 22 deletions

diff --git a/src/corelib/kernel/qobject.cpp b/src/corelib/kernel/qobject.cpp
index 56e1d3d7f1..708b10a75e 100644
--- a/src/corelib/kernel/qobject.cpp
+++ b/src/corelib/kernel/qobject.cpp
@@ -231,7 +231,7 @@ QObjectList QObjectPrivate::receiverList(const char *signal) const
 {
     QObjectList returnValue;
     int signal_index = signalIndex(signal);
-    ConnectionData *cd = connections.loadRelaxed();
+    ConnectionData *cd = connections.loadAcquire();
     if (signal_index < 0 || !cd)
         return returnValue;
     if (signal_index < cd->signalVectorCount()) {
@@ -247,13 +247,17 @@ QObjectList QObjectPrivate::receiverList(const char *signal) const
     return returnValue;
 }
 
+/*!
+  \internal
+  The signalSlotLock() of the sender must be locked while calling this function
+*/
 inline void QObjectPrivate::ensureConnectionData()
 {
     if (connections.loadRelaxed())
         return;
     ConnectionData *cd = new ConnectionData;
     cd->ref.ref();
-    connections.storeRelaxed(cd);
+    connections.storeRelease(cd);
 }
 
 /*!
@@ -418,7 +422,7 @@ bool QObjectPrivate::isSignalConnected(uint signalIndex, bool checkDeclarative)
     if (checkDeclarative && isDeclarativeSignalConnected(signalIndex))
         return true;
 
-    ConnectionData *cd = connections.loadRelaxed();
+    ConnectionData *cd = connections.loadAcquire();
     if (!cd)
         return false;
     SignalVector *signalVector = cd->signalVector.loadRelaxed();
@@ -441,7 +445,7 @@ bool QObjectPrivate::isSignalConnected(uint signalIndex, bool checkDeclarative)
 
 bool QObjectPrivate::maybeSignalConnected(uint signalIndex) const
 {
-    ConnectionData *cd = connections.loadRelaxed();
+    ConnectionData *cd = connections.loadAcquire();
     if (!cd)
         return false;
     SignalVector *signalVector = cd->signalVector.loadRelaxed();
@@ -1039,7 +1043,7 @@ QObject::~QObject()
     if (!d->isDeletingChildren && d->declarativeData && QAbstractDeclarativeData::destroyed)
         QAbstractDeclarativeData::destroyed(d->declarativeData, this);
 
-    QObjectPrivate::ConnectionData *cd = d->connections.loadRelaxed();
+    QObjectPrivate::ConnectionData *cd = d->connections.loadAcquire();
     if (cd) {
         if (cd->currentSender) {
             cd->currentSender->receiverDeleted();
@@ -1404,11 +1408,13 @@ bool QObject::event(QEvent *e)
         {
             QAbstractMetaCallEvent *mce = static_cast<QAbstractMetaCallEvent*>(e);
 
-            if (!d_func()->connections.loadRelaxed()) {
+            QObjectPrivate::ConnectionData *connections = d_func()->connections.loadAcquire();
+            if (!connections) {
                 QMutexLocker locker(signalSlotLock(this));
                 d_func()->ensureConnectionData();
+                connections = d_func()->connections.loadRelaxed();
             }
-            QObjectPrivate::Sender sender(this, const_cast<QObject*>(mce->sender()), mce->signalId());
+            QObjectPrivate::Sender sender(this, const_cast<QObject*>(mce->sender()), mce->signalId(), connections);
 
             mce->placeMetaCall(this);
             break;
@@ -1750,7 +1756,7 @@ void QObjectPrivate::setThreadData_helper(QThreadData *currentData, QThreadData
     }
 
     // the current emitting thread shouldn't restore currentSender after calling moveToThread()
-    ConnectionData *cd = connections.loadRelaxed();
+    ConnectionData *cd = connections.loadAcquire();
     if (cd) {
         if (cd->currentSender) {
             cd->currentSender->receiverDeleted();
@@ -2757,8 +2763,8 @@ int QObject::receivers(const char *signal) const
                                                              signal_index);
         }
 
-        QObjectPrivate::ConnectionData *cd = d->connections.loadRelaxed();
         QMutexLocker locker(signalSlotLock(this));
+        QObjectPrivate::ConnectionData *cd = d->connections.loadRelaxed();
         if (cd && signal_index < cd->signalVectorCount()) {
             const QObjectPrivate::Connection *c = cd->signalVector.loadRelaxed()->at(signal_index).first.loadRelaxed();
             while (c) {
@@ -4016,8 +4022,8 @@ void doActivate(QObject *sender, int signal_index, void **argv)
 
     bool senderDeleted = false;
     {
-    Q_ASSERT(sp->connections.loadAcquire());
-    QObjectPrivate::ConnectionDataPointer connections(sp->connections.loadRelaxed());
+    Q_ASSERT(sp->connections.loadRelaxed());
+    QObjectPrivate::ConnectionDataPointer connections(sp->connections.loadAcquire());
     QObjectPrivate::SignalVector *signalVector = connections->signalVector.loadRelaxed();
 
     const QObjectPrivate::ConnectionList *list;
@@ -4093,7 +4099,9 @@ void doActivate(QObject *sender, int signal_index, void **argv)
             if (c->isSingleShot && !QObjectPrivate::removeConnection(c))
                 continue;
 
-            QObjectPrivate::Sender senderData(receiverInSameThread ? receiver : nullptr, sender, signal_index);
+            QObjectPrivate::Sender senderData(
+                    receiverInSameThread ? receiver : nullptr, sender, signal_index,
+                    receiverInSameThread ? QObjectPrivate::get(receiver)->connections.loadAcquire() : nullptr);
 
             if (c->isSlotObject) {
                 SlotObjectGuard obj{c->slotObj};
@@ -4142,7 +4150,7 @@ void doActivate(QObject *sender, int signal_index, void **argv)
             senderDeleted = true;
     }
     if (!senderDeleted) {
-        sp->connections.loadRelaxed()->cleanOrphanedConnections(sender);
+        sp->connections.loadAcquire()->cleanOrphanedConnections(sender);
 
         if (callbacks_enabled && signal_spy_set->signal_end_callback != nullptr)
             signal_spy_set->signal_end_callback(sender, signal_index);
@@ -5251,9 +5259,9 @@ QMetaObject::Connection QObjectPrivate::connectImpl(const QObject *sender, int s
     QOrderedMutexLocker locker(signalSlotLock(sender),
                                signalSlotLock(receiver));
 
-    if (type & Qt::UniqueConnection && slot && QObjectPrivate::get(s)->connections.loadRelaxed()) {
+    if (type & Qt::UniqueConnection && slot) {
         QObjectPrivate::ConnectionData *connections = QObjectPrivate::get(s)->connections.loadRelaxed();
-        if (connections->signalVectorCount() > signal_index) {
+        if (connections && connections->signalVectorCount() > signal_index) {
             const QObjectPrivate::Connection *c2 = connections->signalVector.loadRelaxed()->at(signal_index).first.loadRelaxed();
 
             while (c2) {
@@ -5530,7 +5538,7 @@ inline bool QObjectPrivate::removeConnection(QObjectPrivate::Connection *c)
 QtPrivate::QPropertyAdaptorSlotObject *
 QObjectPrivate::getPropertyAdaptorSlotObject(const QMetaProperty &property)
 {
-    if (auto conns = connections.loadRelaxed()) {
+    if (auto conns = connections.loadAcquire()) {
         Q_Q(QObject);
         const QMetaObject *metaObject = q->metaObject();
         int signal_index = methodIndexToSignalIndex(&metaObject, property.notifySignalIndex());
diff --git a/src/corelib/kernel/qobject_p_p.h b/src/corelib/kernel/qobject_p_p.h
index a9bd290d37..3519e4dd13 100644
--- a/src/corelib/kernel/qobject_p_p.h
+++ b/src/corelib/kernel/qobject_p_p.h
@@ -224,19 +224,18 @@ struct QObjectPrivate::ConnectionData
 
 struct QObjectPrivate::Sender
 {
-    Sender(QObject *receiver, QObject *sender, int signal)
+    Sender(QObject *receiver, QObject *sender, int signal, ConnectionData *receiverConnections)
         : receiver(receiver), sender(sender), signal(signal)
     {
-        if (receiver) {
-            ConnectionData *cd = receiver->d_func()->connections.loadRelaxed();
-            previous = cd->currentSender;
-            cd->currentSender = this;
+        if (receiverConnections) {
+            previous = receiverConnections->currentSender;
+            receiverConnections->currentSender = this;
         }
     }
     ~Sender()
     {
         if (receiver)
-            receiver->d_func()->connections.loadRelaxed()->currentSender = previous;
+            receiver->d_func()->connections.loadAcquire()->currentSender = previous;
     }
     void receiverDeleted()
     {