'Re: fix for KConfigPrivate::groupList (kconfig.cpp)'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-devel
Subject:    Re: fix for KConfigPrivate::groupList (kconfig.cpp)
From:       "Friedrich W. H. Kossebau" <kossebau () kde ! org>
Date:       2023-12-29 14:35:40
Message-ID: 3073388.CbtlEUcBR6 () klux
[Download RAW message or body]

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

> 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:

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. 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?

Cheers
Friedrich


[prev in list] [next in list] [prev in thread] [next in thread]
Configure | About | News | Add a list | Sponsored by KoreLogic