Bug 428160 - Crash in libksysguard process reading in TM update
Summary: Crash in libksysguard process reading in TM update
Status: RESOLVED FIXED
Alias: None
Product: plasmashell
Classification: Plasma
Component: Task Manager and Icons-Only Task Manager (show other bugs)
Version: master
Platform: Other Linux
: NOR normal
Target Milestone: 1.0
Assignee: Eike Hein
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-10-23 22:59 UTC by David Edmundson
Modified: 2020-10-26 18:24 UTC (History)
4 users (show)

See Also:
Latest Commit:
Version Fixed In: 5.20.2


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description David Edmundson 2020-10-23 22:59:27 UTC
==17716== Invalid read of size 1
==17716==    at 0x858CE66: void doActivate<false>(QObject*, int, void**) (qobject.cpp:3770)
==17716==    by 0x858698A: QMetaObject::activate(QObject*, QMetaObject const*, int, void**) (qobject.cpp:3946)
==17716==    by 0x121AC166: KSysGuard::AbstractProcesses::processUpdated(long, QVector<QPair<KSysGuard::Process::Change, QVariant> > const&) (moc_processes_base_p.cpp:164)
==17716==    by 0x121DE009: KSysGuard::ProcessesLocal::updateProcessInfo(long, KSysGuard::Process*)::$_2::operator()() const (src/kde/workspace/libksysguard/processcore/processes_linux_p.cpp:559)
==17716==    by 0x121DDF35: QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, KSysGuard::ProcessesLocal::updateProcessInfo(long, KSysGuard::Process*)::$_2>::call(KSysGuard::ProcessesLocal::updateProcessInfo(long, KSysGuard::Process*)::$_2&, void**) (qobjectdefs_impl.h:146)
==17716==    by 0x121DDF00: void QtPrivate::Functor<KSysGuard::ProcessesLocal::updateProcessInfo(long, KSysGuard::Process*)::$_2, 0>::call<QtPrivate::List<>, void>(KSysGuard::ProcessesLocal::updateProcessInfo(long, KSysGuard::Process*)::$_2&, void*, void**) (qobjectdefs_impl.h:256)
==17716==    by 0x121DDEAB: QtPrivate::QFunctorSlotObject<KSysGuard::ProcessesLocal::updateProcessInfo(long, KSysGuard::Process*)::$_2, 0, QtPrivate::List<>, void>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) (qobjectdefs_impl.h:443)
==17716==    by 0x854A180: QtPrivate::QSlotObjectBase::call(QObject*, void**) (qobjectdefs_impl.h:398)
==17716==    by 0x858D429: void doActivate<false>(QObject*, int, void**) (qobject.cpp:3886)
==17716==    by 0x858698A: QMetaObject::activate(QObject*, QMetaObject const*, int, void**) (qobject.cpp:3946)
==17716==    by 0x121AC7E4: KSysGuard::ReadProcSmapsRunnable::finished() (moc_read_procsmaps_runnable.cpp:134)
==17716==    by 0x121F3990: KSysGuard::ReadProcSmapsRunnable::run() (src/kde/workspace/libksysguard/processcore/read_procsmaps_runnable.cpp:37)
==17716==  Address 0x1b4139f0 is 32 bytes inside a block of size 88 free'd
==17716==    at 0x483C08B: operator delete(void*, unsigned long) (vg_replace_malloc.c:593)
==17716==    by 0x857D70A: QObjectPrivate::~QObjectPrivate() (qobject.cpp:237)
==17716==    by 0x858E3C2: QScopedPointerDeleter<QObjectData>::cleanup(QObjectData*) (qscopedpointer.h:60)
==17716==    by 0x858AFE6: QScopedPointer<QObjectData, QScopedPointerDeleter<QObjectData> >::~QScopedPointer() (qscopedpointer.h:107)
==17716==    by 0x857F57C: QObject::~QObject() (qobject.cpp:972)
==17716==    by 0x121ACC97: KSysGuard::AbstractProcesses::~AbstractProcesses() (src/kde/workspace/libksysguard/processcore/processes_base_p.h:48)
==17716==    by 0x121DDD36: KSysGuard::ProcessesLocal::~ProcessesLocal() (src/kde/workspace/libksysguard/processcore/processes_linux_p.cpp:764)
==17716==    by 0x121DDD5B: KSysGuard::ProcessesLocal::~ProcessesLocal() (src/kde/workspace/libksysguard/processcore/processes_linux_p.cpp:762)
==17716==    by 0x121D254C: KSysGuard::Processes::Private::~Private() (src/kde/workspace/libksysguard/processcore/processes.cpp:89)
==17716==    by 0x121D2C8E: KSysGuard::Processes::~Processes() (src/kde/workspace/libksysguard/processcore/processes.cpp:110)
==17716==    by 0x1D281E9C: Backend::parentPid(long long) const (src/kde/workspace/plasma-desktop/applets/taskmanager/plugin/backend.cpp:545)




The fact that we parse all of smaps just to get a parent PID is also somewhat insane.
Comment 1 David Edmundson 2020-10-23 23:03:26 UTC
I think the crash is as follows:

    connect(runnable, &ReadProcSmapsRunnable::finished, this, [this, pid, runnable]() {
        Q_EMIT processUpdated(pid, { { Process::VmPSS, runnable->pss() } });
        runnable->deleteLater();
    }, Qt::DirectConnection);



We set the context to "this" so we should auto disconnect when the Processes object is destroyed...

*BUT*

Qt::DirectConnection

means that this is run in the runner thread

so "this" can be valid at the time of the signal being emitted and null by the time we get to this emit.
Comment 2 David Edmundson 2020-10-23 23:05:47 UTC
Currently reproducible whilst by having Hydrogen from flatpak open. (flatpak is significant as it's fetching PID2 like an idiot...)

Debug shows TM fetches PID data like crazy, it also seems to not have a singleton Processes object backing things.

My cgroup TM patch might suppress this? but we should fix the root cause anyway and also backport something.
Comment 3 David Edmundson 2020-10-23 23:23:28 UTC
Simple fix: https://phabricator.kde.org/P642
Comment 4 Bug Janitor Service 2020-10-25 23:00:26 UTC
A possibly relevant merge request was started @ https://invent.kde.org/plasma/libksysguard/-/merge_requests/90
Comment 5 David Edmundson 2020-10-26 17:25:18 UTC
Git commit 9b5161cb9a16eaad205631b2a4c382dbbbe2072c by David Edmundson.
Committed on 25/10/2020 at 23:00.
Pushed by davidedmundson into branch 'master'.

Handle smap read result in the correct thread

We set the context to "this" so we should auto disconnect when the
Processes object is destroyed.
But Qt::DirectConnection means that is run in the runner thread.

It is possible to have a scenario where "this" is active when the
connection is made, but destroyed before we get to this line.
Related: bug 428048

But scoping the connect to Processes existing we would have leaked the
worker if Processes got destroyed whilst the worker was running.

M  +8    -5    processcore/processes_linux_p.cpp
M  +3    -8    processcore/read_procsmaps_runnable.cpp
M  +1    -4    processcore/read_procsmaps_runnable.h

https://invent.kde.org/plasma/libksysguard/commit/9b5161cb9a16eaad205631b2a4c382dbbbe2072c
Comment 6 David Edmundson 2020-10-26 17:25:54 UTC
Git commit 2d522614357d6dc5a5a7b738a35574f5d8f69da5 by David Edmundson.
Committed on 26/10/2020 at 17:25.
Pushed by davidedmundson into branch 'Plasma/5.20'.

Handle smap read result in the correct thread

We set the context to "this" so we should auto disconnect when the
Processes object is destroyed.
But Qt::DirectConnection means that is run in the runner thread.

It is possible to have a scenario where "this" is active when the
connection is made, but destroyed before we get to this line.
Related: bug 428048

But scoping the connect to Processes existing we would have leaked the
worker if Processes got destroyed whilst the worker was running.


(cherry picked from commit 9b5161cb9a16eaad205631b2a4c382dbbbe2072c)

M  +8    -5    processcore/processes_linux_p.cpp
M  +3    -8    processcore/read_procsmaps_runnable.cpp
M  +1    -4    processcore/read_procsmaps_runnable.h

https://invent.kde.org/plasma/libksysguard/commit/2d522614357d6dc5a5a7b738a35574f5d8f69da5