Bug 486178 - VNC plugin deadlocks on connection
Summary: VNC plugin deadlocks on connection
Status: RESOLVED FIXED
Alias: None
Product: krdc
Classification: Applications
Component: VNC (other bugs)
Version First Reported In: 24.02.2
Platform: Other Linux
: NOR grave
Target Milestone: ---
Assignee: Urs Wolfer
URL:
Keywords:
: 485215 (view as bug list)
Depends on:
Blocks:
 
Reported: 2024-04-26 22:55 UTC by Thiago Macieira
Modified: 2024-06-08 18:45 UTC (History)
3 users (show)

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Thiago Macieira 2024-04-26 22:55:49 UTC
SUMMARY
VNC plugin deadlocks as soon as you try to connect


STEPS TO REPRODUCE
1. Launch KRDC
2. Attempt to connect to a VNC host
3. Close KRDC

OBSERVED RESULT
KRDC freezes and must be killed

EXPECTED RESULT
KRDC closes. Preferably, the connection does establish.

SOFTWARE/OS VERSIONS
KDE Plasma Version: 6.0.3
KDE Frameworks Version: 6.1.0
Qt Version: 6.7.0

ADDITIONAL INFORMATION
Upon trying to connect, KRDC is still usable, but the debugger shows one thread to be already frozen:
Thread 30 (Thread 0x7fffb4a006c0 (LWP 497638) "VncClientThread"):
#0  0x00007ffff5711bcd in syscall () at /lib64/libc.so.6
#1  0x00007ffff60dcca5 in QBasicMutex::lockInternal() () at /lib64/glibc-hwcaps/x86-64-v4/libQt6Core.so.6.7.0
#2  0x00007fffe341886c in QBasicMutex::lock() (this=0x555555b37208) at /usr/include/qt6/QtCore/qmutex.h:41
#3  QMutexLocker<QMutex>::relock() (this=<synthetic pointer>) at /usr/include/qt6/QtCore/qmutex.h:257
#4  VncClientThread::run() (this=0x555555b37180) at /usr/src/debug/krdc-24.02.2/vnc/vncclientthread.cpp:521
#5  0x00007ffff60dc1b8 in  () at /lib64/glibc-hwcaps/x86-64-v4/libQt6Core.so.6.7.0
#6  0x00007ffff5692bb2 in start_thread () at /lib64/libc.so.6
#7  0x00007ffff571400c in clone3 () at /lib64/libc.so.6

    QMutexLocker locker(&mutex);

    while (!m_stopped) { // try to connect as long as the server allows
        locker.relock();
        m_passwordError = false;

This will NEVER EVER work because the mutex is locked by that locker and nothing can unlock it. This is a difference in behaviour between Qt 5 and Qt 6.
Qt 5: https://codebrowser.dev/qt5/qtbase/src/corelib/thread/qmutex.h.html#_ZN12QMutexLocker6relockEv
Qt 6: https://codebrowser.dev/qt6/qtbase/src/corelib/thread/qmutex.h.html#_ZN12QMutexLocker6relockEv

The change happened in Qt 6.4, commit 1b1456975347b044c11169458b53c9f6083dbc59
Comment 1 Thiago Macieira 2024-04-27 16:53:38 UTC
I'd say "vncplugin doesn't work at all, ever, under any conditions, if you try to use it" is sufficient for "critical", but ok for "grave"

BTW, I tried to fix that function myself. I can apply a workaround to just restore the previous behaviour, but that's papering over the sun with a sieve. I really don't understand what's going on and what is being locked. Is it trying to protect m_stopped? Why not use an atomic instead? Or, better, QThread::isInterruptionRequested()? But on line 549, it accesses m_stopped without a mutex, so I don't know if it needed protecting in the first place.

Ditto for m_passwordError: why not an atomic? Or why not use a local variable and only lock to save the state when exiting the function? 

This code should be rewritten with closer locks, not that one QMutexLocker at the top that is used incorrectly.
Comment 2 Bug Janitor Service 2024-04-30 18:42:07 UTC
A possibly relevant merge request was started @ https://invent.kde.org/network/krdc/-/merge_requests/99
Comment 3 Nicolas Fella 2024-04-30 22:46:52 UTC
Git commit 48fbcdffc4ff22a11b56d8809739e0f096d190e6 by Nicolas Fella, on behalf of Thiago Macieira.
Committed on 30/04/2024 at 22:45.
Pushed by nicolasfella into branch 'release/24.02'.

VNC: replace m_stopped with QThread's interruptionRequested

It's an out-of-line function call, but it doesn't require a mutex for
most uses.

The uses in the callbacks are replaced with isRunning(), because
interruptionRequested resets back to false as soon as run() exits.
(cherry picked from commit 33f89ef0bb4f5186dad7ceecad599a0e834e7f88)

M  +14   -24   vnc/vncclientthread.cpp
M  +0    -1    vnc/vncclientthread.h

https://invent.kde.org/network/krdc/-/commit/48fbcdffc4ff22a11b56d8809739e0f096d190e6
Comment 4 Stefan Becker 2024-05-01 09:24:27 UTC
*** Bug 485215 has been marked as a duplicate of this bug. ***
Comment 5 Stefan Becker 2024-05-01 10:47:54 UTC
I can confirm that after applying the patch VNC connections work again with krdc.
Comment 6 Nicolas Fella 2024-06-08 18:45:40 UTC
Git commit e6a169e40b80b4fcd5e42dbba17d82f3f7bf4feb by Nicolas Fella, on behalf of Thiago Macieira.
Committed on 08/06/2024 at 18:42.
Pushed by nicolasfella into branch 'release/24.05'.

VNC: replace m_stopped with QThread's interruptionRequested

It's an out-of-line function call, but it doesn't require a mutex for
most uses.

The uses in the callbacks are replaced with isRunning(), because
interruptionRequested resets back to false as soon as run() exits.
(cherry picked from commit 33f89ef0bb4f5186dad7ceecad599a0e834e7f88)

M  +14   -24   vnc/vncclientthread.cpp
M  +0    -1    vnc/vncclientthread.h

https://invent.kde.org/network/krdc/-/commit/e6a169e40b80b4fcd5e42dbba17d82f3f7bf4feb