On Fri, Dec 29, 2023 at 3:35 PM Friedrich W. H. Kossebau <kosse...@kde.org> wrote: > > Hi, > > Am Freitag, 29. Dezember 2023, 10:53:45 CET schrieb Tommaso Massimi: > > running plasma-systemmonitor with valgring a lot of problems are declared, > > I'm trying to check them out. > > > > I'm not sure if this is the best way to communicate with the development > > team, > > so I'm writing this mail also to have some indication about that. Please cc > > me, I'm not subscribed to the list > > Ideally the bug/crash itself would have been filed as a report against kconfig > or plasma-systemmonitor (bugs.kde,org). So that related discussion is stored > at a central point with some scheme over it. > > As you even already came up with a patch based on own analysis, you might have > created a MR on invent.kde.org. > > See details at https://community.kde.org/Get_Involved
Thanks > > part of valgrind output (neon unstable development 25-12-2023) > > > > ==70026== Invalid read of size 16 > > ==70026== at 0x668FAF7: ??? (in > > /usr/lib/x86_64-linux-gnu/libQt6Core.so.6.6.1) > > ==70026== by 0x575CB05: calculateHash<QStringView> (qhash.h:57) > > ==70026== by 0x575CB05: > > QHashPrivate::Data<QHashPrivate::Node<QStringView, QHashDummyValue> > > > > >::findBucket(QStringView const&) const [clone .isra.0] (qhash.h:683) > > > > ==70026== by 0x575FF43: findOrInsert (qhash.h:718) > > ==70026== by 0x575FF43: QHash<QStringView, QHashDummyValue>::iterator > > QHash<QStringView, > > QHashDummyValue>::emplace_helper<QHashDummyValue>(QStringView&&, > > QHashDummyValue&&) [clone .isra.0] (qhash.h:1335) > > ==70026== by 0x5761E89: emplace<QHashDummyValue> (qhash.h:1321) > > ==70026== by 0x5761E89: insert (qset.h:158) > > ==70026== by 0x5761E89: operator() (kconfig.cpp:325) > > ==70026== by 0x5761E89: > > forEachEntryWhoseGroupStartsWith<KConfigPrivate::groupList(const QString&) > > const::<lambda(KEntryMapConstIterator)> > (kconfigdata_p.h:252) > > ==70026== by 0x5761E89: KConfigPrivate::groupList(QString const&) const > > (kconfig.cpp:320) > > ==70026== by 0x5771089: KConfigGroup::groupList() const > > (kconfiggroup.cpp:1147) > > ==70026== by 0x1B94F929: PageDataObject::load(KConfigBase const&, > > QString const&) (PageDataObject.cpp:235) > > ==70026== by 0x1B95705E: PagesModel::componentComplete() > > (PagesModel.cpp:99) > > ==70026== by 0x53C1876: > > QQmlObjectCreator::finalize(QQmlInstantiationInterrupt&) (in > > /usr/lib/x86_64-linux-gnu/libQt6Qml.so.6.6.1) > > ==70026== by 0x54489AC: > > QQmlComponentPrivate::complete(QQmlEnginePrivate*, > > QQmlComponentPrivate::ConstructionState*) (in > > /usr/lib/x86_64-linux-gnu/libQt6Qml.so.6.6.1) > > ==70026== by 0x5448CAB: QQmlComponentPrivate::completeCreate() (in > > /usr/lib/x86_64-linux-gnu/libQt6Qml.so.6.6.1) > > ==70026== by 0x544AC88: > > QQmlComponentPrivate::createWithProperties(QObject*, QMap<QString, > > QVariant> const&, QQmlContext*, QQmlComponentPrivate::CreateBehavior) (in > > /usr/lib/x86_64-linux-gnu/libQt6Qml.so.6.6.1) > > ==70026== by 0x54400DF: > > QQmlApplicationEnginePrivate::finishLoad(QQmlComponent*) (in > > /usr/lib/x86_64-linux-gnu/libQt6Qml.so.6.6.1) > > ==70026== Address 0xcd3c40a is 26 bytes inside a block of size 38 alloc'd > > ==70026== at 0x4848899: malloc (in > > /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so) > > ==70026== by 0x65A5677: QArrayData::allocate(QArrayData**, long long, > > long long, long long, QArrayData::AllocationOption) (in > > /usr/lib/x86_64-linux-gnu/libQt6Core.so.6.6.1) > > ==70026== by 0x657DCFE: QString::QString(long long, Qt::Initialization) > > (in /usr/lib/x86_64-linux-gnu/libQt6Core.so.6.6.1) > > ==70026== by 0x6589D97: QString::fromUtf8(QByteArrayView) (in > > /usr/lib/x86_64-linux-gnu/libQt6Core.so.6.6.1) > > ==70026== by 0x577DA4E: fromUtf8<> (qstring.h:588) > > ==70026== by 0x577DA4E: KConfigIniBackend::parseConfig(QByteArray > > const&, KEntryMap&, QFlags<KConfigBackend::ParseOption>, bool) > > (kconfigini.cpp:157) > > ==70026== by 0x5760C68: KConfigPrivate::parseConfigFiles() > > (kconfig.cpp:798) > > ==70026== by 0x5784E81: KSharedConfig::KSharedConfig(QString const&, > > QFlags<KConfig::OpenFlag>, QStandardPaths::StandardLocation) > > (ksharedconfig.cpp:123) > > ==70026== by 0x57854E0: KSharedConfig::openConfig(QString const&, > > QFlags<KConfig::OpenFlag>, QStandardPaths::StandardLocation) > > (ksharedconfig.cpp:88) > > ==70026== by 0x1B957006: PagesModel::componentComplete() > > (PagesModel.cpp:96) > > ==70026== by 0x53C1876: > > QQmlObjectCreator::finalize(QQmlInstantiationInterrupt&) (in > > /usr/lib/x86_64-linux-gnu/libQt6Qml.so.6.6.1) > > ==70026== by 0x54489AC: > > QQmlComponentPrivate::complete(QQmlEnginePrivate*, > > QQmlComponentPrivate::ConstructionState*) (in > > /usr/lib/x86_64-linux-gnu/libQt6Qml.so.6.6.1) > > ==70026== by 0x5448CAB: QQmlComponentPrivate::completeCreate() (in > > /usr/lib/x86_64-linux-gnu/libQt6Qml.so.6.6.1) > > > > > > > > this problem is generated in this function: > > > > > > ==70026== by 0x5761E89: KConfigPrivate::groupList(QString const&) const > > (kconfig.cpp:320) > > > > i.e. > > > > QStringList KConfigPrivate::groupList(const QString &groupName) const > > { > > const QString theGroup = groupName + QLatin1Char('\x1d'); > > QSet<QStringView> groups; > > > > entryMap.forEachEntryWhoseGroupStartsWith(theGroup, [&theGroup, > > &groups](KEntryMapConstIterator entryMapIt) { > > if (isNonDeletedKey(entryMapIt)) { > > const QString &entryGroup = entryMapIt->first.mGroup; > > const auto subgroupStartPos = theGroup.size(); > > const auto subgroupEndPos = findFirstGroupEndPos(entryGroup, > > subgroupStartPos); > > groups.insert(QStringView(entryGroup).mid(subgroupStartPos, > > subgroupEndPos - subgroupStartPos)); > > } > > }); > > > > return stringListFromStringViewCollection(groups); > > } > > > > > > > > in this line the function .mid (deprecated in QStringView) is creating a > > temporary object which is inserted to groups, > > > > groups.insert(QStringView(entryGroup).mid(subgroupStartPos, > > subgroupEndPos - subgroupStartPos)); > > > > > > groups is declared as : > > QSet<QStringView> groups; > > > > QStringView doesn't own data, it is like a wrapper/reference to a qstring. > > so the value inserted on group is like a reference to a temporary qstring; > > but the qstring will be deleted while the QStringView will remain in group > > pointing to garbage > > After a more close look I would partially disagree and point to another > possible cause: correct, my analisys was wrong, I apologize > KConfigPrivate::groupList() is a const method. The member "entryMap" and all > the QString instances it has in its data structure are therefore not changed > by design (beware, KConfigBase API is not thread-safe, see also below). > The whole method juggles with QStringViews on those QString instances, to > avoid doing deep copies on all the string slices handling, unless finally > needed when delivering the result to the caller. > This includes QStringView::mid(), which does not create any temporary QString, > instead returns another QStringView, using the own data pointers: > QStringView(m_data + pos, n); > See complete method e.g. at https://codebrowser.dev/qt6/qtbase/src/corelib/ > text/qstringview.h.html#_ZNK11QStringView3midExx > No temporary QString instances here by what I can see. > > Looking at the backtrace it seems the bad read indeed is in qHash(QStringView > key, size_t seed) and so possibly due to a QStringView with invalid data. So > the question is how with all the const QString instances and the views on them > this can happen. > > Now, the backtrace you shared shows that there are more threads active at the > same time. I'm not sure: valgrind can shown more backtraces, in this case: the backtrace where the error was detected (read unallocated memory) > > ==70026== Invalid read of size 16 > > ==70026== at 0x668FAF7: ??? the backtrace where that memory was allocated > > ==70026== Address 0xcd3c40a is 26 bytes inside a block of size 38 alloc'd > > ==70026== at 0x4848899: malloc (in 70026 is the PID, reported on each line. so I see only one thread. Probably you know this task and you well know that there are other threads > And in some other one KConfig structures are also manipulated, the > one with KConfigIniBackend::parseConfig(...). > The API docs of KConfig state that KSharedConfig::openConfig is thread-safe, > but hints that one should do this per thread and have separate KConfig > instances, see also https://api.kde.org/frameworks/kconfig/html/ > classKSharedConfig.html. > > On a quick look it seems that though the separete KConfig instance creation > happens for both threads, the failing one has code in > PagesModel::componentComplete() to creat an own > copy by auto config = KSharedConfig::openConfig(...), which then is passed on > as the KConfigBase reference on which groupList() is called. > > Based on that I conclude for now the proposed patch might only shadow the > actual cause by chance. Rather comes with a cost for everyone due to more deep > QString instance creations. > > My gut instinct would have me look closer at https://invent.kde.org/ > frameworks/kconfig/-/merge_requests/256 and see if the change from the > implicit shared QMap to std::map did not miss out some special behaviour > relied on before to share data between different thread instances, unless > modified. > > Can you try to revert those changes locally and see if that resolves the bad > read? sure, I'll do. I'm trying to catch more details on current code, I'll let you know if I found some intresting point > Cheers > Friedrich >