From ae6f73e8566fa76470937aca737141183929a5ec Mon Sep 17 00:00:00 2001 From: Thiago Macieira Date: Wed, 18 Dec 2019 18:45:36 -0800 Subject: 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 --- src/corelib/plugin/qlibrary.cpp | 42 +++++++++++++++++++++++------------- src/corelib/plugin/qlibrary_p.h | 22 ++++++++++--------- src/corelib/plugin/qlibrary_unix.cpp | 24 +++++++++++---------- src/corelib/plugin/qlibrary_win.cpp | 24 +++++++++++---------- src/corelib/plugin/qpluginloader.cpp | 8 +++---- 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 #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 inst; // used by QFactoryLoader QAtomicPointer::type> instanceFactory; - QJsonObject metaData; + QAtomicPointer::type> pHnd; + // the mutex protects the fields below + QMutex mutex; + QPointer 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 bundleUrl = CFURLCreateFromFileSystemRepresentation(NULL, reinterpret_cast(utf8Bundle.data()), utf8Bundle.length(), true); QCFType bundle = CFBundleCreate(NULL, bundleUrl); @@ -268,23 +269,24 @@ bool QLibraryPrivate::load_sys() char executableFile[FILENAME_MAX]; CFURLGetFileSystemRepresentation(url, true, reinterpret_cast(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(QDir::toNativeSeparators(attempt).utf16())); + hnd = LoadLibrary(reinterpret_cast(QDir::toNativeSeparators(attempt).utf16())); #else // Q_OS_WINRT QString path = QDir::toNativeSeparators(QDir::current().relativeFilePath(attempt)); - pHnd = LoadPackagedLibrary(reinterpret_cast(path.utf16()), 0); - if (pHnd) + hnd = LoadPackagedLibrary(reinterpret_cast(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(pHnd), + reinterpret_cast(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) -- cgit v1.2.3