1berry/qtbase
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

QObjectPrivate: fix data race on ConnectionData contents

Browse Source 
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>
This commit is contained in:
David Faure 2024-03-08 10:36:11 +01:00
parent e46f99fdaa
commit 75d82afa0d
2 changed files with 29 additions and 22 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

40
src/corelib/kernel/qobject.cpp
View File

@ -231,7 +231,7 @@ QObjectList QObjectPrivate::receiverList(const char *signal) const
{ {
QObjectList returnValue; QObjectList returnValue;
int signal_index = signalIndex(signal); int signal_index = signalIndex(signal);
ConnectionData *cd = connections.loadRelaxed(); ConnectionData *cd = connections.loadAcquire();
if (signal_index < 0 || !cd) if (signal_index < 0 || !cd)
return returnValue; return returnValue;
if (signal_index < cd->signalVectorCount()) { if (signal_index < cd->signalVectorCount()) {
@ -247,13 +247,17 @@ QObjectList QObjectPrivate::receiverList(const char *signal) const
return returnValue; return returnValue;
} }
/*!
\internal
The signalSlotLock() of the sender must be locked while calling this function
*/
inline void QObjectPrivate::ensureConnectionData() inline void QObjectPrivate::ensureConnectionData()
{ {
if (connections.loadRelaxed()) if (connections.loadRelaxed())
return; return;
ConnectionData *cd = new ConnectionData; ConnectionData *cd = new ConnectionData;
cd->ref.ref(); cd->ref.ref();
connections.storeRelaxed(cd); connections.storeRelease(cd);
} }
/*! /*!
@ -418,7 +422,7 @@ bool QObjectPrivate::isSignalConnected(uint signalIndex, bool checkDeclarative)
if (checkDeclarative && isDeclarativeSignalConnected(signalIndex)) if (checkDeclarative && isDeclarativeSignalConnected(signalIndex))
return true; return true;
ConnectionData *cd = connections.loadRelaxed(); ConnectionData *cd = connections.loadAcquire();
if (!cd) if (!cd)
return false; return false;
SignalVector *signalVector = cd->signalVector.loadRelaxed(); SignalVector *signalVector = cd->signalVector.loadRelaxed();
@ -441,7 +445,7 @@ bool QObjectPrivate::isSignalConnected(uint signalIndex, bool checkDeclarative)
bool QObjectPrivate::maybeSignalConnected(uint signalIndex) const bool QObjectPrivate::maybeSignalConnected(uint signalIndex) const
{ {
ConnectionData *cd = connections.loadRelaxed(); ConnectionData *cd = connections.loadAcquire();
if (!cd) if (!cd)
return false; return false;
SignalVector *signalVector = cd->signalVector.loadRelaxed(); SignalVector *signalVector = cd->signalVector.loadRelaxed();
@ -1039,7 +1043,7 @@ QObject::~QObject()
if (!d->isDeletingChildren && d->declarativeData && QAbstractDeclarativeData::destroyed) if (!d->isDeletingChildren && d->declarativeData && QAbstractDeclarativeData::destroyed)
QAbstractDeclarativeData::destroyed(d->declarativeData, this); QAbstractDeclarativeData::destroyed(d->declarativeData, this);
QObjectPrivate::ConnectionData *cd = d->connections.loadRelaxed(); QObjectPrivate::ConnectionData *cd = d->connections.loadAcquire();
if (cd) { if (cd) {
if (cd->currentSender) { if (cd->currentSender) {
cd->currentSender->receiverDeleted(); cd->currentSender->receiverDeleted();
@ -1404,11 +1408,13 @@ bool QObject::event(QEvent *e)
{ {
QAbstractMetaCallEvent *mce = static_cast<QAbstractMetaCallEvent*>(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)); QMutexLocker locker(signalSlotLock(this));
d_func()->ensureConnectionData(); 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); mce->placeMetaCall(this);
break; break;
@ -1750,7 +1756,7 @@ void QObjectPrivate::setThreadData_helper(QThreadData *currentData, QThreadData
} }
// the current emitting thread shouldn't restore currentSender after calling moveToThread() // the current emitting thread shouldn't restore currentSender after calling moveToThread()
ConnectionData *cd = connections.loadRelaxed(); ConnectionData *cd = connections.loadAcquire();
if (cd) { if (cd) {
if (cd->currentSender) { if (cd->currentSender) {
cd->currentSender->receiverDeleted(); cd->currentSender->receiverDeleted();
@ -2757,8 +2763,8 @@ int QObject::receivers(const char *signal) const
signal_index); signal_index);
} }
QObjectPrivate::ConnectionData *cd = d->connections.loadRelaxed();
QMutexLocker locker(signalSlotLock(this)); QMutexLocker locker(signalSlotLock(this));
QObjectPrivate::ConnectionData *cd = d->connections.loadRelaxed();
if (cd && signal_index < cd->signalVectorCount()) { if (cd && signal_index < cd->signalVectorCount()) {
const QObjectPrivate::Connection *c = cd->signalVector.loadRelaxed()->at(signal_index).first.loadRelaxed(); const QObjectPrivate::Connection *c = cd->signalVector.loadRelaxed()->at(signal_index).first.loadRelaxed();
while (c) { while (c) {
@ -4016,8 +4022,8 @@ void doActivate(QObject *sender, int signal_index, void **argv)
bool senderDeleted = false; bool senderDeleted = false;
{ {
Q_ASSERT(sp->connections.loadAcquire()); Q_ASSERT(sp->connections.loadRelaxed());
QObjectPrivate::ConnectionDataPointer connections(sp->connections.loadRelaxed()); QObjectPrivate::ConnectionDataPointer connections(sp->connections.loadAcquire());
QObjectPrivate::SignalVector *signalVector = connections->signalVector.loadRelaxed(); QObjectPrivate::SignalVector *signalVector = connections->signalVector.loadRelaxed();
const QObjectPrivate::ConnectionList *list; const QObjectPrivate::ConnectionList *list;
@ -4093,7 +4099,9 @@ void doActivate(QObject *sender, int signal_index, void **argv)
if (c->isSingleShot && !QObjectPrivate::removeConnection(c)) if (c->isSingleShot && !QObjectPrivate::removeConnection(c))
continue; 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) { if (c->isSlotObject) {
SlotObjectGuard obj{c->slotObj}; SlotObjectGuard obj{c->slotObj};
@ -4142,7 +4150,7 @@ void doActivate(QObject *sender, int signal_index, void **argv)
senderDeleted = true; senderDeleted = true;
} }
if (!senderDeleted) { if (!senderDeleted) {
sp->connections.loadRelaxed()->cleanOrphanedConnections(sender); sp->connections.loadAcquire()->cleanOrphanedConnections(sender);
if (callbacks_enabled && signal_spy_set->signal_end_callback != nullptr) if (callbacks_enabled && signal_spy_set->signal_end_callback != nullptr)
signal_spy_set->signal_end_callback(sender, signal_index); 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), QOrderedMutexLocker locker(signalSlotLock(sender),
signalSlotLock(receiver)); 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(); 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(); const QObjectPrivate::Connection *c2 = connections->signalVector.loadRelaxed()->at(signal_index).first.loadRelaxed();
while (c2) { while (c2) {
@ -5530,7 +5538,7 @@ inline bool QObjectPrivate::removeConnection(QObjectPrivate::Connection *c)
QtPrivate::QPropertyAdaptorSlotObject * QtPrivate::QPropertyAdaptorSlotObject *
QObjectPrivate::getPropertyAdaptorSlotObject(const QMetaProperty &property) QObjectPrivate::getPropertyAdaptorSlotObject(const QMetaProperty &property)
{ {
if (auto conns = connections.loadRelaxed()) { if (auto conns = connections.loadAcquire()) {
Q_Q(QObject); Q_Q(QObject);
const QMetaObject *metaObject = q->metaObject(); const QMetaObject *metaObject = q->metaObject();
int signal_index = methodIndexToSignalIndex(&metaObject, property.notifySignalIndex()); int signal_index = methodIndexToSignalIndex(&metaObject, property.notifySignalIndex());

11
src/corelib/kernel/qobject_p_p.h
View File

@ -224,19 +224,18 @@ struct QObjectPrivate::ConnectionData
struct QObjectPrivate::Sender 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) : receiver(receiver), sender(sender), signal(signal)
{ {
if (receiver) { if (receiverConnections) {
ConnectionData *cd = receiver->d_func()->connections.loadRelaxed(); previous = receiverConnections->currentSender;
previous = cd->currentSender; receiverConnections->currentSender = this;
cd->currentSender = this;
} }
} }
~Sender() ~Sender()
{ {
if (receiver) if (receiver)
receiver->d_func()->connections.loadRelaxed()->currentSender = previous; receiver->d_func()->connections.loadAcquire()->currentSender = previous;
} }
void receiverDeleted() void receiverDeleted()
{ {