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

List:       kde-devel
Subject:    Re: fix for KConfigPrivate::groupList (kconfig.cpp)
From:       Tommaso Massimi <tmassimi () gmail ! com>
Date:       2023-12-29 15:36:41
Message-ID: CAMd1qC4d=TutKRp=2W0xxVce4Piq+Bke8MUsrCxXj5wzr7qkRg () mail ! gmail ! com
[Download RAW message or body]

On Fri, Dec 29, 2023 at 3:35 PM Friedrich W. H. Kossebau
<kossebau@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
>
[prev in list] [next in list] [prev in thread] [next in thread]