Fix race condition for client buffer integration initialization

This race happened because QWaylandIntegration::clientBufferIntegration (which
lazily initializes the integration) was called from numerous places including
code that may run on different threads without any kind of syncrhonization.

An example of this is Qt3D, which indirectly and simultaneously calls
createPlatformWindow (from the GUI thread) and createPlatformOpenGLContext
(from its render thread), both of which needs to use the client buffer
integration.

In this patch, we fix it by first checking if the integration is initialized.
In that case, it's safe to use it. Otherwise we lock a mutex, re-check if
initialization has happened on another thread in the meantime, and then finally
we initialize it.

This way we should avoid the expense of mutex locking after initialization is
complete, while still staying race free during initialization.

Fixes: QTBUG-76504
Change-Id: I327840ebf41e014882cb659bc3e4fafb7bdb7a98
Reviewed-by: David Edmundson <davidedmundson@kde.org>
Reviewed-by: Paul Olav Tvete <paul.tvete@qt.io>

1 files changed, 20 insertions, 11 deletions

diff --git a/src/client/qwaylandintegration.cpp b/src/client/qwaylandintegration.cpp
index 97e0203c..46bef294 100644
--- a/src/client/qwaylandintegration.cpp
+++ b/src/client/qwaylandintegration.cpp
@@ -301,11 +301,14 @@ QPlatformTheme *QWaylandIntegration::createPlatformTheme(const QString &name) co
     return GenericWaylandTheme::createUnixTheme(name);
 }
 
+// May be called from non-GUI threads
 QWaylandClientBufferIntegration *QWaylandIntegration::clientBufferIntegration() const
 {
-    if (!mClientBufferIntegrationInitialized)
+    // Do an inexpensive check first to avoid locking whenever possible
+    if (Q_UNLIKELY(!mClientBufferIntegrationInitialized))
         const_cast<QWaylandIntegration *>(this)->initializeClientBufferIntegration();
 
+    Q_ASSERT(mClientBufferIntegrationInitialized);
     return mClientBufferIntegration && mClientBufferIntegration->isValid() ? mClientBufferIntegration.data() : nullptr;
 }
 
@@ -325,9 +328,12 @@ QWaylandShellIntegration *QWaylandIntegration::shellIntegration() const
     return mShellIntegration.data();
 }
 
+// May be called from non-GUI threads
 void QWaylandIntegration::initializeClientBufferIntegration()
 {
-    mClientBufferIntegrationInitialized = true;
+    QMutexLocker lock(&mClientBufferInitLock);
+    if (mClientBufferIntegrationInitialized)
+        return;
 
     QString targetKey;
     bool disableHardwareIntegration = qEnvironmentVariableIsSet("QT_WAYLAND_DISABLE_HW_INTEGRATION");
@@ -345,17 +351,20 @@ void QWaylandIntegration::initializeClientBufferIntegration()
 
     if (targetKey.isEmpty()) {
         qWarning("Failed to determine what client buffer integration to use");
-        return;
+    } else {
+        QStringList keys = QWaylandClientBufferIntegrationFactory::keys();
+        if (keys.contains(targetKey)) {
+            mClientBufferIntegration.reset(QWaylandClientBufferIntegrationFactory::create(targetKey, QStringList()));
+        }
+        if (mClientBufferIntegration)
+            mClientBufferIntegration->initialize(mDisplay.data());
+        else
+            qWarning("Failed to load client buffer integration: %s\n", qPrintable(targetKey));
     }
 
-    QStringList keys = QWaylandClientBufferIntegrationFactory::keys();
-    if (keys.contains(targetKey)) {
-        mClientBufferIntegration.reset(QWaylandClientBufferIntegrationFactory::create(targetKey, QStringList()));
-    }
-    if (mClientBufferIntegration)
-        mClientBufferIntegration->initialize(mDisplay.data());
-    else
-        qWarning("Failed to load client buffer integration: %s\n", qPrintable(targetKey));
+    // This must be set last to make sure other threads don't use the
+    // integration before initialization is complete.
+    mClientBufferIntegrationInitialized = true;
 }
 
 void QWaylandIntegration::initializeServerBufferIntegration()