From 4a432a7617d9dd84cc66cd5cffc8469d34001eb9 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 Pick-to: 6.6 Change-Id: I8bd6bb457b9c42218247fffd179750ec6c9e3252 Reviewed-by: Eirik Aavitsland Reviewed-by: Thiago Macieira Reviewed-by: Edward Welbourne --- 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 f555cb804d..fce9f098b7 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 72726e36f9..14174da173 100644 --- a/tests/auto/corelib/plugin/qfactoryloader/CMakeLists.txt +++ b/tests/auto/corelib/plugin/qfactoryloader/CMakeLists.txt @@ -9,4 +9,5 @@ endif() 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 0000000000..c43a69c707 --- /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 0000000000..bce4e186db --- /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 0000000000..7321080fb4 --- /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 7b32c6953a..fb3b6f5acb 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 228291de1b..32f14e7a0a 100644 --- a/tests/auto/corelib/plugin/qfactoryloader/tst_qfactoryloader.cpp +++ b/tests/auto/corelib/plugin/qfactoryloader/tst_qfactoryloader.cpp @@ -27,6 +27,8 @@ private slots: void usingTwoFactoriesFromSameDir(); void extraSearchPath(); void multiplePaths(); + void staticPlugin_data(); + void staticPlugin(); }; static const char binFolderC[] = "bin"; @@ -130,5 +132,48 @@ void tst_QFactoryLoader::multiplePaths() #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"