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/qfactoryloader.cpp | 15 ++++--------
 src/corelib/plugin/qlibrary.cpp       | 45 ++++++++++++++++++++++++++++-------
 src/corelib/plugin/qlibrary_p.h       |  6 ++---
 3 files changed, 44 insertions(+), 22 deletions(-)

(limited to 'src/corelib')

diff --git a/src/corelib/plugin/qfactoryloader.cpp b/src/corelib/plugin/qfactoryloader.cpp
index 18f10c9b43..9a6a69676d 100644
--- a/src/corelib/plugin/qfactoryloader.cpp
+++ b/src/corelib/plugin/qfactoryloader.cpp
@@ -389,17 +389,12 @@ QObject *QFactoryLoader::instance(int index) const
     QMutexLocker lock(&d->mutex);
     if (index < d->libraryList.size()) {
         QLibraryPrivate *library = d->libraryList.at(index);
-        if (library->instance || library->loadPlugin()) {
-            if (!library->inst)
-                library->inst = library->instance();
-            QObject *obj = library->inst.data();
-            if (obj) {
-                if (!obj->parent())
-                    obj->moveToThread(QCoreApplicationPrivate::mainThread());
-                return obj;
-            }
+        if (QObject *obj = library->pluginInstance()) {
+            if (!obj->parent())
+                obj->moveToThread(QCoreApplicationPrivate::mainThread());
+            return obj;
         }
-        return 0;
+        return nullptr;
     }
     index -= d->libraryList.size();
     lock.unlock();
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;
 }
 
 /*!
diff --git a/src/corelib/plugin/qlibrary_p.h b/src/corelib/plugin/qlibrary_p.h
index db5afac98e..29724655a3 100644
--- a/src/corelib/plugin/qlibrary_p.h
+++ b/src/corelib/plugin/qlibrary_p.h
@@ -86,7 +86,7 @@ public:
     QString fullVersion;
 
     bool load();
-    bool loadPlugin(); // loads and resolves instance
+    QtPluginInstanceFunction loadPlugin(); // loads and resolves instance
     bool unload(UnloadFlag flag = UnloadSys);
     void release();
     QFunctionPointer resolve(const char *);
@@ -100,8 +100,8 @@ public:
     static QStringList suffixes_sys(const QString &fullVersion);
     static QStringList prefixes_sys();
 
-    QPointer<QObject> inst;
-    QtPluginInstanceFunction instance;
+    QPointer<QObject> inst;         // used by QFactoryLoader
+    QAtomicPointer<std::remove_pointer<QtPluginInstanceFunction>::type> instanceFactory;
     QJsonObject metaData;
 
     QString errorString;
-- 
cgit v1.2.3