From f6c1cebe42193a62fa0b9c6a881bb1a973b1b8a9 Mon Sep 17 00:00:00 2001
From: Thiago Macieira <thiago.macieira@intel.com>
Date: Wed, 18 Dec 2019 18:17:38 -0800
Subject: QPluginLoader: rework the loading and the caching of instance

There was a race condition in accessing the cached instance factory
member, so rework loadPlugin() to return the cached or newly discovered
instance, with proper, atomic caching. Because I had to change that, I
took the opportunity to fix the QFactoryLoader code that calls
loadPlugin().

Note that QLibraryPrivate::loadPlugin() returns non-nullptr now if the
instance is known, which means the last return in QPluginLoader::load()
will convert to true, not false, if the instance got cached between the
earlier check and the call to loadPlugin(). That's probably what was
intended.

Task-number: QTBUG-39642
Change-Id: I46bf1f65e8db46afbde5fffd15e1a42d2b6cbf2c
Reviewed-by: David Faure <david.faure@kdab.com>
---
 src/corelib/plugin/qlibrary.cpp | 45 ++++++++++++++++++++++++++++++++---------
 1 file changed, 36 insertions(+), 9 deletions(-)

(limited to 'src/corelib/plugin/qlibrary.cpp')

diff --git a/src/corelib/plugin/qlibrary.cpp b/src/corelib/plugin/qlibrary.cpp
index eeaa3c18ec..7ce959cc8f 100644
--- a/src/corelib/plugin/qlibrary.cpp
+++ b/src/corelib/plugin/qlibrary.cpp
@@ -498,7 +498,7 @@ inline void QLibraryStore::releaseLibrary(QLibraryPrivate *lib)
 }
 
 QLibraryPrivate::QLibraryPrivate(const QString &canonicalFileName, const QString &version, QLibrary::LoadHints loadHints)
-    : pHnd(0), fileName(canonicalFileName), fullVersion(version), instance(0),
+    : pHnd(0), fileName(canonicalFileName), fullVersion(version),
       libraryRefCount(0), libraryUnloadCount(0), pluginState(MightBeAPlugin)
 {
     loadHintsInt.storeRelaxed(loadHints);
@@ -539,6 +539,32 @@ void QLibraryPrivate::setLoadHints(QLibrary::LoadHints lh)
     mergeLoadHints(lh);
 }
 
+QObject *QLibraryPrivate::pluginInstance()
+{
+    // first, check if the instance is cached and hasn't been deleted
+    QObject *obj = inst.data();
+    if (obj)
+        return obj;
+
+    // We need to call the plugin's factory function. Is that cached?
+    // skip increasing the reference count (why? -Thiago)
+    QtPluginInstanceFunction factory = instanceFactory.loadAcquire();
+    if (!factory)
+        factory = loadPlugin();
+
+    if (!factory)
+        return nullptr;
+
+    obj = factory();
+
+    // cache again
+    if (inst)
+        obj = inst;
+    else
+        inst = obj;
+    return obj;
+}
+
 bool QLibraryPrivate::load()
 {
     if (pHnd) {
@@ -585,7 +611,7 @@ bool QLibraryPrivate::unload(UnloadFlag flag)
             //can get deleted
             libraryRefCount.deref();
             pHnd = 0;
-            instance = 0;
+            instanceFactory.storeRelaxed(nullptr);
         }
     }
 
@@ -597,22 +623,23 @@ void QLibraryPrivate::release()
     QLibraryStore::releaseLibrary(this);
 }
 
-bool QLibraryPrivate::loadPlugin()
+QtPluginInstanceFunction QLibraryPrivate::loadPlugin()
 {
-    if (instance) {
+    if (auto ptr = instanceFactory.loadAcquire()) {
         libraryUnloadCount.ref();
-        return true;
+        return ptr;
     }
     if (pluginState == IsNotAPlugin)
-        return false;
+        return nullptr;
     if (load()) {
-        instance = (QtPluginInstanceFunction)resolve("qt_plugin_instance");
-        return instance;
+        auto ptr = reinterpret_cast<QtPluginInstanceFunction>(resolve("qt_plugin_instance"));
+        instanceFactory.storeRelease(ptr); // two threads may store the same value
+        return ptr;
     }
     if (qt_debug_component())
         qWarning() << "QLibraryPrivate::loadPlugin failed on" << fileName << ":" << errorString;
     pluginState = IsNotAPlugin;
-    return false;
+    return nullptr;
 }
 
 /*!
-- 
cgit v1.2.3