Avoid crash in KWin::DrmOutput::updateCursor
ClosedPublic

Authored by meven on Apr 16 2020, 4:14 PM.

Details

Summary

BUG: 420077

Sample stack traces :

From bug:

#2  QImage::copy (this=this@entry=0x558117775e20, r=...) at image/qimage.cpp:1172
#3  0x00007f22d0a24cdf in QImage::detach (this=this@entry=0x558117775e20) at image/qimage.cpp:1091
#4  0x00007f22d0a25ae0 in QImage::fill (this=0x558117775e20, color=...) at image/qimage.cpp:1806
#5  0x00007f22d0a25f5f in QImage::fill (this=this@entry=0x558117775e20, color=color@entry=Qt::transparent) at image/qimage.cpp:1780
#6  0x00007f22bf3bdffd in KWin::DrmOutput::updateCursor (this=0x5581176fb780) at ./plugins/platforms/drm/drm_output.cpp:175
#7  0x00007f22bf3b0e55 in KWin::DrmBackend::updateCursor (this=0x558117669b60) at ./plugins/platforms/drm/drm_backend.cpp:701

Locally reproduced:

#0  0x00007f360611e159 in KWayland::Server::OutputDeviceInterface::transform() const (this=<optimized out>)
    at /home/meven/kde/src/kwayland/src/server/outputdevice_interface.cpp:590
#1  0x00007f3607438059 in KWin::AbstractWaylandOutput::transform() const (this=this@entry=0x5645bed10f90) at /home/meven/kde/src/kwin/abstract_wayland_output.cpp:317
#2  0x00007f35ecd8acd3 in KWin::DrmOutput::matrixDisplay(QSize const&) const (this=0x5645bed10f90, s=...)
    at /home/meven/kde/src/kwin/plugins/platforms/drm/drm_output.cpp:155
#3  0x00007f35ecd8efa9 in KWin::DrmOutput::updateCursor() (this=<optimized out>) at /home/meven/kde/src/kwin/plugins/platforms/drm/drm_output.cpp:179
#4  0x00007f35ecd81db5 in KWin::DrmBackend::updateCursor() (this=0x5645bec743a0) at /home/meven/kde/src/kwin/plugins/platforms/drm/drm_backend.cpp:701
#5  0x00007f36049e7fe7 in  () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#6  0x00007f36075ee43f in KWin::Cursors::currentCursorChanged(KWin::Cursor*) (this=<optimized out>, _t1=<optimized out>)
    at /home/meven/kde/build/kwin/kwin_autogen/EWIEGA46WW/moc_cursor.cpp:385
Test Plan

Could not reproduce

Diff Detail

Repository
R108 KWin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
meven created this revision.Apr 16 2020, 4:14 PM
Restricted Application added a project: KWin. · View Herald TranscriptApr 16 2020, 4:14 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
meven requested review of this revision.Apr 16 2020, 4:14 PM
meven edited the summary of this revision. (Show Details)Apr 16 2020, 4:15 PM
meven added a comment.Apr 16 2020, 4:17 PM

I noticed another bug where after kwin_wayland stops normally, we get a black screen with no more input working, suggesting libinput handle was not released.

meven added a comment.Apr 16 2020, 4:22 PM

Well in fact a kwin_wayland prcess stays :

39      ../sysdeps/unix/sysv/linux/ppoll.c: Aucun fichier ou dossier de ce type.
(gdb) bt
#0  0x00007f7eef107cf6 in __GI_ppoll (fds=0x564c64bc8678, nfds=6, timeout=<optimized out>, sigmask=0x0) at ../sysdeps/unix/sysv/linux/ppoll.c:39
#1  0x00007f7eefc91df1 in qt_safe_poll(pollfd*, unsigned long, timespec const*) () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#2  0x00007f7eefc935b2 in QEventDispatcherUNIX::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#3  0x00007f7edb63dc1d in  () at /usr/lib/x86_64-linux-gnu/qt5/plugins/platforms/KWinQpaPlugin.so
#4  0x0000564c62b34882 in  ()
#5  0x0000564c62b34969 in  ()
#6  0x0000564c62b21a57 in  ()
#7  0x0000564c62b1f828 in  ()
#8  0x00007f7eef014b97 in __libc_start_main (main=
    0x564c62b1e5a0, argc=4, argv=0x7ffdbec40ce8, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7ffdbec40cd8) at ../csu/libc-start.c:310
#9  0x0000564c62b2084a in _start ()
meven added a comment.Apr 16 2020, 4:26 PM
Thread 1 has target name 'kwin_wayland'
Thread 1 has target id 'Thread 0x7f7ef2cdc3c0 (LWP 9387)'
Thread 2 has target name 'QDBusConnection'
Thread 2 has target id 'Thread 0x7f7ed8408700 (LWP 9388)'
Thread 3 has target name 'QThread'
Thread 3 has target id 'Thread 0x7f7ed75a3700 (LWP 9389)'
Thread 4 has target name 'libinput-connec'
Thread 4 has target id 'Thread 0x7f7ed6da2700 (LWP 9390)'
Thread 5 has target name 'kwin_wa:disk$0'
Thread 5 has target id 'Thread 0x7f7ec61c3700 (LWP 9391)'
Thread 6 has target name 'Thread (pooled)'
Thread 6 has target id 'Thread 0x7f7ebffff700 (LWP 9393)'
(gdb) thread apply all bt

Thread 6 (Thread 0x7f7ebffff700 (LWP 9393)):
#0  0x00007f7ef110e384 in __libc_read (fd=23, buf=0x7f7ebfffeba7, nbytes=1) at ../sysdeps/unix/sysv/linux/read.c:27
#1  0x00007f7eefb93e55 in  () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#2  0x00007f7eefbc343e in  () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#3  0x00007f7eefb41964 in QAbstractFileEngine::readLine(char*, long long) () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#4  0x00007f7eefb94450 in  () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#5  0x00007f7eefb5a14f in QFileDevice::readLineData(char*, long long) () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#6  0x00007f7eefb62ecb in QIODevice::readLine(char*, long long) () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#7  0x00007f7eefb6332d in QIODevice::readLine(long long) () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#8  0x0000564c62b353b9 in  ()
#9  0x0000564c62b36346 in  ()
#10 0x00007f7eefa4af42 in  () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#11 0x00007f7eefa477ec in  () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#12 0x00007f7ef11046db in start_thread (arg=0x7f7ebffff700) at pthread_create.c:463
#13 0x00007f7eef11488f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95

Thread 5 (Thread 0x7f7ec61c3700 (LWP 9391)):
#0  0x00007f7ef110a9f3 in futex_wait_cancelable (private=<optimized out>, expected=0, futex_word=0x564c64a97408) at ../sysdeps/unix/sysv/linux/futex-internal.h:88
#1  0x00007f7ef110a9f3 in __pthread_cond_wait_common (abstime=0x0, mutex=0x564c64a973b8, cond=0x564c64a973e0) at pthread_cond_wait.c:502
#2  0x00007f7ef110a9f3 in __pthread_cond_wait (cond=0x564c64a973e0, mutex=0x564c64a973b8) at pthread_cond_wait.c:655
#3  0x00007f7ec763decb in  () at /usr/lib/x86_64-linux-gnu/dri/i965_dri.so
#4  0x00007f7ec763dac7 in  () at /usr/lib/x86_64-linux-gnu/dri/i965_dri.so
#5  0x00007f7ef11046db in start_thread (arg=0x7f7ec61c3700) at pthread_create.c:463
#6  0x00007f7eef11488f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95

Thread 4 (Thread 0x7f7ed6da2700 (LWP 9390)):
#0  0x00007f7eef107bf9 in __GI___poll (fds=0x7f7ecc0029e0, nfds=2, timeout=-1) at ../sysdeps/unix/sysv/linux/poll.c:29
#1  0x00007f7ee62a65c9 in  () at /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
#2  0x00007f7ee62a66dc in g_main_context_iteration () at /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
#3  0x00007f7eefc960db in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#4  0x00007f7eefc3563a in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#5  0x00007f7eefa46317 in QThread::exec() () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#6  0x00007f7eefa477ec in  () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#7  0x00007f7ef11046db in start_thread (arg=0x7f7ed6da2700) at pthread_create.c:463
#8  0x00007f7eef11488f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95

Thread 3 (Thread 0x7f7ed75a3700 (LWP 9389)):
#0  0x00007f7eef107bf9 in __GI___poll (fds=0x7f7ec8002de0, nfds=2, timeout=-1) at ../sysdeps/unix/sysv/linux/poll.c:29
#1  0x00007f7ee62a65c9 in  () at /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
#2  0x00007f7ee62a66dc in g_main_context_iteration () at /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
#3  0x00007f7eefc960bc in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#4  0x00007f7eefc3563a in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#5  0x00007f7eefa46317 in QThread::exec() () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#6  0x00007f7eefa477ec in  () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#7  0x00007f7ef11046db in start_thread (arg=0x7f7ed75a3700) at pthread_create.c:463
#8  0x00007f7eef11488f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95

Thread 2 (Thread 0x7f7ed8408700 (LWP 9388)):
#0  0x00007f7eef107bf9 in __GI___poll (fds=0x7f7ed00053d0, nfds=2, timeout=-1) at ../sysdeps/unix/sysv/linux/poll.c:29
#1  0x00007f7ee62a65c9 in  () at /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
#2  0x00007f7ee62a66dc in g_main_context_iteration () at /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0
#3  0x00007f7eefc960bc in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#4  0x00007f7eefc3563a in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#5  0x00007f7eefa46317 in QThread::exec() () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#6  0x00007f7ef0e83555 in  () at /usr/lib/x86_64-linux-gnu/libQt5DBus.so.5
#7  0x00007f7eefa477ec in  () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#8  0x00007f7ef11046db in start_thread (arg=0x7f7ed8408700) at pthread_create.c:463
#9  0x00007f7eef11488f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95

Thread 1 (Thread 0x7f7ef2cdc3c0 (LWP 9387)):
#0  0x00007f7eef107cf6 in __GI_ppoll (fds=0x564c64bc8678, nfds=6, timeout=<optimized out>, sigmask=0x0) at ../sysdeps/unix/sysv/linux/ppoll.c:39
#1  0x00007f7eefc91df1 in qt_safe_poll(pollfd*, unsigned long, timespec const*) () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#2  0x00007f7eefc935b2 in QEventDispatcherUNIX::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () at /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#3  0x00007f7edb63dc1d in  () at /usr/lib/x86_64-linux-gnu/qt5/plugins/platforms/KWinQpaPlugin.so
#4  0x0000564c62b34882 in  ()
#5  0x0000564c62b34969 in  ()
#6  0x0000564c62b21a57 in  ()
#7  0x0000564c62b1f828 in  ()
#8  0x00007f7eef014b97 in __libc_start_main (main=
    0x564c62b1e5a0, argc=4, argv=0x7ffdbec40ce8, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7ffdbec40cd8) at ../csu/libc-start.c:310
#9  0x0000564c62b2084a in _start ()
davidedmundson requested changes to this revision.Apr 16 2020, 4:34 PM

I think part is superfluous, but the rest makes sense.

plugins/platforms/drm/drm_output.cpp
85–88

From what I can tell we never call m_cursorPlane->setCurrent with anything non-null? So this is all a no-op.

The buffers are the m_cursor buffers deleted below, and they're set directly below.

This revision now requires changes to proceed.Apr 16 2020, 4:34 PM
meven updated this revision to Diff 80294.Apr 16 2020, 4:44 PM

Remove unneeded changes

meven marked an inline comment as done.Apr 16 2020, 4:44 PM
davidedmundson accepted this revision.Apr 16 2020, 4:44 PM

You can still have the m_cursorPlane->setOutput(nullptr) line.

This revision is now accepted and ready to land.Apr 16 2020, 4:44 PM
apol added a subscriber: apol.Apr 16 2020, 5:10 PM

Patch looks good

plugins/platforms/drm/drm_output.cpp
175

Shouldn't have the space return;

Stable branch?

meven updated this revision to Diff 80349.Apr 17 2020, 7:28 AM
meven marked an inline comment as done.

Add spaces return, improve tearDown()

meven added inline comments.Apr 17 2020, 7:31 AM
plugins/platforms/drm/drm_output.cpp
92–93

I replace those deletes, as they did not delete the m_cursor array and allowed m_cursor[i] to keep dangling pointers causing down the line crash in QImage rather than a proper segfault where m_cursor[i] was used.

meven marked an inline comment as done.Apr 17 2020, 7:34 AM
zzag added inline comments.Apr 17 2020, 7:37 AM
plugins/platforms/drm/drm_output.cpp
92–93

m_cursor is not a dynamically allocated array. We should not delete[] it.

fvogt requested changes to this revision.Apr 17 2020, 8:26 AM
fvogt added a subscriber: fvogt.
fvogt added inline comments.
plugins/platforms/drm/drm_output.cpp
92–93

To avoid dangling pointers it would have to use

delete m_cursor[0];
m_cursor[0] = nullptr;

delete[] m_cursor is like free(m_cursor). It'll most likely crash.

This revision now requires changes to proceed.Apr 17 2020, 8:26 AM
meven updated this revision to Diff 80353.Apr 17 2020, 8:31 AM
meven marked 2 inline comments as done.

Fix m_cursor deletion

davidedmundson accepted this revision.Apr 17 2020, 4:10 PM
bport added a subscriber: bport.Apr 17 2020, 4:14 PM
bport added inline comments.
plugins/platforms/drm/drm_output.cpp
68

no space between return and ;

175

you still have the space :-)

meven updated this revision to Diff 80407.Apr 17 2020, 4:19 PM

Remove spaces, this time

meven marked 2 inline comments as done.Apr 17 2020, 4:19 PM
meven added inline comments.
plugins/platforms/drm/drm_output.cpp
175

@apol implied they were needed... And the file has both style mixed.

meven marked 2 inline comments as done.Apr 17 2020, 4:19 PM
bport added inline comments.Apr 17 2020, 4:21 PM
plugins/platforms/drm/drm_output.cpp
175

I thought he wanted you to remove it

Shouldn't have the space return;

This revision was not accepted when it landed; it landed in state Needs Review.Apr 17 2020, 4:31 PM
This revision was automatically updated to reflect the committed changes.