From 28c852df3825be05bea9e3d17475c73c409ee534 Mon Sep 17 00:00:00 2001
From: Johan Klokkhammer Helsing <johan.helsing@qt.io>
Date: Mon, 8 Jul 2019 10:22:03 +0200
Subject: 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>
---
 src/client/qwaylandintegration.cpp | 31 ++++++++++++++++++++-----------
 src/client/qwaylandintegration_p.h |  2 ++
 2 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/src/client/qwaylandintegration.cpp b/src/client/qwaylandintegration.cpp
index 97e0203cd..46bef2944 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()
diff --git a/src/client/qwaylandintegration_p.h b/src/client/qwaylandintegration_p.h
index a5a3d7b69..7c1cb978a 100644
--- a/src/client/qwaylandintegration_p.h
+++ b/src/client/qwaylandintegration_p.h
@@ -54,6 +54,7 @@
 #include <QtWaylandClient/qtwaylandclientglobal.h>
 #include <qpa/qplatformintegration.h>
 #include <QtCore/QScopedPointer>
+#include <QtCore/QMutex>
 
 QT_BEGIN_NAMESPACE
 
@@ -148,6 +149,7 @@ private:
     QScopedPointer<QPlatformAccessibility> mAccessibility;
 #endif
     bool mFailed = false;
+    QMutex mClientBufferInitLock;
     bool mClientBufferIntegrationInitialized = false;
     bool mServerBufferIntegrationInitialized = false;
     bool mShellIntegrationInitialized = false;
-- 
cgit v1.2.3