QApplicationStatic: enforce acquire-release semantics on creation

On systems with weak memory ordering, it was possible for the
storeRelaxed(Initialized) to be observed by another thread performing a
loadRelaxed() without observing the contents of the object itself. The
mutex *does* release the contents of the object to memory, but without
the corresponding mutex acquisition, we couldn't guarantee that the
object's contents would be observed. Now we can.

We don't need to fix the load inside the mutex because the mutex will
have acquired everything from either a previous call to pointer() or to
reset(). The store inside reset() need not be storeRelease() either
because the effect of observing the Uninitialized state will be to lock
the mutex.

None of this is used to protect the data as it is being mutated by the
user in multiple threads, or their access simultaneously with reset()
(which is why the load outside the mutex was removed).

Thanks to litb on Slack for noticing this and bringing to my attention.

Pick-to: 6.5
Change-Id: Idd5e1bb52be047d7b4fffffd1752df5b4d9b2e3f
Reviewed-by: Marc Mutz <marc.mutz@qt.io>

1 files changed, 8 insertions, 8 deletions

diff --git a/src/corelib/kernel/qapplicationstatic.h b/src/corelib/kernel/qapplicationstatic.h
index 2f2cab9174..1eadeb20e2 100644
--- a/src/corelib/kernel/qapplicationstatic.h
+++ b/src/corelib/kernel/qapplicationstatic.h
@@ -30,7 +30,8 @@ template <typename QAS> struct ApplicationHolder
     Q_DISABLE_COPY_MOVE(ApplicationHolder)
     ~ApplicationHolder()
     {
-        if (guard.loadRelaxed() == QtGlobalStatic::Initialized) {
+        if (guard.loadAcquire() == QtGlobalStatic::Initialized) {
+            // No mutex! Up to external code to ensure no race happens.
             guard.storeRelease(QtGlobalStatic::Destroyed);
             realPointer()->~PlainType();
         }
@@ -44,24 +45,23 @@ template <typename QAS> struct ApplicationHolder
     // called from QGlobalStatic::instance()
     PlainType *pointer() noexcept(MutexLockIsNoexcept && ConstructionIsNoexcept)
     {
-        if (guard.loadRelaxed() == QtGlobalStatic::Initialized)
+        if (guard.loadAcquire() == QtGlobalStatic::Initialized)
             return realPointer();
         QMutexLocker locker(&mutex);
         if (guard.loadRelaxed() == QtGlobalStatic::Uninitialized) {
             QAS::innerFunction(&storage);
             QObject::connect(QCoreApplication::instance(), &QObject::destroyed, reset);
-            guard.storeRelaxed(QtGlobalStatic::Initialized);
+            guard.storeRelease(QtGlobalStatic::Initialized);
         }
         return realPointer();
     }
 
     static void reset()
     {
-        if (guard.loadRelaxed() == QtGlobalStatic::Initialized) {
-            QMutexLocker locker(&mutex);
-            realPointer()->~PlainType();
-            guard.storeRelaxed(QtGlobalStatic::Uninitialized);
-        }
+        // we only synchronize using the mutex here, not the guard
+        QMutexLocker locker(&mutex);
+        realPointer()->~PlainType();
+        guard.storeRelaxed(QtGlobalStatic::Uninitialized);
     }
 };
 } // namespace QtGlobalStatic