From 288d3aceee83192bb84d73c60268a18722488adf Mon Sep 17 00:00:00 2001 From: Thiago Macieira Date: Fri, 17 Aug 2012 18:10:23 +0200 Subject: Improve QLibrary global destruction The previous solution was a global static, which got deleted at the end of the execution of the application or at QtCore unload, whichever came first. Unfortunately, the end of the execution often came first, which is inconvenient: it means the global was deleted before all atexit functions were run, including some QLibrary destructors. Consequently, some QLibrary destructors did not reach the global data and were thus unable to unload their libraries or delete their data properly. The previous solution leaked. This solution instead uses a Q_DESTRUCTOR_FUNCTION, which makes a requirement to destroy only at QtCore unload time. Thus, we're sure that all references have been dropped. Additionally, during the cleanup, do try to unload the libraries that have a single reference count left. That means either a QLibrary that was destroyed without unload(), or a use of the static QLibrary::resolve functions. Change-Id: I12e0943b0c6edc27390c103b368d1b04bfe7e302 Reviewed-by: Lars Knoll --- src/corelib/plugin/qlibrary.cpp | 147 +++++++++++++++++++++++++++++----------- 1 file changed, 109 insertions(+), 38 deletions(-) (limited to 'src/corelib/plugin/qlibrary.cpp') diff --git a/src/corelib/plugin/qlibrary.cpp b/src/corelib/plugin/qlibrary.cpp index d17067684a..7d6c3c3063 100644 --- a/src/corelib/plugin/qlibrary.cpp +++ b/src/corelib/plugin/qlibrary.cpp @@ -79,8 +79,6 @@ QT_BEGIN_NAMESPACE # define QT_NO_DEBUG_PLUGIN_CHECK #endif -static QBasicMutex qt_library_mutex; - /*! \class QLibrary \inmodule QtCore @@ -339,46 +337,126 @@ static void installCoverageTool(QLibraryPrivate *libPrivate) #endif } -typedef QMap LibraryMap; +class QLibraryStore +{ +public: + inline ~QLibraryStore(); + static inline QLibraryPrivate *findOrCreate(const QString &fileName, const QString &version); + static inline void releaseLibrary(QLibraryPrivate *lib); + + static inline void cleanup(); -struct LibraryData { +private: + static inline QLibraryStore *instance(); + + // all members and instance() are protected by qt_library_mutex + typedef QMap LibraryMap; LibraryMap libraryMap; - QSet loadedLibs; }; -Q_GLOBAL_STATIC(LibraryData, libraryData) +static QBasicMutex qt_library_mutex; +static QLibraryStore *qt_library_data = 0; + +QLibraryStore::~QLibraryStore() +{ + qt_library_data = 0; +} + +inline void QLibraryStore::cleanup() +{ + QLibraryStore *data = qt_library_data; + if (!data) + return; + + // find any libraries that are still loaded but have a no one attached to them + LibraryMap::Iterator it = data->libraryMap.begin(); + for (; it != data->libraryMap.end(); ++it) { + QLibraryPrivate *lib = it.value(); + if (lib->libraryRefCount.load() == 1) { + Q_ASSERT(lib->pHnd); + Q_ASSERT(lib->libraryUnloadCount.load() > 0); + lib->libraryUnloadCount.store(1); + lib->unload(); + delete lib; + it.value() = 0; + } + } -static LibraryMap *libraryMap() + if (qt_debug_component()) { + // dump all objects that remain + foreach (QLibraryPrivate *lib, data->libraryMap) { + if (lib) + qDebug() << "On QtCore unload," << lib->fileName << "was leaked, with" + << lib->libraryRefCount.load() << "users"; + } + } + + delete data; +} + +static void qlibraryCleanup() +{ + QLibraryStore::cleanup(); +} +Q_DESTRUCTOR_FUNCTION(qlibraryCleanup) + +// must be called with a locked mutex +QLibraryStore *QLibraryStore::instance() { - LibraryData *data = libraryData(); - return data ? &data->libraryMap : 0; + if (Q_UNLIKELY(!qt_library_data)) + qt_library_data = new QLibraryStore; + return qt_library_data; +} + +inline QLibraryPrivate *QLibraryStore::findOrCreate(const QString &fileName, const QString &version) +{ + QMutexLocker locker(&qt_library_mutex); + QLibraryStore *data = instance(); + + // check if this library is already loaded + QLibraryPrivate *lib = data->libraryMap.value(fileName); + if (!lib) + lib = new QLibraryPrivate(fileName, version); + + // track this library + data->libraryMap.insert(fileName, lib); + + lib->libraryRefCount.ref(); + return lib; +} + +inline void QLibraryStore::releaseLibrary(QLibraryPrivate *lib) +{ + QMutexLocker locker(&qt_library_mutex); + QLibraryStore *data = instance(); + + if (lib->libraryRefCount.deref()) { + // still in use + return; + } + + // no one else is using + Q_ASSERT(lib->libraryUnloadCount.load() == 0); + + QLibraryPrivate *that = data->libraryMap.take(lib->fileName); + Q_ASSERT(lib == that); + Q_UNUSED(that); + delete lib; } QLibraryPrivate::QLibraryPrivate(const QString &canonicalFileName, const QString &version) : pHnd(0), fileName(canonicalFileName), fullVersion(version), instance(0), loadHints(0), - libraryRefCount(1), libraryUnloadCount(0), pluginState(MightBeAPlugin) -{ libraryMap()->insert(canonicalFileName, this); } + libraryRefCount(0), libraryUnloadCount(0), pluginState(MightBeAPlugin) +{ } QLibraryPrivate *QLibraryPrivate::findOrCreate(const QString &fileName, const QString &version) { - QMutexLocker locker(&qt_library_mutex); - if (QLibraryPrivate *lib = libraryMap()->value(fileName)) { - lib->libraryRefCount.ref(); - return lib; - } - - return new QLibraryPrivate(fileName, version); + return QLibraryStore::findOrCreate(fileName, version); } QLibraryPrivate::~QLibraryPrivate() { - LibraryMap * const map = libraryMap(); - if (map) { - QLibraryPrivate *that = map->take(fileName); - Q_ASSERT(this == that); - Q_UNUSED(that); - } } QFunctionPointer QLibraryPrivate::resolve(const char *symbol) @@ -391,9 +469,10 @@ QFunctionPointer QLibraryPrivate::resolve(const char *symbol) bool QLibraryPrivate::load() { - libraryUnloadCount.ref(); - if (pHnd) + if (pHnd) { + libraryUnloadCount.ref(); return true; + } if (fileName.isEmpty()) return false; @@ -403,11 +482,8 @@ bool QLibraryPrivate::load() if (ret) { //when loading a library we add a reference to it so that the QLibraryPrivate won't get deleted //this allows to unload the library at a later time - if (LibraryData *lib = libraryData()) { - lib->loadedLibs += this; - libraryRefCount.ref(); - } - + libraryUnloadCount.ref(); + libraryRefCount.ref(); installCoverageTool(this); } @@ -425,10 +501,7 @@ bool QLibraryPrivate::unload() qWarning() << "QLibraryPrivate::unload succeeded on" << fileName; //when the library is unloaded, we release the reference on it so that 'this' //can get deleted - if (LibraryData *lib = libraryData()) { - if (lib->loadedLibs.remove(this)) - libraryRefCount.deref(); - } + libraryRefCount.deref(); pHnd = 0; instance = 0; } @@ -439,9 +512,7 @@ bool QLibraryPrivate::unload() void QLibraryPrivate::release() { - QMutexLocker locker(&qt_library_mutex); - if (!libraryRefCount.deref()) - delete this; + QLibraryStore::releaseLibrary(this); } bool QLibraryPrivate::loadPlugin() -- cgit v1.2.3 From 0da54716647780df0e1b54b03c3e8e007b78bb8a Mon Sep 17 00:00:00 2001 From: Thiago Macieira Date: Sun, 27 Jan 2013 22:38:32 -0800 Subject: Work around an unfixed glibc bug in dlclose(3) on exit During exit, libraries are unloaded and global destructors are run. If we call dlclose(3) from inside the global destructors, we might be telling libdl to unload a module it has already unloaded. I cannot reproduce the issue on my Fedora 17 machine with glibc 2.15, but it could be reliably be reproduced on an Ubuntu 11.10. The assertion is identical to the one reported upstream at http://sourceware.org/bugzilla/show_bug.cgi?id=11941 (see better explanation at https://bugzilla.novell.com/show_bug.cgi?id=622977). I cannot find any evidence in glibc's source code that the bug has been fixed. Change-Id: I97745f89e8c5481196e645dada8762d607a9fb2c Reviewed-by: Lars Knoll --- src/corelib/plugin/qlibrary.cpp | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) (limited to 'src/corelib/plugin/qlibrary.cpp') diff --git a/src/corelib/plugin/qlibrary.cpp b/src/corelib/plugin/qlibrary.cpp index 7d6c3c3063..40b6b0d150 100644 --- a/src/corelib/plugin/qlibrary.cpp +++ b/src/corelib/plugin/qlibrary.cpp @@ -376,7 +376,14 @@ inline void QLibraryStore::cleanup() Q_ASSERT(lib->pHnd); Q_ASSERT(lib->libraryUnloadCount.load() > 0); lib->libraryUnloadCount.store(1); +#ifdef __GLIBC__ + // glibc has a bug in unloading from global destructors + // see https://bugzilla.novell.com/show_bug.cgi?id=622977 + // and http://sourceware.org/bugzilla/show_bug.cgi?id=11941 + lib->unload(QLibraryPrivate::NoUnloadSys); +#else lib->unload(); +#endif delete lib; it.value() = 0; } @@ -490,15 +497,16 @@ bool QLibraryPrivate::load() return ret; } -bool QLibraryPrivate::unload() +bool QLibraryPrivate::unload(UnloadFlag flag) { if (!pHnd) return false; if (!libraryUnloadCount.deref()) { // only unload if ALL QLibrary instance wanted to delete inst.data(); - if (unload_sys()) { + if (flag == NoUnloadSys || unload_sys()) { if (qt_debug_component()) - qWarning() << "QLibraryPrivate::unload succeeded on" << fileName; + qWarning() << "QLibraryPrivate::unload succeeded on" << fileName + << (flag == NoUnloadSys ? "(faked)" : ""); //when the library is unloaded, we release the reference on it so that 'this' //can get deleted libraryRefCount.deref(); -- cgit v1.2.3 From f79cc2d66b16af40aef7af1803c3ba50c939b03f Mon Sep 17 00:00:00 2001 From: Thiago Macieira Date: Tue, 5 Feb 2013 13:39:54 -0800 Subject: Fix crash-on-exit with some plugin systems (e.g. sensors) Don't assume that all entries in the QLibrary global data refer to libraries have have been loaded. There are three (not two) ways of getting entries there: 1) creating a QLibrary 2) using the static QLibrary::resolve 3) via plugins The unload code was meant to handle the first two cases only: libraries are still loaded at the end of the execution if the static methods were called or if QLibrary objects were leaked. It didn't handle the case of plugins being found by the directory scanner in QFactoryLoader but never loaded. Note it's possible that this assertion also happened with leaked QLibrary. Change-Id: Idcd7a551f96d8fe500cbca682f8014f5122b7584 Reviewed-by: Thomas McGuire Reviewed-by: Lars Knoll --- src/corelib/plugin/qlibrary.cpp | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) (limited to 'src/corelib/plugin/qlibrary.cpp') diff --git a/src/corelib/plugin/qlibrary.cpp b/src/corelib/plugin/qlibrary.cpp index 40b6b0d150..236832097a 100644 --- a/src/corelib/plugin/qlibrary.cpp +++ b/src/corelib/plugin/qlibrary.cpp @@ -373,17 +373,18 @@ inline void QLibraryStore::cleanup() for (; it != data->libraryMap.end(); ++it) { QLibraryPrivate *lib = it.value(); if (lib->libraryRefCount.load() == 1) { - Q_ASSERT(lib->pHnd); - Q_ASSERT(lib->libraryUnloadCount.load() > 0); - lib->libraryUnloadCount.store(1); + if (lib->libraryUnloadCount.load() > 0) { + Q_ASSERT(lib->pHnd); + lib->libraryUnloadCount.store(1); #ifdef __GLIBC__ - // glibc has a bug in unloading from global destructors - // see https://bugzilla.novell.com/show_bug.cgi?id=622977 - // and http://sourceware.org/bugzilla/show_bug.cgi?id=11941 - lib->unload(QLibraryPrivate::NoUnloadSys); + // glibc has a bug in unloading from global destructors + // see https://bugzilla.novell.com/show_bug.cgi?id=622977 + // and http://sourceware.org/bugzilla/show_bug.cgi?id=11941 + lib->unload(QLibraryPrivate::NoUnloadSys); #else - lib->unload(); + lib->unload(); #endif + } delete lib; it.value() = 0; } -- cgit v1.2.3 From 2c51bc0289e5f4a8a2cfb4a57562633c1018ab58 Mon Sep 17 00:00:00 2001 From: Thiago Macieira Date: Wed, 20 Feb 2013 14:33:46 -0800 Subject: Don't try to unload a library that isn't loaded Both QPluginLoader::unload() and QLibrary::unload() protect against that (they have a "did_load" member), but QFactoryLoaderPrivate's destructor doesn't. In the past (Qt4) all plugins had to be loaded anyway, so there was no mistake in the reference counting. With Qt 5, we don't load plugins unless they're actually used (in QFactoryLoader::instance). Task-number: QTBUG-29773 Change-Id: I3278fa14bac7e26a9faaf999b4e42e950654ac9a Reviewed-by: Konstantin Ritt Reviewed-by: Lars Knoll --- src/corelib/plugin/qlibrary.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/corelib/plugin/qlibrary.cpp') diff --git a/src/corelib/plugin/qlibrary.cpp b/src/corelib/plugin/qlibrary.cpp index 236832097a..1e17c9fbd4 100644 --- a/src/corelib/plugin/qlibrary.cpp +++ b/src/corelib/plugin/qlibrary.cpp @@ -502,7 +502,7 @@ bool QLibraryPrivate::unload(UnloadFlag flag) { if (!pHnd) return false; - if (!libraryUnloadCount.deref()) { // only unload if ALL QLibrary instance wanted to + if (libraryUnloadCount.load() > 0 && !libraryUnloadCount.deref()) { // only unload if ALL QLibrary instance wanted to delete inst.data(); if (flag == NoUnloadSys || unload_sys()) { if (qt_debug_component()) -- cgit v1.2.3 From cafb02911a29b98ac2652fde64e95870e70fd547 Mon Sep 17 00:00:00 2001 From: Thiago Macieira Date: Thu, 21 Feb 2013 13:58:57 -0800 Subject: Make sure that the reference count for plugins is kept correctly For systems where the Unix signature checker isn't enabled (read: Mac and Windows), QPluginLoader must actually load the plugin to query for the metadata. On Mac it even tried to keep the library loaded to avoid unloading and reloading again when the user calls load(). However, that plus the fact that it was calling load_sys() (on Mac) meant that it would bypass the reference count checking. And on all Unix, if a library-that-wasnt-a-plugin was already loaded by way of a QLibrary, it would have an effect of unloading said library. So remove the "caching" of the library. We should instead invest time to write a proper Mach-O binary decoder. Task-number: QTBUG-29776 Change-Id: Iebbddabe60047aafedeced21f26a170f59656757 Reviewed-by: Liang Qi Reviewed-by: Shawn Rutledge --- src/corelib/plugin/qlibrary.cpp | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) (limited to 'src/corelib/plugin/qlibrary.cpp') diff --git a/src/corelib/plugin/qlibrary.cpp b/src/corelib/plugin/qlibrary.cpp index 1e17c9fbd4..3432f9619d 100644 --- a/src/corelib/plugin/qlibrary.cpp +++ b/src/corelib/plugin/qlibrary.cpp @@ -711,7 +711,7 @@ void QLibraryPrivate::updatePluginState() hTempModule = ::LoadLibraryEx((wchar_t*)QDir::toNativeSeparators(fileName).utf16(), 0, dwFlags); SetErrorMode(oldmode); #else - temporary_load = load_sys(); + temporary_load = load(); #endif } QtPluginQueryVerificationDataFunction getMetaData = NULL; @@ -736,11 +736,10 @@ void QLibraryPrivate::updatePluginState() if (getMetaData) ret = qt_get_metadata(getMetaData, this, &exceptionThrown); + if (temporary_load) + unload(); if (!exceptionThrown) { - if (!ret) { - if (temporary_load) - unload_sys(); - } else { + if (ret) { success = true; } retryLoadLibrary = false; -- cgit v1.2.3