From 9b8124a418f5ac902133652e877181ae414687d9 Mon Sep 17 00:00:00 2001 From: Dominik Holland Date: Wed, 14 Feb 2024 12:37:59 +0100 Subject: Fix memory leaks found by the Address Sanitizer Pick-to: 6.7 6.6 6.5 6.2 Change-Id: I7095ffb9fc26dc6d74b11cf22d73c8cd844a1dcd Reviewed-by: Robert Griebl --- src/interfaceframework/qifconfiguration.cpp | 8 ++++- src/interfaceframework/qifconfiguration_p.h | 1 + src/interfaceframework/qifpendingreply.h | 39 ++++++++-------------- src/interfaceframework/qifproxyserviceobject.cpp | 5 +++ src/interfaceframework/qifproxyserviceobject.h | 1 + src/interfaceframework/qifqmlconversion_helper.cpp | 2 ++ src/interfaceframework/qifservicemanager.cpp | 12 +++++-- src/interfaceframework/qifservicemanager.h | 3 +- src/interfaceframework/qifsimulationengine.cpp | 2 +- .../templates/backend_qtro/backend.cpp.tpl | 4 +-- .../ifcodegen/templates/frontend/interface.cpp.tpl | 12 +++++++ .../ifcodegen/templates/frontend/interface_p.h.tpl | 1 + .../ifcodegen/templates/test/tst_test.cpp.tpl | 6 ++-- .../qifabstractfeature/tst_qifabstractfeature.cpp | 13 ++++---- .../core/qifconfiguration/tst_qifconfiguration.cpp | 3 +- .../tst_qiffilterandbrowsemodel.cpp | 1 + .../core/qifpagingmodel/tst_qifpagingmodel.cpp | 1 + .../core/qifpendingreply/tst_qifpendingreply.cpp | 4 +-- 18 files changed, 73 insertions(+), 45 deletions(-) diff --git a/src/interfaceframework/qifconfiguration.cpp b/src/interfaceframework/qifconfiguration.cpp index 8b0b09c0..18e064e9 100644 --- a/src/interfaceframework/qifconfiguration.cpp +++ b/src/interfaceframework/qifconfiguration.cpp @@ -37,6 +37,10 @@ QIfConfigurationManager *QIfConfigurationManager::instance() return &s_manager; } +QIfConfigurationManager::~QIfConfigurationManager() +{ + qDeleteAll(m_settingsHash.constBegin(), m_settingsHash.constEnd()); +} QIfAbstractFeature::DiscoveryMode discoveryModeFromString(const QString &modeString) { @@ -101,8 +105,10 @@ void QIfConfigurationManager::readInitialSettings(const QString &configPath) if (discoveryModeVariant.isValid()) { auto discoveryMode = discoveryModeFromString(discoveryModeVariant.toString()); - if (discoveryMode == QIfAbstractFeature::InvalidAutoDiscovery) + if (discoveryMode == QIfAbstractFeature::InvalidAutoDiscovery) { + delete settingsObject; return; + } settingsObject->discoveryMode = discoveryMode; settingsObject->discoveryModeSet = true; } diff --git a/src/interfaceframework/qifconfiguration_p.h b/src/interfaceframework/qifconfiguration_p.h index f8e1ed5b..bce8c1b5 100644 --- a/src/interfaceframework/qifconfiguration_p.h +++ b/src/interfaceframework/qifconfiguration_p.h @@ -54,6 +54,7 @@ class Q_QTINTERFACEFRAMEWORK_EXPORT QIfConfigurationManager { public: static QIfConfigurationManager *instance(); + ~QIfConfigurationManager(); void readInitialSettings(const QString &configPath); QIfSettingsObject *settingsObject(const QString &group, bool create = false); diff --git a/src/interfaceframework/qifpendingreply.h b/src/interfaceframework/qifpendingreply.h index fed19031..19f25a10 100644 --- a/src/interfaceframework/qifpendingreply.h +++ b/src/interfaceframework/qifpendingreply.h @@ -115,17 +115,15 @@ public: else if (failed) failed(); } else { - QSharedPointer w = m_watcher; + QWeakPointer w = m_watcher; if (success) { QObject::connect(watcher(), &QIfPendingReplyWatcher::replySuccess, watcher(), [success, w]() { - success(w->value().value()); - }); - } - if (failed) { - QObject::connect(watcher(), &QIfPendingReplyWatcher::replyFailed, watcher(), [failed]() { - failed(); + if (w) + success(w.toStrongRef()->value().value()); }); } + if (failed) + QObject::connect(watcher(), &QIfPendingReplyWatcher::replyFailed, watcher(), failed); } } @@ -164,17 +162,15 @@ public: else if (failed) failed(); } else { - QSharedPointer w = m_watcher; + QWeakPointer w = m_watcher; if (success) { QObject::connect(watcher(), &QIfPendingReplyWatcher::replySuccess, watcher(), [success, w]() { - success(w->value()); - }); - } - if (failed) { - QObject::connect(watcher(), &QIfPendingReplyWatcher::replyFailed, watcher(), [failed]() { - failed(); + if (w) + success(w.toStrongRef()->value()); }); } + if (failed) + QObject::connect(watcher(), &QIfPendingReplyWatcher::replyFailed, watcher(), failed); } } @@ -209,17 +205,10 @@ public: else if (failed) failed(); } else { - QSharedPointer w = m_watcher; - if (success) { - QObject::connect(watcher(), &QIfPendingReplyWatcher::replySuccess, watcher(), [success, w]() { - success(); - }); - } - if (failed) { - QObject::connect(watcher(), &QIfPendingReplyWatcher::replyFailed, watcher(), [failed]() { - failed(); - }); - } + if (success) + QObject::connect(watcher(), &QIfPendingReplyWatcher::replySuccess, watcher(), success); + if (failed) + QObject::connect(watcher(), &QIfPendingReplyWatcher::replyFailed, watcher(), failed); } } diff --git a/src/interfaceframework/qifproxyserviceobject.cpp b/src/interfaceframework/qifproxyserviceobject.cpp index da4e8631..c7f69db9 100644 --- a/src/interfaceframework/qifproxyserviceobject.cpp +++ b/src/interfaceframework/qifproxyserviceobject.cpp @@ -67,6 +67,11 @@ QIfProxyServiceObject::QIfProxyServiceObject(const QHash &interfaceMap, QObject *parent = nullptr); + ~QIfProxyServiceObject() override; QStringList interfaces() const override; QIfFeatureInterface *interfaceInstance(const QString &interface) const override; diff --git a/src/interfaceframework/qifqmlconversion_helper.cpp b/src/interfaceframework/qifqmlconversion_helper.cpp index 3b62c2f0..daa493d5 100644 --- a/src/interfaceframework/qifqmlconversion_helper.cpp +++ b/src/interfaceframework/qifqmlconversion_helper.cpp @@ -112,6 +112,8 @@ QVariant qtif_convertFromJSON(const QVariant &value) } void *gadget = metaType.create(); + auto cleanup = qScopeGuard([gadget, metaType] { metaType.destroy(gadget); }); + if (!Q_UNLIKELY(gadget)) { qWarning("Couldn't create a new instance of %s", metaType.name()); return QVariant(); diff --git a/src/interfaceframework/qifservicemanager.cpp b/src/interfaceframework/qifservicemanager.cpp index 78a93f1a..8d53c617 100644 --- a/src/interfaceframework/qifservicemanager.cpp +++ b/src/interfaceframework/qifservicemanager.cpp @@ -523,8 +523,8 @@ QIfServiceInterface *QIfServiceManagerPrivate::loadServiceBackendInterface(struc For more information about QIfServiceManager and how it works, see its \l{QIfServiceManager}{C++ documentation}. */ -QIfServiceManager::QIfServiceManager() - : QAbstractListModel(nullptr) +QIfServiceManager::QIfServiceManager(QObject *parent) + : QAbstractListModel(parent) , d_ptr(new QIfServiceManagerPrivate(this)) { QtInterfaceFrameworkModule::registerTypes(); @@ -536,7 +536,7 @@ QIfServiceManager::QIfServiceManager() */ QIfServiceManager *QIfServiceManager::instance() { - static auto *instance = new QIfServiceManager(); + static auto *instance = new QIfServiceManager(qApp); return instance; } @@ -547,6 +547,12 @@ QIfServiceManager *QIfServiceManager::create(QQmlEngine *, QJSEngine *) return manager; } +QIfServiceManager::~QIfServiceManager() +{ + unloadAllBackends(); + delete d_ptr; +} + /*! \qmlmethod list ServiceManager::findServiceByInterface(interface, searchFlags, preferredBackends) diff --git a/src/interfaceframework/qifservicemanager.h b/src/interfaceframework/qifservicemanager.h index 3587c37f..c2981cfb 100644 --- a/src/interfaceframework/qifservicemanager.h +++ b/src/interfaceframework/qifservicemanager.h @@ -47,6 +47,7 @@ public: static QIfServiceManager *instance(); static QIfServiceManager *create(QQmlEngine *, QJSEngine *); + ~QIfServiceManager() override; Q_INVOKABLE QList findServiceByInterface(const QString &interface, QIfServiceManager::SearchFlags searchFlags = IncludeAll, const QStringList &preferredBackends = QStringList()); Q_INVOKABLE bool hasInterface(const QString &interface) const; @@ -60,7 +61,7 @@ public: QHash roleNames() const override; private: - explicit QIfServiceManager(); + explicit QIfServiceManager(QObject *parent = nullptr); QIfServiceManagerPrivate * const d_ptr; Q_DECLARE_PRIVATE(QIfServiceManager) }; diff --git a/src/interfaceframework/qifsimulationengine.cpp b/src/interfaceframework/qifsimulationengine.cpp index 8be4ab60..4ac5450b 100644 --- a/src/interfaceframework/qifsimulationengine.cpp +++ b/src/interfaceframework/qifsimulationengine.cpp @@ -232,7 +232,7 @@ QIfSimulationEngine::QIfSimulationEngine(QObject *parent) */ QIfSimulationEngine::QIfSimulationEngine(const QString &identifier, QObject *parent) : QQmlApplicationEngine (parent) - , m_globalObject(new QIfSimulationGlobalObject) + , m_globalObject(new QIfSimulationGlobalObject(this)) , m_identifier(identifier) { rootContext()->setContextProperty(u"IfSimulator"_s, m_globalObject); diff --git a/src/tools/ifcodegen/templates/backend_qtro/backend.cpp.tpl b/src/tools/ifcodegen/templates/backend_qtro/backend.cpp.tpl index bfba9ff7..f7cce9bb 100644 --- a/src/tools/ifcodegen/templates/backend_qtro/backend.cpp.tpl +++ b/src/tools/ifcodegen/templates/backend_qtro/backend.cpp.tpl @@ -63,7 +63,7 @@ void {{zone_class}}::sync() {% for property in interface.properties %} {% if not property.type.is_model %} QRemoteObjectPendingReply<{{property|return_type}}> {{property}}Reply = m_parent->m_replica->{{property|getter_name}}(m_zone); - auto {{property}}Watcher = new QRemoteObjectPendingCallWatcher({{property}}Reply); + auto {{property}}Watcher = new QRemoteObjectPendingCallWatcher({{property}}Reply, this); connect({{property}}Watcher, &QRemoteObjectPendingCallWatcher::finished, this, [this](QRemoteObjectPendingCallWatcher *self) mutable { if (self->error() == QRemoteObjectPendingCallWatcher::NoError) { m_{{property}} = self->returnValue().value<{{property|return_type}}>(); @@ -177,7 +177,7 @@ void {{class}}::syncZones() if (m_replica.isNull()) return; QRemoteObjectPendingReply zoneReply = m_replica->availableZones(); - auto zoneWatcher = new QRemoteObjectPendingCallWatcher(zoneReply); + auto zoneWatcher = new QRemoteObjectPendingCallWatcher(zoneReply, this); connect(zoneWatcher, &QRemoteObjectPendingCallWatcher::finished, this, [this, zoneReply](QRemoteObjectPendingCallWatcher *self) mutable { if (self->error() == QRemoteObjectPendingCallWatcher::NoError) { if (!m_synced) { diff --git a/src/tools/ifcodegen/templates/frontend/interface.cpp.tpl b/src/tools/ifcodegen/templates/frontend/interface.cpp.tpl index 72be30d9..8947cf84 100644 --- a/src/tools/ifcodegen/templates/frontend/interface.cpp.tpl +++ b/src/tools/ifcodegen/templates/frontend/interface.cpp.tpl @@ -60,6 +60,18 @@ using namespace Qt::StringLiterals; {{module.module_name|upperfirst}}::registerTypes(); } +{{class}}Private::~{{class}}Private() +{ +{% for property in interface.properties %} +{% if property.type.is_model %} + if (m_{{property}}) { + delete m_{{property}}->serviceObject(); + delete m_{{property}}; + } +{% endif %} +{% endfor %} +} + /*! \internal */ {{class}}Private *{{class}}Private::get({{class}} *v) { diff --git a/src/tools/ifcodegen/templates/frontend/interface_p.h.tpl b/src/tools/ifcodegen/templates/frontend/interface_p.h.tpl index 83f2e475..0bd0949d 100644 --- a/src/tools/ifcodegen/templates/frontend/interface_p.h.tpl +++ b/src/tools/ifcodegen/templates/frontend/interface_p.h.tpl @@ -56,6 +56,7 @@ public: {{class}}Private(const QString &interface, {{class}} *parent); {% endif %} {% endif %} + ~{{class}}Private(); static {{class}}Private *get({{class}} *p); static const {{class}}Private *get(const {{class}} *p); diff --git a/src/tools/ifcodegen/templates/test/tst_test.cpp.tpl b/src/tools/ifcodegen/templates/test/tst_test.cpp.tpl index cf5cec88..b34a807b 100644 --- a/src/tools/ifcodegen/templates/test/tst_test.cpp.tpl +++ b/src/tools/ifcodegen/templates/test/tst_test.cpp.tpl @@ -36,8 +36,8 @@ class {{interface}}TestBackend : public {{interface}}BackendInterface Q_OBJECT public: - {{interface}}TestBackend() - : {{interface}}BackendInterface() + {{interface}}TestBackend(QObject *parent = nullptr) + : {{interface}}BackendInterface(parent) {% for property in interface.properties %} {% if property.type.is_model %} {% if interface_zoned %} @@ -169,7 +169,7 @@ public: explicit {{interface}}TestServiceObject(QObject *parent=nullptr) : QIfServiceObject(parent), m_name(QLatin1String("")) { - m_backend = new {{interface}}TestBackend; + m_backend = new {{interface}}TestBackend(this); m_interfaces << {{module.module_name|upperfirst}}_{{interface}}_iid; } diff --git a/tests/auto/core/qifabstractfeature/tst_qifabstractfeature.cpp b/tests/auto/core/qifabstractfeature/tst_qifabstractfeature.cpp index 71cbe9f1..e078d60a 100644 --- a/tests/auto/core/qifabstractfeature/tst_qifabstractfeature.cpp +++ b/tests/auto/core/qifabstractfeature/tst_qifabstractfeature.cpp @@ -218,9 +218,9 @@ private: QIfFeatureTester *createTester(bool testBaseFunctions = false) { if (m_isModel) - return new QIfFeatureTester(new TestFeatureListModel(testBaseFunctions)); + return new QIfFeatureTester(new TestFeatureListModel(testBaseFunctions, this), this); else - return new QIfFeatureTester(new TestFeature(testBaseFunctions)); + return new QIfFeatureTester(new TestFeature(testBaseFunctions, this), this); } QIfServiceManager *m_manager; @@ -387,9 +387,9 @@ void BaseTest::testAutoDiscovery_qml() QVERIFY2(obj, qPrintable(component.errorString())); QIfFeatureTester *defaultItem; if (m_isModel) - defaultItem = new QIfFeatureTester(obj->findChild("default")); + defaultItem = new QIfFeatureTester(obj->findChild("default"), this); else - defaultItem = new QIfFeatureTester(obj->findChild("default")); + defaultItem = new QIfFeatureTester(obj->findChild("default"), this); QVERIFY(defaultItem); QCOMPARE(defaultItem->discoveryMode(), QIfAbstractFeature::AutoDiscovery); QVERIFY(defaultItem->serviceObject()); @@ -397,9 +397,9 @@ void BaseTest::testAutoDiscovery_qml() QIfFeatureTester *autoDiscoveryDisabledItem; if (m_isModel) - autoDiscoveryDisabledItem = new QIfFeatureTester(obj->findChild("autoDiscoveryDisabled")); + autoDiscoveryDisabledItem = new QIfFeatureTester(obj->findChild("autoDiscoveryDisabled"), this); else - autoDiscoveryDisabledItem = new QIfFeatureTester(obj->findChild("autoDiscoveryDisabled")); + autoDiscoveryDisabledItem = new QIfFeatureTester(obj->findChild("autoDiscoveryDisabled"), this); QVERIFY(autoDiscoveryDisabledItem); QSignalSpy autoDiscoveryChanged(autoDiscoveryDisabledItem, &QIfFeatureTester::discoveryModeChanged); QSignalSpy serviceObjectChangedSpy(autoDiscoveryDisabledItem, &QIfFeatureTester::serviceObjectChanged); @@ -414,6 +414,7 @@ void BaseTest::testAutoDiscovery_qml() delete defaultItem; delete autoDiscoveryDisabledItem; + delete obj; } void BaseTest::testProxyServiceObject() diff --git a/tests/auto/core/qifconfiguration/tst_qifconfiguration.cpp b/tests/auto/core/qifconfiguration/tst_qifconfiguration.cpp index 8d232e5f..80c63541 100644 --- a/tests/auto/core/qifconfiguration/tst_qifconfiguration.cpp +++ b/tests/auto/core/qifconfiguration/tst_qifconfiguration.cpp @@ -557,7 +557,7 @@ void tst_QIfConfiguration::testServiceObjects() auto backend = new TestBackend; QIfServiceManager::instance()->registerService(backend, QStringList({"testFeature"})); - auto testFeature = new TestFeature; + std::unique_ptr testFeature(new TestFeature); testFeature->startAutoDiscovery(); QVERIFY(testFeature->serviceObject()); @@ -578,6 +578,7 @@ void tst_QIfConfiguration::testServiceObjects() QVERIFY(testFeature2->serviceObject()); QCOMPARE(backend2->serviceSettings, QVariantMap({{"key1", "value1"}})); + delete testFeature2; } void tst_QIfConfiguration::simulationEngine() diff --git a/tests/auto/core/qiffilterandbrowsemodel/tst_qiffilterandbrowsemodel.cpp b/tests/auto/core/qiffilterandbrowsemodel/tst_qiffilterandbrowsemodel.cpp index fb7a6e7f..c9668684 100644 --- a/tests/auto/core/qiffilterandbrowsemodel/tst_qiffilterandbrowsemodel.cpp +++ b/tests/auto/core/qiffilterandbrowsemodel/tst_qiffilterandbrowsemodel.cpp @@ -352,6 +352,7 @@ public: QIfServiceObject(parent) { m_backend = new TestBackend; + m_backend->setParent(this); m_interfaces << QIfFilterAndBrowseModel_iid; } diff --git a/tests/auto/core/qifpagingmodel/tst_qifpagingmodel.cpp b/tests/auto/core/qifpagingmodel/tst_qifpagingmodel.cpp index fc51e870..4c02ec4a 100644 --- a/tests/auto/core/qifpagingmodel/tst_qifpagingmodel.cpp +++ b/tests/auto/core/qifpagingmodel/tst_qifpagingmodel.cpp @@ -141,6 +141,7 @@ public: QIfServiceObject(parent) { m_backend = new TestBackend; + m_backend->setParent(this); m_interfaces << QIfPagingModel_iid; } diff --git a/tests/auto/core/qifpendingreply/tst_qifpendingreply.cpp b/tests/auto/core/qifpendingreply/tst_qifpendingreply.cpp index 9ef54ec7..1914ccb3 100644 --- a/tests/auto/core/qifpendingreply/tst_qifpendingreply.cpp +++ b/tests/auto/core/qifpendingreply/tst_qifpendingreply.cpp @@ -14,7 +14,7 @@ #define TEST_FUNCTION(NAME, TYPE) QIfPendingReply TestObject::test_##NAME(TYPE result, bool fail) { \ QIfPendingReply reply; \ - auto timer = new QTimer(); \ + auto timer = new QTimer(this); \ timer->setSingleShot(true); \ connect(timer, &QTimer::timeout, this, [reply, fail, result]() mutable { \ if (fail) \ @@ -90,7 +90,7 @@ Q_DECLARE_METATYPE(TestGadget) QIfPendingReply TestObject::test_void(bool fail) { QIfPendingReply reply; - auto timer = new QTimer(); + auto timer = new QTimer(this); timer->setSingleShot(true); connect(timer, &QTimer::timeout, this, [reply, fail]() mutable { if (fail) -- cgit v1.2.3