QFactoryLoader: fix mem-leak on setExtraSearchPath()

When, like in tst_QFactoryLoader::extraSearchPath(), where asan caught
it, or, presumably, on re-creation of a QGuiApplication with a
different QT_QPA_PLATFORM_PLUGIN_PATH, setExtraSearchPath() is called
with a different path than before, then it would leak QLibaryPrivate
objects in the call to libraryList.clear().

Fix by adding QLibraryPrivate::Deleter and holding the objects in
unique_ptr<QLibraryPrivate, Deleter> instead of as raw pointers. This
statically guarantees we're not leaking these objects anywhere else in
QFactoryLoader.

Change the name of the container from libraryList to libraries to catch
any unported users, incl. in older branches.

Since libraryList is now a std::vector (QList cannot hold move-only
types), statically assert that it was never attempted to be copied or
moved, even in older branches, with Q_DISABLE_COPY_MOVE().

Amends ddba24535fb5732c3cb757414cf1a393bd98f693.

Not picking to 6.4 and 6.3, as they are closed at this point.

Pick-to: 6.6 6.5
Fixes: QTBUG-115286
Change-Id: I6d1272622b12c505975cc72f9aba0d126d2817e2
Reviewed-by: Edward Welbourne <edward.welbourne@qt.io>
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>

2 files changed, 21 insertions, 12 deletions

diff --git a/src/corelib/plugin/qfactoryloader.cpp b/src/corelib/plugin/qfactoryloader.cpp
index e3649622bd..f555cb804d 100644
--- a/src/corelib/plugin/qfactoryloader.cpp
+++ b/src/corelib/plugin/qfactoryloader.cpp
@@ -32,6 +32,8 @@
 
 #include <qtcore_tracepoints_p.h>
 
+#include <vector>
+
 QT_BEGIN_NAMESPACE
 
 using namespace Qt::StringLiterals;
@@ -98,6 +100,7 @@ QJsonObject QPluginParsedMetaData::toJson() const
 class QFactoryLoaderPrivate : public QObjectPrivate
 {
     Q_DECLARE_PUBLIC(QFactoryLoader)
+    Q_DISABLE_COPY_MOVE(QFactoryLoaderPrivate)
 public:
     QFactoryLoaderPrivate() { }
     QByteArray iid;
@@ -105,7 +108,7 @@ public:
     ~QFactoryLoaderPrivate();
     mutable QMutex mutex;
     QDuplicateTracker<QString> loadedPaths;
-    QList<QLibraryPrivate*> libraryList;
+    std::vector<QLibraryPrivate::UniquePtr> libraries;
     QMap<QString,QLibraryPrivate*> keyMap;
     QString suffix;
     QString extraSearchPath;
@@ -133,10 +136,7 @@ struct QFactoryLoaderGlobals
 Q_GLOBAL_STATIC(QFactoryLoaderGlobals, qt_factoryloader_global)
 
 QFactoryLoaderPrivate::~QFactoryLoaderPrivate()
-{
-    for (QLibraryPrivate *library : std::as_const(libraryList))
-        library->release();
-}
+    = default;
 
 inline void QFactoryLoaderPrivate::updateSinglePath(const QString &path)
 {
@@ -184,7 +184,7 @@ inline void QFactoryLoaderPrivate::updateSinglePath(const QString &path)
 
         Q_TRACE(QFactoryLoader_update, fileName);
 
-        std::unique_ptr<QLibraryPrivate, LibraryReleaser> library;
+        QLibraryPrivate::UniquePtr library;
         library.reset(QLibraryPrivate::findOrCreate(QFileInfo(fileName).canonicalFilePath()));
         if (!library->isPlugin()) {
             qCDebug(lcFactoryLoader) << library->errorString << Qt::endl
@@ -229,7 +229,7 @@ inline void QFactoryLoaderPrivate::updateSinglePath(const QString &path)
         if (keyUsageCount || keys.isEmpty()) {
             library->setLoadHints(QLibrary::PreventUnloadHint); // once loaded, don't unload
             QMutexLocker locker(&mutex);
-            libraryList += library.release();
+            libraries.push_back(std::move(library));
         }
     };
 }
@@ -328,7 +328,7 @@ void QFactoryLoader::setExtraSearchPath(const QString &path)
     } else {
         // must re-scan everything
         d->loadedPaths.clear();
-        d->libraryList.clear();
+        d->libraries.clear();
         d->keyMap.clear();
         update();
     }
@@ -343,7 +343,7 @@ QFactoryLoader::MetaDataList QFactoryLoader::metaData() const
     QList<QPluginParsedMetaData> metaData;
 #if QT_CONFIG(library)
     QMutexLocker locker(&d->mutex);
-    for (QLibraryPrivate *library : std::as_const(d->libraryList))
+    for (const auto &library : d->libraries)
         metaData.append(library->metaData);
 #endif
 
@@ -369,8 +369,8 @@ QObject *QFactoryLoader::instance(int index) const
 
 #if QT_CONFIG(library)
     QMutexLocker lock(&d->mutex);
-    if (index < d->libraryList.size()) {
-        QLibraryPrivate *library = d->libraryList.at(index);
+    if (size_t(index) < d->libraries.size()) {
+        QLibraryPrivate *library = d->libraries[index].get();
         if (QObject *obj = library->pluginInstance()) {
             if (!obj->parent())
                 obj->moveToThread(QCoreApplicationPrivate::mainThread());
@@ -378,7 +378,8 @@ QObject *QFactoryLoader::instance(int index) const
         }
         return nullptr;
     }
-    index -= d->libraryList.size();
+    // we know d->libraries.size() <= index <= numeric_limits<decltype(index)>::max() → no overflow
+    index -= static_cast<int>(d->libraries.size());
     lock.unlock();
 #endif
 
diff --git a/src/corelib/plugin/qlibrary_p.h b/src/corelib/plugin/qlibrary_p.h
index e3bbbe104b..87d36ee5c8 100644
--- a/src/corelib/plugin/qlibrary_p.h
+++ b/src/corelib/plugin/qlibrary_p.h
@@ -28,6 +28,8 @@
 #  include "QtCore/qt_windows.h"
 #endif
 
+#include <memory>
+
 QT_REQUIRE_CONFIG(library);
 
 QT_BEGIN_NAMESPACE
@@ -54,6 +56,12 @@ public:
 #endif
     enum UnloadFlag { UnloadSys, NoUnloadSys };
 
+    struct Deleter {
+        // QLibraryPrivate::release() is not, yet, and cannot easily be made, noexcept:
+        void operator()(QLibraryPrivate *p) const { p->release(); }
+    };
+    using UniquePtr = std::unique_ptr<QLibraryPrivate, Deleter>;
+
     const QString fileName;
     const QString fullVersion;