From 5344baee29a50097bbfc3bc283e1617ed63bb92d Mon Sep 17 00:00:00 2001 From: Thiago Macieira Date: Mon, 13 Nov 2023 14:59:20 -0800 Subject: [PATCH] QFactoryLoader::instance(): don't fully parse static plugins' data Commit d9766ddc3d525cf08acec4c3483e61d86c9899a8 (Qt 5.12) replaced the use of the old binary JSON format with CBOR, which is more compact and standard, but requires actual parsing instead of just a quick size verification. For regular, loaded plugins, the metadata is stored in parsed QCborValue format, but for static plugins, we were re-parsing each staticplugin's metadata for every single call. This avoids a full parsing and only parses the CBOR header to find the IIDs (moc always outputs the IID first). Fixes: QTBUG-114253 Change-Id: I8bd6bb457b9c42218247fffd179750ec6c9e3252 Reviewed-by: Eirik Aavitsland Reviewed-by: Thiago Macieira Reviewed-by: Edward Welbourne (cherry picked from commit 4a432a7617d9dd84cc66cd5cffc8469d34001eb9) Reviewed-by: Qt CI Bot --- src/corelib/plugin/qfactoryloader.cpp | 123 +++++++++++++++++- .../plugin/qfactoryloader/CMakeLists.txt | 1 + .../staticplugin/CMakeLists.txt | 14 ++ .../qfactoryloader/staticplugin/main.cpp | 22 ++++ .../qfactoryloader/staticplugin/plugin.json | 3 + .../plugin/qfactoryloader/test/CMakeLists.txt | 4 +- .../qfactoryloader/tst_qfactoryloader.cpp | 45 +++++++ 7 files changed, 204 insertions(+), 8 deletions(-) create mode 100644 tests/auto/corelib/plugin/qfactoryloader/staticplugin/CMakeLists.txt create mode 100644 tests/auto/corelib/plugin/qfactoryloader/staticplugin/main.cpp create mode 100644 tests/auto/corelib/plugin/qfactoryloader/staticplugin/plugin.json diff --git a/src/corelib/plugin/qfactoryloader.cpp b/src/corelib/plugin/qfactoryloader.cpp index f555cb804d8..fce9f098b76 100644 --- a/src/corelib/plugin/qfactoryloader.cpp +++ b/src/corelib/plugin/qfactoryloader.cpp @@ -5,15 +5,13 @@ #include "qfactoryloader_p.h" #ifndef QT_NO_QOBJECT -#include "qfactoryinterface.h" - #include "private/qcoreapplication_p.h" #include "private/qduplicatetracker_p.h" #include "private/qloggingregistry_p.h" #include "private/qobject_p.h" #include "qcborarray.h" #include "qcbormap.h" -#include "qcborvalue.h" +#include "qcborstreamreader.h" #include "qcborvalue.h" #include "qdiriterator.h" #include "qfileinfo.h" @@ -40,6 +38,122 @@ using namespace Qt::StringLiterals; Q_TRACE_POINT(qtcore, QFactoryLoader_update, const QString &fileName); +namespace { +struct IterationResult +{ + enum Result { + FinishedSearch = 0, + ContinueSearch, + + // parse errors + ParsingError = -1, + InvalidMetaDataVersion = -2, + InvalidTopLevelItem = -3, + InvalidHeaderItem = -4, + }; + Result result; + QCborError error = { QCborError::NoError }; + + Q_IMPLICIT IterationResult(Result r) : result(r) {} + Q_IMPLICIT IterationResult(QCborError e) : result(ParsingError), error(e) {} +}; + +struct QFactoryLoaderIidSearch +{ + QLatin1StringView iid; + bool matchesIid = false; + QFactoryLoaderIidSearch(QLatin1StringView iid) : iid(iid) {} + + static QString readString(QCborStreamReader &reader) + { + QString result; + if (!reader.isLengthKnown()) + return result; + auto r = reader.readString(); + if (r.status != QCborStreamReader::Ok) + return result; + result = std::move(r.data); + r = reader.readString(); + if (r.status != QCborStreamReader::EndOfString) + result = QString(); + return result; + } + + static IterationResult::Result skip(QCborStreamReader &reader) + { + // skip this, whatever it is + reader.next(); + return IterationResult::ContinueSearch; + } + + IterationResult::Result operator()(QtPluginMetaDataKeys key, QCborStreamReader &reader) + { + if (key != QtPluginMetaDataKeys::IID) + return skip(reader); + matchesIid = (readString(reader) == iid); + return IterationResult::FinishedSearch; + } + IterationResult::Result operator()(const QCborValue &, QCborStreamReader &reader) + { + return skip(reader); + } +}; +} // unnamed namespace + +template static IterationResult iterateInPluginMetaData(QByteArrayView raw, F &&f) +{ + QPluginMetaData::Header header; + Q_ASSERT(raw.size() >= qsizetype(sizeof(header))); + memcpy(&header, raw.data(), sizeof(header)); + if (Q_UNLIKELY(header.version > QPluginMetaData::CurrentMetaDataVersion)) + return IterationResult::InvalidMetaDataVersion; + + // use fromRawData to keep QCborStreamReader from copying + raw = raw.sliced(sizeof(header)); + QByteArray ba = QByteArray::fromRawData(raw.data(), raw.size()); + QCborStreamReader reader(ba); + if (reader.isInvalid()) + return reader.lastError(); + if (!reader.isMap()) + return IterationResult::InvalidTopLevelItem; + if (!reader.enterContainer()) + return reader.lastError(); + while (reader.isValid()) { + IterationResult::Result r; + if (reader.isInteger()) { + // integer key, one of ours + qint64 value = reader.toInteger(); + auto key = QtPluginMetaDataKeys(value); + if (qint64(key) != value) + return IterationResult::InvalidHeaderItem; + if (!reader.next()) + return reader.lastError(); + r = f(key, reader); + } else { + QCborValue key = QCborValue::fromCbor(reader); + if (key.isInvalid()) + return reader.lastError(); + r = f(key, reader); + } + + if (QCborError e = reader.lastError()) + return e; + if (r != IterationResult::ContinueSearch) + return r; + } + + if (!reader.leaveContainer()) + return reader.lastError(); + return IterationResult::FinishedSearch; +} + +static bool isIidMatch(QByteArrayView raw, QLatin1StringView iid) +{ + QFactoryLoaderIidSearch search(iid); + iterateInPluginMetaData(raw, search); + return search.matchesIid; +} + bool QPluginParsedMetaData::parse(QByteArrayView raw) { QPluginMetaData::Header header; @@ -387,8 +501,7 @@ QObject *QFactoryLoader::instance(int index) const const QList staticPlugins = QPluginLoader::staticPlugins(); for (QStaticPlugin plugin : staticPlugins) { QByteArrayView pluginData(static_cast(plugin.rawMetaData), plugin.rawMetaDataSize); - QPluginParsedMetaData parsed(pluginData); - if (parsed.isError() || parsed.value(QtPluginMetaDataKeys::IID) != iid) + if (!isIidMatch(pluginData, iid)) continue; if (index == 0) diff --git a/tests/auto/corelib/plugin/qfactoryloader/CMakeLists.txt b/tests/auto/corelib/plugin/qfactoryloader/CMakeLists.txt index c55ededca2e..2192a0756ea 100644 --- a/tests/auto/corelib/plugin/qfactoryloader/CMakeLists.txt +++ b/tests/auto/corelib/plugin/qfactoryloader/CMakeLists.txt @@ -3,4 +3,5 @@ add_subdirectory(plugin1) add_subdirectory(plugin2) +add_subdirectory(staticplugin) add_subdirectory(test) diff --git a/tests/auto/corelib/plugin/qfactoryloader/staticplugin/CMakeLists.txt b/tests/auto/corelib/plugin/qfactoryloader/staticplugin/CMakeLists.txt new file mode 100644 index 00000000000..c43a69c707e --- /dev/null +++ b/tests/auto/corelib/plugin/qfactoryloader/staticplugin/CMakeLists.txt @@ -0,0 +1,14 @@ +# Copyright (C) 2022 The Qt Company Ltd. +# SPDX-License-Identifier: BSD-3-Clause + +qt_internal_add_cmake_library(tst_qfactoryloader_staticplugin + STATIC + SOURCES + main.cpp + LIBRARIES + Qt::Core +) + +qt_autogen_tools_initial_setup(tst_qfactoryloader_staticplugin) + +target_compile_definitions(tst_qfactoryloader_staticplugin PRIVATE QT_STATICPLUGIN) diff --git a/tests/auto/corelib/plugin/qfactoryloader/staticplugin/main.cpp b/tests/auto/corelib/plugin/qfactoryloader/staticplugin/main.cpp new file mode 100644 index 00000000000..bce4e186db8 --- /dev/null +++ b/tests/auto/corelib/plugin/qfactoryloader/staticplugin/main.cpp @@ -0,0 +1,22 @@ +// Copyright (C) 2018 Intel Corporation. +// SPDX-License-Identifier: LicenseRef-Qt-Commercial OR GPL-3.0-only WITH Qt-GPL-exception-1.0 +#include +#include + +class StaticPlugin1 : public QObject +{ + Q_OBJECT + Q_PLUGIN_METADATA(IID "StaticPlugin1" FILE "plugin.json") +public: + StaticPlugin1() {} +}; + +class StaticPlugin2 : public QObject +{ + Q_OBJECT + Q_PLUGIN_METADATA(IID "StaticPlugin2" FILE "plugin.json") +public: + StaticPlugin2() {} +}; + +#include "main.moc" diff --git a/tests/auto/corelib/plugin/qfactoryloader/staticplugin/plugin.json b/tests/auto/corelib/plugin/qfactoryloader/staticplugin/plugin.json new file mode 100644 index 00000000000..7321080fb4d --- /dev/null +++ b/tests/auto/corelib/plugin/qfactoryloader/staticplugin/plugin.json @@ -0,0 +1,3 @@ +{ + "Keys": [ "Value" ] +} diff --git a/tests/auto/corelib/plugin/qfactoryloader/test/CMakeLists.txt b/tests/auto/corelib/plugin/qfactoryloader/test/CMakeLists.txt index 7b32c6953aa..fb3b6f5acb8 100644 --- a/tests/auto/corelib/plugin/qfactoryloader/test/CMakeLists.txt +++ b/tests/auto/corelib/plugin/qfactoryloader/test/CMakeLists.txt @@ -13,11 +13,9 @@ qt_internal_add_test(tst_qfactoryloader ../tst_qfactoryloader.cpp LIBRARIES Qt::CorePrivate + tst_qfactoryloader_staticplugin ) -## Scopes: -##################################################################### - qt_internal_extend_target(tst_qfactoryloader CONDITION NOT QT_FEATURE_library LIBRARIES tst_qfactoryloader_plugin1 diff --git a/tests/auto/corelib/plugin/qfactoryloader/tst_qfactoryloader.cpp b/tests/auto/corelib/plugin/qfactoryloader/tst_qfactoryloader.cpp index f1c926d8a39..e9b5e4418f8 100644 --- a/tests/auto/corelib/plugin/qfactoryloader/tst_qfactoryloader.cpp +++ b/tests/auto/corelib/plugin/qfactoryloader/tst_qfactoryloader.cpp @@ -25,6 +25,8 @@ public slots: private slots: void usingTwoFactoriesFromSameDir(); void extraSearchPath(); + void staticPlugin_data(); + void staticPlugin(); }; static const char binFolderC[] = "bin"; @@ -105,5 +107,48 @@ void tst_QFactoryLoader::extraSearchPath() #endif } +Q_IMPORT_PLUGIN(StaticPlugin1) +Q_IMPORT_PLUGIN(StaticPlugin2) +constexpr bool IsDebug = +#ifdef QT_NO_DEBUG + false && +#endif + true; + +void tst_QFactoryLoader::staticPlugin_data() +{ + QTest::addColumn("iid"); + auto addRow = [](const char *iid) { + QTest::addRow("%s", iid) << QString(iid); + }; + addRow("StaticPlugin1"); + addRow("StaticPlugin2"); +} + +void tst_QFactoryLoader::staticPlugin() +{ + QFETCH(QString, iid); + QFactoryLoader loader(iid.toLatin1(), "/irrelevant"); + QFactoryLoader::MetaDataList list = loader.metaData(); + QCOMPARE(list.size(), 1); + + QCborMap map = list.at(0).toCbor(); + QCOMPARE(map[int(QtPluginMetaDataKeys::QtVersion)], + QT_VERSION_CHECK(QT_VERSION_MAJOR, QT_VERSION_MINOR, 0)); + QCOMPARE(map[int(QtPluginMetaDataKeys::IID)], iid); + QCOMPARE(map[int(QtPluginMetaDataKeys::ClassName)], iid); + QCOMPARE(map[int(QtPluginMetaDataKeys::IsDebug)], IsDebug); + + QCborValue metaData = map[int(QtPluginMetaDataKeys::MetaData)]; + QVERIFY(metaData.isMap()); + QCOMPARE(metaData["Keys"], QCborArray{ "Value" }); + QCOMPARE(loader.indexOf("Value"), 0); + + // instantiate + QObject *instance = loader.instance(0); + QVERIFY(instance); + QCOMPARE(instance->metaObject()->className(), iid); +} + QTEST_MAIN(tst_QFactoryLoader) #include "tst_qfactoryloader.moc"