QLibrary: introduce a mutex to protect non-atomic internals

And make pHnd atomic.

The majority of the variables is updated in QLibraryPrivate::load_sys
and updatePluginState(), which get the mutex protection.
QLibraryPrivate::unload_sys() doesn't need a mutex protection because we
have the refcounting.

[ChangeLog][QtCore][QLibrary & QPluginLoader] Fixed a number of race
conditions caused by having two QLibrary objects pointing to the same
library being operated in different threads.

Fixes: QTBUG-39642
Change-Id: I46bf1f65e8db46afbde5fffd15e1a5b3f5e74ea4
Reviewed-by: Lars Knoll <lars.knoll@qt.io>

5 files changed, 68 insertions, 52 deletions

diff --git a/src/corelib/plugin/qlibrary.cpp b/src/corelib/plugin/qlibrary.cpp
index 7ce959cc8f..ddb053c26f 100644
--- a/src/corelib/plugin/qlibrary.cpp
+++ b/src/corelib/plugin/qlibrary.cpp
@@ -407,7 +407,7 @@ inline void QLibraryStore::cleanup()
         QLibraryPrivate *lib = it.value();
         if (lib->libraryRefCount.loadRelaxed() == 1) {
             if (lib->libraryUnloadCount.loadRelaxed() > 0) {
-                Q_ASSERT(lib->pHnd);
+                Q_ASSERT(lib->pHnd.loadRelaxed());
                 lib->libraryUnloadCount.storeRelaxed(1);
 #ifdef __GLIBC__
                 // glibc has a bug in unloading from global destructors
@@ -498,8 +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),
-      libraryRefCount(0), libraryUnloadCount(0), pluginState(MightBeAPlugin)
+    : fileName(canonicalFileName), fullVersion(version), pluginState(MightBeAPlugin)
 {
     loadHintsInt.storeRelaxed(loadHints);
     if (canonicalFileName.isEmpty())
@@ -519,7 +518,7 @@ QLibraryPrivate::~QLibraryPrivate()
 void QLibraryPrivate::mergeLoadHints(QLibrary::LoadHints lh)
 {
     // if the library is already loaded, we can't change the load hints
-    if (pHnd)
+    if (pHnd.loadRelaxed())
         return;
 
     loadHintsInt.storeRelaxed(lh);
@@ -527,7 +526,7 @@ void QLibraryPrivate::mergeLoadHints(QLibrary::LoadHints lh)
 
 QFunctionPointer QLibraryPrivate::resolve(const char *symbol)
 {
-    if (!pHnd)
+    if (!pHnd.loadRelaxed())
         return 0;
     return resolve_sys(symbol);
 }
@@ -542,7 +541,7 @@ void QLibraryPrivate::setLoadHints(QLibrary::LoadHints lh)
 QObject *QLibraryPrivate::pluginInstance()
 {
     // first, check if the instance is cached and hasn't been deleted
-    QObject *obj = inst.data();
+    QObject *obj = (QMutexLocker(&mutex), inst.data());
     if (obj)
         return obj;
 
@@ -558,6 +557,7 @@ QObject *QLibraryPrivate::pluginInstance()
     obj = factory();
 
     // cache again
+    QMutexLocker locker(&mutex);
     if (inst)
         obj = inst;
     else
@@ -567,7 +567,7 @@ QObject *QLibraryPrivate::pluginInstance()
 
 bool QLibraryPrivate::load()
 {
-    if (pHnd) {
+    if (pHnd.loadRelaxed()) {
         libraryUnloadCount.ref();
         return true;
     }
@@ -576,7 +576,9 @@ bool QLibraryPrivate::load()
 
     Q_TRACE(QLibraryPrivate_load_entry, fileName);
 
+    mutex.lock();
     bool ret = load_sys();
+    mutex.unlock();
     if (qt_debug_component()) {
         if (ret) {
             qDebug() << "loaded library" << fileName;
@@ -599,9 +601,10 @@ bool QLibraryPrivate::load()
 
 bool QLibraryPrivate::unload(UnloadFlag flag)
 {
-    if (!pHnd)
+    if (!pHnd.loadRelaxed())
         return false;
     if (libraryUnloadCount.loadRelaxed() > 0 && !libraryUnloadCount.deref()) { // only unload if ALL QLibrary instance wanted to
+        QMutexLocker locker(&mutex);
         delete inst.data();
         if (flag == NoUnloadSys || unload_sys()) {
             if (qt_debug_component())
@@ -610,12 +613,13 @@ bool QLibraryPrivate::unload(UnloadFlag flag)
             //when the library is unloaded, we release the reference on it so that 'this'
             //can get deleted
             libraryRefCount.deref();
-            pHnd = 0;
+            pHnd.storeRelaxed(nullptr);
             instanceFactory.storeRelaxed(nullptr);
+            return true;
         }
     }
 
-    return (pHnd == 0);
+    return false;
 }
 
 void QLibraryPrivate::release()
@@ -746,6 +750,7 @@ bool QLibraryPrivate::isPlugin()
 
 void QLibraryPrivate::updatePluginState()
 {
+    QMutexLocker locker(&mutex);
     errorString.clear();
     if (pluginState != MightBeAPlugin)
         return;
@@ -766,7 +771,7 @@ void QLibraryPrivate::updatePluginState()
     }
 #endif
 
-    if (!pHnd) {
+    if (!pHnd.loadRelaxed()) {
         // scan for the plugin metadata without loading
         success = findPatternUnloaded(fileName, this);
     } else {
@@ -830,7 +835,7 @@ bool QLibrary::load()
     if (!d)
         return false;
     if (did_load)
-        return d->pHnd;
+        return d->pHnd.loadRelaxed();
     did_load = true;
     return d->load();
 }
@@ -866,7 +871,7 @@ bool QLibrary::unload()
  */
 bool QLibrary::isLoaded() const
 {
-    return d && d->pHnd;
+    return d && d->pHnd.loadRelaxed();
 }
 
 
@@ -977,8 +982,10 @@ void QLibrary::setFileName(const QString &fileName)
 
 QString QLibrary::fileName() const
 {
-    if (d)
+    if (d) {
+        QMutexLocker locker(&d->mutex);
         return d->qualifiedFileName.isEmpty() ? d->fileName : d->qualifiedFileName;
+    }
     return QString();
 }
 
@@ -1119,7 +1126,12 @@ QFunctionPointer QLibrary::resolve(const QString &fileName, const QString &versi
 */
 QString QLibrary::errorString() const
 {
-    return (!d || d->errorString.isEmpty()) ? tr("Unknown error") : d->errorString;
+    QString str;
+    if (d) {
+        QMutexLocker locker(&d->mutex);
+        str = d->errorString;
+    }
+    return str.isEmpty() ? tr("Unknown error") : str;
 }
 
 /*!
diff --git a/src/corelib/plugin/qlibrary_p.h b/src/corelib/plugin/qlibrary_p.h
index 29724655a3..d756126c64 100644
--- a/src/corelib/plugin/qlibrary_p.h
+++ b/src/corelib/plugin/qlibrary_p.h
@@ -54,10 +54,10 @@
 
 #include <QtCore/private/qglobal_p.h>
 #include "QtCore/qlibrary.h"
+#include "QtCore/qmutex.h"
 #include "QtCore/qpointer.h"
 #include "QtCore/qstringlist.h"
 #include "QtCore/qplugin.h"
-#include "QtCore/qsharedpointer.h"
 #ifdef Q_OS_WIN
 #  include "QtCore/qt_windows.h"
 #endif
@@ -72,18 +72,15 @@ class QLibraryStore;
 class QLibraryPrivate
 {
 public:
-
 #ifdef Q_OS_WIN
-    HINSTANCE
+    using Handle = HINSTANCE;
 #else
-    void *
+    using Handle = void *;
 #endif
-    pHnd;
-
     enum UnloadFlag { UnloadSys, NoUnloadSys };
 
-    QString fileName, qualifiedFileName;
-    QString fullVersion;
+    const QString fileName;
+    const QString fullVersion;
 
     bool load();
     QtPluginInstanceFunction loadPlugin(); // loads and resolves instance
@@ -94,17 +91,22 @@ public:
     QLibrary::LoadHints loadHints() const
     { return QLibrary::LoadHints(loadHintsInt.loadRelaxed()); }
     void setLoadHints(QLibrary::LoadHints lh);
+    QObject *pluginInstance();
 
     static QLibraryPrivate *findOrCreate(const QString &fileName, const QString &version = QString(),
                                          QLibrary::LoadHints loadHints = nullptr);
     static QStringList suffixes_sys(const QString &fullVersion);
     static QStringList prefixes_sys();
 
-    QPointer<QObject> inst;         // used by QFactoryLoader
     QAtomicPointer<std::remove_pointer<QtPluginInstanceFunction>::type> instanceFactory;
-    QJsonObject metaData;
+    QAtomicPointer<std::remove_pointer<Handle>::type> pHnd;
 
+    // the mutex protects the fields below
+    QMutex mutex;
+    QPointer<QObject> inst;         // used by QFactoryLoader
+    QJsonObject metaData;
     QString errorString;
+    QString qualifiedFileName;
 
     void updatePluginState();
     bool isPlugin();
diff --git a/src/corelib/plugin/qlibrary_unix.cpp b/src/corelib/plugin/qlibrary_unix.cpp
index 23f96dbdaf..29813e5863 100644
--- a/src/corelib/plugin/qlibrary_unix.cpp
+++ b/src/corelib/plugin/qlibrary_unix.cpp
@@ -214,8 +214,9 @@ bool QLibraryPrivate::load_sys()
 #endif
 
     bool retry = true;
-    for(int prefix = 0; retry && !pHnd && prefix < prefixes.size(); prefix++) {
-        for(int suffix = 0; retry && !pHnd && suffix < suffixes.size(); suffix++) {
+    Handle hnd = nullptr;
+    for (int prefix = 0; retry && !hnd && prefix < prefixes.size(); prefix++) {
+        for (int suffix = 0; retry && !hnd && suffix < suffixes.size(); suffix++) {
             if (!prefixes.at(prefix).isEmpty() && name.startsWith(prefixes.at(prefix)))
                 continue;
             if (path.isEmpty() && prefixes.at(prefix).contains(QLatin1Char('/')))
@@ -232,7 +233,7 @@ bool QLibraryPrivate::load_sys()
                 attempt = path + prefixes.at(prefix) + name + suffixes.at(suffix);
             }
 
-            pHnd = dlopen(QFile::encodeName(attempt), dlFlags);
+            hnd = dlopen(QFile::encodeName(attempt), dlFlags);
 #ifdef Q_OS_ANDROID
             if (!pHnd) {
                 auto attemptFromBundle = attempt;
@@ -248,7 +249,7 @@ bool QLibraryPrivate::load_sys()
             }
 #endif
 
-            if (!pHnd && fileName.startsWith(QLatin1Char('/')) && QFile::exists(attempt)) {
+            if (!hnd && fileName.startsWith(QLatin1Char('/')) && QFile::exists(attempt)) {
                 // We only want to continue if dlopen failed due to that the shared library did not exist.
                 // However, we are only able to apply this check for absolute filenames (since they are
                 // not influenced by the content of LD_LIBRARY_PATH, /etc/ld.so.cache, DT_RPATH etc...)
@@ -259,7 +260,7 @@ bool QLibraryPrivate::load_sys()
     }
 
 #ifdef Q_OS_MAC
-    if (!pHnd) {
+    if (!hnd) {
         QByteArray utf8Bundle = fileName.toUtf8();
         QCFType<CFURLRef> bundleUrl = CFURLCreateFromFileSystemRepresentation(NULL, reinterpret_cast<const UInt8*>(utf8Bundle.data()), utf8Bundle.length(), true);
         QCFType<CFBundleRef> bundle = CFBundleCreate(NULL, bundleUrl);
@@ -268,23 +269,24 @@ bool QLibraryPrivate::load_sys()
             char executableFile[FILENAME_MAX];
             CFURLGetFileSystemRepresentation(url, true, reinterpret_cast<UInt8*>(executableFile), FILENAME_MAX);
             attempt = QString::fromUtf8(executableFile);
-            pHnd = dlopen(QFile::encodeName(attempt), dlFlags);
+            hnd = dlopen(QFile::encodeName(attempt), dlFlags);
         }
     }
 #endif
-    if (!pHnd) {
+    if (!hnd) {
         errorString = QLibrary::tr("Cannot load library %1: %2").arg(fileName, qdlerror());
     }
-    if (pHnd) {
+    if (hnd) {
         qualifiedFileName = attempt;
         errorString.clear();
     }
-    return (pHnd != 0);
+    pHnd.storeRelaxed(hnd);
+    return (hnd != nullptr);
 }
 
 bool QLibraryPrivate::unload_sys()
 {
-    if (dlclose(pHnd)) {
+    if (dlclose(pHnd.loadAcquire())) {
 #if defined (Q_OS_QNX)                // Workaround until fixed in QNX; fixes crash in
         char *error = dlerror();      // QtDeclarative auto test "qqmlenginecleanup" for instance
         if (!qstrcmp(error, "Shared objects still referenced")) // On QNX that's only "informative"
@@ -316,7 +318,7 @@ Q_CORE_EXPORT QFunctionPointer qt_mac_resolve_sys(void *handle, const char *symb
 
 QFunctionPointer QLibraryPrivate::resolve_sys(const char* symbol)
 {
-    QFunctionPointer address = QFunctionPointer(dlsym(pHnd, symbol));
+    QFunctionPointer address = QFunctionPointer(dlsym(pHnd.loadAcquire(), symbol));
     return address;
 }
 
diff --git a/src/corelib/plugin/qlibrary_win.cpp b/src/corelib/plugin/qlibrary_win.cpp
index c8309104aa..000bf76276 100644
--- a/src/corelib/plugin/qlibrary_win.cpp
+++ b/src/corelib/plugin/qlibrary_win.cpp
@@ -95,26 +95,27 @@ bool QLibraryPrivate::load_sys()
         attempts.prepend(QDir::rootPath() + fileName);
 #endif
 
+    Handle hnd = nullptr;
     for (const QString &attempt : qAsConst(attempts)) {
 #ifndef Q_OS_WINRT
-        pHnd = LoadLibrary(reinterpret_cast<const wchar_t*>(QDir::toNativeSeparators(attempt).utf16()));
+        hnd = LoadLibrary(reinterpret_cast<const wchar_t*>(QDir::toNativeSeparators(attempt).utf16()));
 #else // Q_OS_WINRT
         QString path = QDir::toNativeSeparators(QDir::current().relativeFilePath(attempt));
-        pHnd = LoadPackagedLibrary(reinterpret_cast<LPCWSTR>(path.utf16()), 0);
-        if (pHnd)
+        hnd = LoadPackagedLibrary(reinterpret_cast<LPCWSTR>(path.utf16()), 0);
+        if (hnd)
             qualifiedFileName = attempt;
 #endif // !Q_OS_WINRT
 
         // If we have a handle or the last error is something other than "unable
         // to find the module", then bail out
-        if (pHnd || ::GetLastError() != ERROR_MOD_NOT_FOUND)
+        if (hnd || ::GetLastError() != ERROR_MOD_NOT_FOUND)
             break;
     }
 
 #ifndef Q_OS_WINRT
     SetErrorMode(oldmode);
 #endif
-    if (!pHnd) {
+    if (!hnd) {
         errorString = QLibrary::tr("Cannot load library %1: %2").arg(
                     QDir::toNativeSeparators(fileName), qt_error_string());
     } else {
@@ -123,7 +124,7 @@ bool QLibraryPrivate::load_sys()
 
 #ifndef Q_OS_WINRT
         wchar_t buffer[MAX_PATH];
-        ::GetModuleFileName(pHnd, buffer, MAX_PATH);
+        ::GetModuleFileName(hnd, buffer, MAX_PATH);
 
         QString moduleFileName = QString::fromWCharArray(buffer);
         moduleFileName.remove(0, 1 + moduleFileName.lastIndexOf(QLatin1Char('\\')));
@@ -138,19 +139,20 @@ bool QLibraryPrivate::load_sys()
             HMODULE hmod;
             bool ok = GetModuleHandleEx(GET_MODULE_HANDLE_EX_FLAG_PIN |
                                         GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS,
-                                        reinterpret_cast<const wchar_t *>(pHnd),
+                                        reinterpret_cast<const wchar_t *>(hnd),
                                         &hmod);
-            Q_ASSERT(!ok || hmod == pHnd);
+            Q_ASSERT(!ok || hmod == hnd);
             Q_UNUSED(ok);
         }
 #endif // !Q_OS_WINRT
     }
-    return (pHnd != 0);
+    pHnd.storeRelaxed(hnd);
+    return (pHnd != nullptr);
 }
 
 bool QLibraryPrivate::unload_sys()
 {
-    if (!FreeLibrary(pHnd)) {
+    if (!FreeLibrary(pHnd.loadAcquire())) {
         errorString = QLibrary::tr("Cannot unload library %1: %2").arg(
                     QDir::toNativeSeparators(fileName),  qt_error_string());
         return false;
@@ -161,7 +163,7 @@ bool QLibraryPrivate::unload_sys()
 
 QFunctionPointer QLibraryPrivate::resolve_sys(const char* symbol)
 {
-    FARPROC address = GetProcAddress(pHnd, symbol);
+    FARPROC address = GetProcAddress(pHnd.loadAcquire(), symbol);
     return QFunctionPointer(address);
 }
 QT_END_NAMESPACE
diff --git a/src/corelib/plugin/qpluginloader.cpp b/src/corelib/plugin/qpluginloader.cpp
index dac8502dae..0e12793d5e 100644
--- a/src/corelib/plugin/qpluginloader.cpp
+++ b/src/corelib/plugin/qpluginloader.cpp
@@ -196,9 +196,7 @@ QObject *QPluginLoader::instance()
 {
     if (!isLoaded() && !load())
         return 0;
-    if (!d->inst && d->instance)
-        d->inst = d->instance();
-    return d->inst.data();
+    return d->pluginInstance();
 }
 
 /*!
@@ -233,7 +231,7 @@ bool QPluginLoader::load()
     if (!d || d->fileName.isEmpty())
         return false;
     if (did_load)
-        return d->pHnd && d->instance;
+        return d->pHnd && d->instanceFactory.loadAcquire();
     if (!d->isPlugin())
         return false;
     did_load = true;
@@ -275,7 +273,7 @@ bool QPluginLoader::unload()
  */
 bool QPluginLoader::isLoaded() const
 {
-    return d && d->pHnd && d->instance;
+    return d && d->pHnd && d->instanceFactory.loadRelaxed();
 }
 
 #if defined(QT_SHARED)