Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Random Crashes Upon Database Unlocks on Wayland, Specifically #10415

Closed
ThisNekoGuy opened this issue Mar 13, 2024 · 21 comments · Fixed by #10458
Closed

Random Crashes Upon Database Unlocks on Wayland, Specifically #10415

ThisNekoGuy opened this issue Mar 13, 2024 · 21 comments · Fixed by #10458

Comments

@ThisNekoGuy
Copy link

ThisNekoGuy commented Mar 13, 2024

Overview

When running KeePassXC under Wayland, specifically (for some reason), submitting passwords to unlock a database may randomly crash KeePassXC.

Steps to Reproduce

  1. Run a KDE Plasma Wayland session (Doesn't matter if Plasma 5 or 6; they both suffer from this issue.)
  2. Launch KeePassXC (can either be via terminal or via .desktop file, but DO NOT run with gdb (which keepassxc)
  3. Optionally attach GDB: # gdb --pid=(pidof keepassxc) and run continue once GDB is stalled shortly afterwards
  4. Open KeePassXC from the task tray, if not already open
  5. Attempt to authenticate an existing database and click Unlock
  6. The crash may or may not occur upon this attempt; these crashes are random but usually frequent

Expected Behavior

KeePassXC is expected to not crash upon authentication submission.

Actual Behavior

KeePassXC crashes upon authentication submission.

Context

  • All of the packages on my system are built with debug symbols, so if anything is missing in the GDB output, I have no idea why.
  • Most of the system is built with -O3
  • Compile flags for KeePassXC were:
CFLAGS="-march=znver2 -pipe -O2 -fno-plt -fexceptions -ftrivial-auto-var-init=pattern -Wp,-D_FORTIFY_SOURCE=3 -Wformat -Werror=format-security -Werror=array-bounds -fstack-clash-protection -fstack-protector-strong -fcf-protection -fPIC -ggdb3"
CXXFLAGS="-march=znver2 -pipe -O2 -fno-plt -fexceptions -ftrivial-auto-var-init=pattern -Wp,-D_FORTIFY_SOURCE=3 -Wformat -Werror=format-security -Werror=array-bounds -fstack-clash-protection -fstack-protector-strong -fcf-protection -fPIC -ggdb3"
LDFLAGS="-Wl,-O2,--sort-common,--as-needed,-z,relro,-z,now"
  • System Toolchain: Clang17+LLVM17 with libc++ (Also occurs with Clang/LLVM/libc++ 16)

GDB Log: https://pastebin.com/QZg4xLXL

KeePassXC - Version 2.7.6
Revision: dd21def

Qt 5.15.12
Debugging mode is disabled.

Operating system: Gentoo Linux
CPU architecture: x86_64
Kernel: linux 6.7.1-tkg-pds-gentoo-llvm-zen2 (https://github.com/frogging-family/linux-tkg)

Enabled extensions:
- Auto-Type
- Browser Integration
- SSH Agent
- Secret Service Integration

Cryptographic libraries:
- Botan 3.2.0

Operating System: Gentoo Linux (LLVM17)
Desktop Env: KDE
Windowing System: Wayland

@droidmonkey
Copy link
Member

Please re-upload the log

@ThisNekoGuy
Copy link
Author

ThisNekoGuy commented Mar 13, 2024

I had to upload it to pastebin instead because that upload issue happened on subsequent re-uploads.
Don't know what's going on with Github's file attachments right now.

@droidmonkey
Copy link
Member

droidmonkey commented Mar 13, 2024

I think this is same issue of #10391

Looks like a hang in argon2 processing

@ThisNekoGuy
Copy link
Author

I find it strange then that I wouldn't have this issue in X11; I'd think argon2 wouldn't have anything to do with Wayland.
I started having this issue the moment I stopped using X11 about 2 months ago or so.

@michaelk83
Copy link

Might have to do with different thread handling in Wayland vs X11. The main KeePassXC thread is using QtWaylandClient to manage the other threads, which I'm guessing it wouldn't do under X11.

The crash looks like QtWaylandClient::WlCallback::callback_done() trying to call a bad callback pointer. It says it's at .../qwaylandinputdevice.cpp:185, which doesn't seem related to Argon? But it is in the event loop of runAndWaitForFuture() in readDatabase(). For completeness sake, @ThisNekoGuy which version of Argon2 are you using?

I suspect it might not be the same issue as #10391, since that one seems to depend on the KeePassXC version: ok in 2.7.6, hangs in 2.7.7, and this report is in 2.7.6.

@droidmonkey
Copy link
Member

Argon2 isn't the issue, that is a red herring. The problem most certainly seems to happen in qwayland. Really unsure why though.

@ThisNekoGuy
Copy link
Author

For completeness, the Argon2 source used seems to be this snapshot from 2019.

But, yeah, I can't imagine it's Argon2 because, if it was, this would come up under 2.7.6 for X11 and that's never been the case.

droidmonkey added a commit that referenced this issue Mar 17, 2024
* Fixes #10455
* Fixes #10432
* Fixes #10415

Prevent setting critical key components to nullptr when database data is cleared. This can result in a crash due to race condition between threads.

Added a bunch of asserts to detect this problem and if guards to prevent actual crashes.
@droidmonkey droidmonkey added this to the v2.7.8 milestone Mar 17, 2024
droidmonkey added a commit that referenced this issue Mar 31, 2024
* Fixes #10455
* Fixes #10432
* Fixes #10415

Prevent setting critical key components to nullptr when database data is cleared. This can result in a crash due to race condition between threads.

Added a bunch of asserts to detect this problem and if guards to prevent actual crashes.
droidmonkey added a commit that referenced this issue Mar 31, 2024
* Fixes #10455
* Fixes #10432
* Fixes #10415

Prevent setting critical key components to nullptr when database data is cleared. This can result in a crash due to race condition between threads.

Added a bunch of asserts to detect this problem and if guards to prevent actual crashes.
droidmonkey added a commit that referenced this issue Apr 1, 2024
* Fixes #10455
* Fixes #10432
* Fixes #10415

Prevent setting critical key components to nullptr when database data is cleared. This can result in a crash due to race condition between threads.

Added a bunch of asserts to detect this problem and if guards to prevent actual crashes.
droidmonkey added a commit that referenced this issue Apr 1, 2024
* Fixes #10455
* Fixes #10432
* Fixes #10415

Prevent setting critical key components to nullptr when database data is cleared. This can result in a crash due to race condition between threads.

Added a bunch of asserts to detect this problem and if guards to prevent actual crashes.
droidmonkey added a commit that referenced this issue Apr 1, 2024
* Fixes #10455
* Fixes #10432
* Fixes #10415

Prevent setting critical key components to nullptr when database data is cleared. This can result in a crash due to race condition between threads.

Added a bunch of asserts to detect this problem and if guards to prevent actual crashes.
pull bot pushed a commit to tigerwill90/keepassxc that referenced this issue Apr 13, 2024
* Fixes keepassxreboot#10455
* Fixes keepassxreboot#10432
* Fixes keepassxreboot#10415

Prevent setting critical key components to nullptr when database data is cleared. This can result in a crash due to race condition between threads.

Added a bunch of asserts to detect this problem and if guards to prevent actual crashes.
pull bot pushed a commit to shashinma/keepassxc that referenced this issue Apr 13, 2024
* Fixes keepassxreboot#10455
* Fixes keepassxreboot#10432
* Fixes keepassxreboot#10415

Prevent setting critical key components to nullptr when database data is cleared. This can result in a crash due to race condition between threads.

Added a bunch of asserts to detect this problem and if guards to prevent actual crashes.
@ThisNekoGuy
Copy link
Author

@droidmonkey I recompiled from git to see if the commit fixed it but it still occurs:
keepassxc-git-segfault-crash-backtrace.txt

@ThisNekoGuy
Copy link
Author

ThisNekoGuy commented Apr 14, 2024

I'm not? I thought I was:

>>> Emerging (1 of 1) app-admin/keepassxc-9999::gentoo
>>> Unpacking source...
 * Repository id: keepassxreboot_keepassxc.git
 * To override fetched repository properties, use:
 *   EGIT_OVERRIDE_REPO_KEEPASSXREBOOT_KEEPASSXC
 *   EGIT_OVERRIDE_BRANCH_KEEPASSXREBOOT_KEEPASSXC
 *   EGIT_OVERRIDE_COMMIT_KEEPASSXREBOOT_KEEPASSXC
 *   EGIT_OVERRIDE_COMMIT_DATE_KEEPASSXREBOOT_KEEPASSXC
 * 
 * Fetching https://github.com/keepassxreboot/keepassxc ...
git fetch https://github.com/keepassxreboot/keepassxc +HEAD:refs/git-r3/HEAD
git symbolic-ref refs/git-r3/app-admin/keepassxc/0/__main__ refs/git-r3/HEAD
 * Checking out https://github.com/keepassxreboot/keepassxc to /var/tmp/portage/app-admin/keepassxc-9999/work/keepassxc-9999 ...
git checkout --quiet refs/git-r3/HEAD
GIT update -->
   repository:               https://github.com/keepassxreboot/keepassxc
   at the commit:            6481ecccd79c1bff53887401dc2f6496331007af
>>> Source unpacked in /var/tmp/portage/app-admin/keepassxc-9999/work
>>> Preparing source in /var/tmp/portage/app-admin/keepassxc-9999/work/keepassxc-9999 ...
 * Source directory (CMAKE_USE_DIR): "/var/tmp/portage/app-admin/keepassxc-9999/work/keepassxc-9999"
 * Build directory  (BUILD_DIR):     "/var/tmp/portage/app-admin/keepassxc-9999/work/keepassxc-9999_build"
>>> Source prepared.
>>> Configuring source in /var/tmp/portage/app-admin/keepassxc-9999/work/keepassxc-9999 ...

That's the right commit, isn't it?

@droidmonkey
Copy link
Member

droidmonkey commented Apr 14, 2024

You are crashing in wayland / qwayland code

Thread 1 (Thread 0x75d2770e1080 (LWP 9689) "keepassxc"):
#0  0x0000000000000001 in ?? ()
#1  0x000075d271f5fa35 in QtWaylandClient::WlCallback::callback_done (this=0x606658bb0990, callback_data=<optimized out>) at /var/tmp/portage/dev-qt/qtwayland-5.15.13/work/qtwayland-everywhere-src-5.15.13/src/client/qwaylandinputdevice.cpp:186
#2  0x000075d2727f9cee in ffi_call_unix64 () at /var/tmp/portage/dev-libs/libffi-3.4.4-r4/work/libffi-3.4.4/src/x86/unix64.S:104
#3  0x000075d2727f921b in ffi_call_int (cif=cif@entry=0x7fffb7b50250, fn=fn@entry=0x75d271f88b20 <QtWayland::wl_callback::handle_done(void*, wl_callback*, unsigned int)>, rvalue=rvalue@entry=0x0, avalue=avalue@entry=0x7fffb7b50280, closure=closure@entry=0x0) at /var/tmp/portage/dev-libs/libffi-3.4.4-r4/work/libffi-3.4.4/src/x86/ffi64.c:673
#4  0x000075d2727f8dfb in ffi_call (cif=0x7fffb7b50250, fn=0x75d271f88b20 <QtWayland::wl_callback::handle_done(void*, wl_callback*, unsigned int)>, rvalue=0x0, avalue=0x7fffb7b50280) at /var/tmp/portage/dev-libs/libffi-3.4.4-r4/work/libffi-3.4.4/src/x86/ffi64.c:710
#5  0x000075d2728a82d3 in wl_closure_invoke (closure=0x75d264001c50, flags=1, target=0x606659851530, opcode=0, data=0x606658bb0990) at ../wayland-1.22.0/src/connection.c:1025
#6  0x000075d2728a654d in dispatch_event (display=display@entry=0x606656d3fd40, queue=<optimized out>) at ../wayland-1.22.0/src/wayland-client.c:1631
#7  0x000075d2728a5e7c in dispatch_queue (display=0x606656d3fd40, queue=<optimized out>) at ../wayland-1.22.0/src/wayland-client.c:1777
#8  wl_display_dispatch_queue_pending (display=0x606656d3fd40, queue=0x606656d3fe30) at ../wayland-1.22.0/src/wayland-client.c:2019
#9  0x000075d271f63773 in QtWaylandClient::EventThread::dispatchQueuePending (this=0x606656d74130) at /var/tmp/portage/dev-qt/qtwayland-5.15.13/work/qtwayland-everywhere-src-5.15.13/src/client/qwaylanddisplay.cpp:255
#10 QtWaylandClient::EventThread::readAndDispatchEvents (this=0x606656d74130) at /var/tmp/portage/dev-qt/qtwayland-5.15.13/work/qtwayland-everywhere-src-5.15.13/src/client/qwaylanddisplay.cpp:142
#11 0x000075d276092110 in doActivate<false> (sender=0x606656df66c0, signal_index=4, argv=0x7fffb7b50598) at /var/tmp/portage/dev-qt/qtcore-5.15.13/work/qtbase-everywhere-src-5.15.13/src/corelib/kernel/qobject.cpp:3937
#12 0x000075d2760b3f1b in QEventDispatcherGlib::processEvents (this=0x606656df66c0, flags=...) at /var/tmp/portage/dev-qt/qtcore-5.15.13/work/qtbase-everywhere-src-5.15.13/src/corelib/kernel/qeventdispatcher_glib.cpp:430
#13 0x000075d276056d06 in QEventLoop::processEvents (this=0x7fffb7b506c0, flags=...) at /var/tmp/portage/dev-qt/qtcore-5.15.13/work/qtbase-everywhere-src-5.15.13/src/corelib/kernel/qeventloop.cpp:142
#14 QEventLoop::exec (this=0x7fffb7b506c0, flags=...) at /var/tmp/portage/dev-qt/qtcore-5.15.13/work/qtbase-everywhere-src-5.15.13/src/corelib/kernel/qeventloop.cpp:235
#15 0x0000606656090f12 in AsyncTask::waitForFuture<bool> (future=...) at /var/tmp/portage/app-admin/keepassxc-9999/work/keepassxc-9999/src/core/AsyncTask.h:42

@ThisNekoGuy
Copy link
Author

ThisNekoGuy commented Apr 14, 2024

So... (Isn't that what we were originally discussing further up?)

@droidmonkey
Copy link
Member

droidmonkey commented Apr 14, 2024

Sorry I thought this was a different issue. This is definitely a different crash, but I can't see how this is a keepassxc crash though.

@droidmonkey droidmonkey reopened this Apr 14, 2024
@ThisNekoGuy
Copy link
Author

Do you think it's an upstream qtwayland issue somehow?

@droidmonkey
Copy link
Member

It is strange that it happens on unlock only, we do disable gui elements and such. I would like to think it's an upstream issue. There doesn't seem to be any obvious trigger from our code.

@droidmonkey
Copy link
Member

This could be related to putting the cursor icon into the "busy" state or vice versa.

Here is the relevant qt code:

https://code.qt.io/cgit/qt/qtwayland.git/tree/src/client/qwaylandinputdevice.cpp?h=5.15#n183

I suspect m_fn is nullptr in this crash.

@ThisNekoGuy
Copy link
Author

That might actually make sense, because I noticed whenever I attached GDB to the process and triggered the crash that the cursor is always in the busy state every time when I hover over the window

@ThisNekoGuy
Copy link
Author

ThisNekoGuy commented Apr 14, 2024

I found that having no database currently connected and running through GDB to connect to an existing database causes the same issue with the process dying when the mouse loading spinner comes up too
Screenshot_20240413_204531

@droidmonkey
Copy link
Member

This seems to be a qt crash

droidmonkey added a commit that referenced this issue Apr 28, 2024
* Fixes #10455
* Fixes #10432
* Fixes #10415

Prevent setting critical key components to nullptr when database data is cleared. This can result in a crash due to race condition between threads.

Added a bunch of asserts to detect this problem and if guards to prevent actual crashes.
droidmonkey added a commit that referenced this issue Apr 29, 2024
* Fixes #10455
* Fixes #10432
* Fixes #10415

Prevent setting critical key components to nullptr when database data is cleared. This can result in a crash due to race condition between threads.

Added a bunch of asserts to detect this problem and if guards to prevent actual crashes.
@droidmonkey
Copy link
Member

Closing this as it isn't seemingly related to KeePassXC code.

@ThisNekoGuy
Copy link
Author

@droidmonkey Do you think we can get it fixed upstream though, since you seem to know what the real issue is?

@droidmonkey
Copy link
Member

I have no idea what the actual issue is, the crash logs don't pinpoint it. Just hypothesis.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants