From 0cab9b56e99712c6985eee5af72e53e1fb04113d Mon Sep 17 00:00:00 2001 From: Allan Sandfeld Jensen Date: Wed, 15 Mar 2023 12:18:54 +0100 Subject: [PATCH] Fix UBSAN warnings Convert tagged pointer to an encapsulated tagged pointer type, and clean up where it is used. Change-Id: I9cf5b8d1dfa345eebaa185e44d762d5008769480 Reviewed-by: Thiago Macieira --- src/corelib/kernel/qobject.cpp | 18 ++++++------ src/corelib/kernel/qobject_p.h | 1 + src/corelib/kernel/qobject_p_p.h | 49 +++++++++++++++++++------------- 3 files changed, 39 insertions(+), 29 deletions(-) diff --git a/src/corelib/kernel/qobject.cpp b/src/corelib/kernel/qobject.cpp index d3dead9d8f1..6b7a5dafcb4 100644 --- a/src/corelib/kernel/qobject.cpp +++ b/src/corelib/kernel/qobject.cpp @@ -369,16 +369,16 @@ void QObjectPrivate::ConnectionData::removeConnection(QObjectPrivate::Connection c->prevConnectionList->nextConnectionList.storeRelaxed(n); c->prevConnectionList = nullptr; - Q_ASSERT(c != orphaned.loadRelaxed()); + Q_ASSERT(c != static_cast(orphaned.load(std::memory_order_relaxed))); // add c to orphanedConnections - Connection *o = nullptr; + TaggedSignalVector o = nullptr; /* No ABA issue here: When adding a node, we only care about the list head, it doesn't * matter if the tail changes. */ + o = orphaned.load(std::memory_order_acquire); do { - o = orphaned.loadRelaxed(); c->nextInOrphanList = o; - } while (!orphaned.testAndSetRelease(o, c)); + } while (!orphaned.compare_exchange_strong(o, TaggedSignalVector(c), std::memory_order_release)); #ifndef QT_NO_DEBUG found = false; @@ -396,7 +396,7 @@ void QObjectPrivate::ConnectionData::removeConnection(QObjectPrivate::Connection void QObjectPrivate::ConnectionData::cleanOrphanedConnectionsImpl(QObject *sender, LockPolicy lockPolicy) { QBasicMutex *senderMutex = signalSlotLock(sender); - ConnectionOrSignalVector *c = nullptr; + TaggedSignalVector c = nullptr; { std::unique_lock lock(*senderMutex, std::defer_lock_t{}); if (lockPolicy == NeedToLock) @@ -407,7 +407,7 @@ void QObjectPrivate::ConnectionData::cleanOrphanedConnectionsImpl(QObject *sende // Since ref == 1, no activate() is in process since we locked the mutex. That implies, // that nothing can reference the orphaned connection objects anymore and they can // be safely deleted - c = orphaned.fetchAndStoreRelaxed(nullptr); + c = orphaned.exchange(nullptr, std::memory_order_relaxed); } if (c) { // Deleting c might run arbitrary user code, so we must not hold the lock @@ -421,11 +421,11 @@ void QObjectPrivate::ConnectionData::cleanOrphanedConnectionsImpl(QObject *sende } } -inline void QObjectPrivate::ConnectionData::deleteOrphaned(QObjectPrivate::ConnectionOrSignalVector *o) +inline void QObjectPrivate::ConnectionData::deleteOrphaned(TaggedSignalVector o) { while (o) { - QObjectPrivate::ConnectionOrSignalVector *next = nullptr; - if (SignalVector *v = ConnectionOrSignalVector::asSignalVector(o)) { + TaggedSignalVector next = nullptr; + if (SignalVector *v = static_cast(o)) { next = v->nextInOrphanList; free(v); } else { diff --git a/src/corelib/kernel/qobject_p.h b/src/corelib/kernel/qobject_p.h index 8d382c78ba7..f9bb93f9608 100644 --- a/src/corelib/kernel/qobject_p.h +++ b/src/corelib/kernel/qobject_p.h @@ -111,6 +111,7 @@ public: struct ConnectionOrSignalVector; struct SignalVector; struct Sender; + struct TaggedSignalVector; /* This contains the all connections from and to an object. diff --git a/src/corelib/kernel/qobject_p_p.h b/src/corelib/kernel/qobject_p_p.h index d79ce7e2b93..a9bd290d37f 100644 --- a/src/corelib/kernel/qobject_p_p.h +++ b/src/corelib/kernel/qobject_p_p.h @@ -34,25 +34,35 @@ struct QObjectPrivate::ConnectionList static_assert(std::is_trivially_destructible_v); Q_DECLARE_TYPEINFO(QObjectPrivate::ConnectionList, Q_RELOCATABLE_TYPE); +struct QObjectPrivate::TaggedSignalVector +{ + quintptr c; + + TaggedSignalVector() = default; + TaggedSignalVector(std::nullptr_t) noexcept : c(0) {} + TaggedSignalVector(Connection *v) noexcept : c(reinterpret_cast(v)) { Q_ASSERT(v && (reinterpret_cast(v) & 0x1) == 0); } + TaggedSignalVector(SignalVector *v) noexcept : c(reinterpret_cast(v) | quintptr(1u)) { Q_ASSERT(v); } + explicit operator SignalVector *() const noexcept + { + if (c & 0x1) + return reinterpret_cast(c & ~quintptr(1u)); + return nullptr; + } + explicit operator Connection *() const noexcept + { + return reinterpret_cast(c); + } + operator uintptr_t() const noexcept { return c; } +}; + struct QObjectPrivate::ConnectionOrSignalVector { union { // linked list of orphaned connections that need cleaning up - ConnectionOrSignalVector *nextInOrphanList; + TaggedSignalVector nextInOrphanList; // linked list of connections connected to slots in this object Connection *next; }; - - static SignalVector *asSignalVector(ConnectionOrSignalVector *c) - { - if (reinterpret_cast(c) & 1) - return reinterpret_cast(reinterpret_cast(c) & ~quintptr(1u)); - return nullptr; - } - static Connection *fromSignalVector(SignalVector *v) - { - return reinterpret_cast(reinterpret_cast(v) | quintptr(1u)); - } }; static_assert(std::is_trivial_v); @@ -132,12 +142,12 @@ struct QObjectPrivate::ConnectionData QAtomicPointer signalVector; Connection *senders = nullptr; Sender *currentSender = nullptr; // object currently activating the object - QAtomicPointer orphaned; + std::atomic orphaned = {}; ~ConnectionData() { Q_ASSERT(ref.loadRelaxed() == 0); - auto *c = orphaned.fetchAndStoreRelaxed(nullptr); + TaggedSignalVector c = orphaned.exchange(nullptr, std::memory_order_relaxed); if (c) deleteOrphaned(c); SignalVector *v = signalVector.loadRelaxed(); @@ -159,7 +169,7 @@ struct QObjectPrivate::ConnectionData }; void cleanOrphanedConnections(QObject *sender, LockPolicy lockPolicy = NeedToLock) { - if (orphaned.loadRelaxed() && ref.loadAcquire() == 1) + if (orphaned.load(std::memory_order_relaxed) && ref.loadAcquire() == 1) cleanOrphanedConnectionsImpl(sender, lockPolicy); } void cleanOrphanedConnectionsImpl(QObject *sender, LockPolicy lockPolicy); @@ -194,15 +204,14 @@ struct QObjectPrivate::ConnectionData signalVector.storeRelaxed(newVector); if (vector) { - Connection *o = nullptr; + TaggedSignalVector o = nullptr; /* No ABA issue here: When adding a node, we only care about the list head, it doesn't * matter if the tail changes. */ + o = orphaned.load(std::memory_order_acquire); do { - o = orphaned.loadRelaxed(); vector->nextInOrphanList = o; - } while (!orphaned.testAndSetRelease( - o, ConnectionOrSignalVector::fromSignalVector(vector))); + } while (!orphaned.compare_exchange_strong(o, TaggedSignalVector(vector), std::memory_order_release)); } } int signalVectorCount() const @@ -210,7 +219,7 @@ struct QObjectPrivate::ConnectionData return signalVector.loadAcquire() ? signalVector.loadRelaxed()->count() : -1; } - static void deleteOrphaned(ConnectionOrSignalVector *c); + static void deleteOrphaned(TaggedSignalVector o); }; struct QObjectPrivate::Sender