1berry/qtbase
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

QLibrary: introduce a mutex to protect non-atomic internals

Browse Source 
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>
This commit is contained in:
Thiago Macieira 2019-12-18 18:45:36 -08:00
parent ef92ac5636
commit ae6f73e856
5 changed files with 68 additions and 52 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

42
src/corelib/plugin/qlibrary.cpp
View File

@ -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;
}
/*!

22
src/corelib/plugin/qlibrary_p.h
View File

@ -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();

24
src/corelib/plugin/qlibrary_unix.cpp
View File

@ -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;
}

24
src/corelib/plugin/qlibrary_win.cpp
View File

@ -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

8
src/corelib/plugin/qpluginloader.cpp
View File

@ -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)