fix for KConfigPrivate::groupList (kconfig.cpp)
Friedrich W. H. Kossebau
kossebau at kde.org
Fri Dec 29 14:35:40 GMT 2023
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
More information about the kde-devel
mailing list