Skip to content

Conversation

@droidmonkey
Copy link
Member

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.

Testing strategy

Tested on Windows

Type of change

  • ✅ Bug fix (non-breaking change that fixes an issue)

@carun
Copy link

carun commented Mar 25, 2024

I can help test this if there's a build available as I'm able to reproduce the issue consistently.

@old-pigeon
Copy link

Also able to reproduce 100%, let me know when I can test.

@dimitris-personal
Copy link

Tried this patch on top of 2.7.7 and still got the crash on session lock, steps/environment same as #10432.

@droidmonkey
Copy link
Member Author

You got the same stack trace? That wouldn't be possible.

@dimitris-personal
Copy link

dimitris-personal commented Apr 1, 2024

Hmm, the stack trace looks the same?

Thread 1 "keepassxc" received signal SIGSEGV, Segmentation fault.
0x0000555971c278cd in Database::transformedDatabaseKey (this=<optimized out>) at /usr/include/qt5/QtCore/qscopedpointer.h:116
warning: 116	/usr/include/qt5/QtCore/qscopedpointer.h: No such file or directory
(gdb) bt
#0  0x0000555971c278cd in Database::transformedDatabaseKey (this=<optimized out>) at /usr/include/qt5/QtCore/qscopedpointer.h:116
#1  0x0000555971c61dcf in Kdbx4Reader::readDatabaseImpl (this=0x55597342a070, device=0x7ffcf2b000c0, headerData=..., key=..., db=<optimized out>)
    at /usr/src/debug/keepassxc-2.7.7-2.fc39.x86_64/src/format/Kdbx4Reader.cpp:61
#2  0x0000555971c5e6df in KdbxReader::readDatabase (db=0x555973fca910, key=..., device=0x7ffcf2b000c0, this=<optimized out>) at /usr/src/debug/keepassxc-2.7.7-2.fc39.x86_64/src/format/KdbxReader.cpp:95
#3  KeePass2Reader::readDatabase (this=<optimized out>, device=0x7ffcf2b000c0, key=..., db=0x555973fca910) at /usr/src/debug/keepassxc-2.7.7-2.fc39.x86_64/src/format/KeePass2Reader.cpp:97
#4  0x0000555971c27bfb in Database::open (this=0x555973fca910, filePath=..., key=..., error=0x7ffcf2b00188) at /usr/src/debug/keepassxc-2.7.7-2.fc39.x86_64/src/core/Database.cpp:149
#5  0x0000555971c8d301 in DatabaseOpenWidget::clearForms (this=this@entry=0x555973a89040) at /usr/src/debug/keepassxc-2.7.7-2.fc39.x86_64/src/gui/DatabaseOpenWidget.cpp:275
#6  0x0000555971c8e7f3 in DatabaseOpenWidget::load (this=0x555973a89040, filename=...) at /usr/src/debug/keepassxc-2.7.7-2.fc39.x86_64/src/gui/DatabaseOpenWidget.cpp:241
#7  0x0000555971ca025f in DatabaseWidget::switchToOpenDatabase (this=0x555972fb4bc0, filePath=...) at /usr/src/debug/keepassxc-2.7.7-2.fc39.x86_64/src/gui/DatabaseWidget.cpp:1355
#8  0x0000555971ca6d8d in DatabaseWidget::lock (this=0x555972fb4bc0) at /usr/src/debug/keepassxc-2.7.7-2.fc39.x86_64/src/gui/DatabaseWidget.cpp:1790
#9  0x0000555971c900ee in DatabaseTabWidget::lockDatabases (this=0x5559729d2c90) at /usr/src/debug/keepassxc-2.7.7-2.fc39.x86_64/src/gui/DatabaseTabWidget.cpp:666
#10 0x00007f4789ce92d8 in void doActivate<false>(QObject*, int, void**) () from /lib64/libQt5Core.so.5
#11 0x00007f4789ce92d8 in void doActivate<false>(QObject*, int, void**) () from /lib64/libQt5Core.so.5
#12 0x0000555971c1bd20 in ScreenLockListenerDBus::qt_metacall (this=0x555972c21fc0, _c=QMetaObject::InvokeMetaMethod, _id=3, _a=0x7ffcf2b00620)
    at /usr/src/debug/keepassxc-2.7.7-2.fc39.x86_64/redhat-linux-build/src/keepassx_core_autogen/FUIKO5VHUE/moc_ScreenLockListenerDBus.cpp:141
#13 0x00007f478b65438b in QDBusConnectionPrivate::deliverCall(QObject*, int, QDBusMessage const&, QVector<int> const&, int) () from /lib64/libQt5DBus.so.5
#14 0x00007f4789cdf9fb in QObject::event(QEvent*) () from /lib64/libQt5Core.so.5
#15 0x00007f478afaeb95 in QApplicationPrivate::notify_helper(QObject*, QEvent*) () from /lib64/libQt5Widgets.so.5
#16 0x00007f4789cb4e78 in QCoreApplication::notifyInternal2(QObject*, QEvent*) () from /lib64/libQt5Core.so.5
#17 0x00007f4789cb8325 in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) () from /lib64/libQt5Core.so.5
#18 0x00007f4789d078cf in postEventSourceDispatch(_GSource*, int (*)(void*), void*) () from /lib64/libQt5Core.so.5
#19 0x00007f4788511e5c in g_main_context_dispatch_unlocked.lto_priv () from /lib64/libglib-2.0.so.0
#20 0x00007f478856cf18 in g_main_context_iterate_unlocked.isra () from /lib64/libglib-2.0.so.0
#21 0x00007f478850fad3 in g_main_context_iteration () from /lib64/libglib-2.0.so.0
#22 0x00007f4789d073b9 in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () from /lib64/libQt5Core.so.5
#23 0x00007f4789cb383b in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () from /lib64/libQt5Core.so.5
#24 0x00007f4789cbbacb in QCoreApplication::exec() () from /lib64/libQt5Core.so.5
#25 0x0000555971bad3a5 in main (argc=<optimized out>, argv=<optimized out>) at /usr/src/debug/keepassxc-2.7.7-2.fc39.x86_64/src/main.cpp:215

I built on Fedora using fedpkg mockbuild after adding this PR's patch to the spec file...

@droidmonkey
Copy link
Member Author

You aren't running the code from this PR then. I removed the line item that is #4 on the trace.

@dimitris-personal
Copy link

Yeah, the patch is not being applied in the build for some reason... I rebased it to 2.7.7 but still get that Database::open() frame.

@droidmonkey droidmonkey force-pushed the fix/database-lock-crash branch from b4ea4ed to df812dd Compare April 1, 2024 23:31
* 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 force-pushed the fix/database-lock-crash branch from df812dd to 406d5b2 Compare April 1, 2024 23:50
@codecov
Copy link

codecov bot commented Apr 2, 2024

Codecov Report

Attention: Patch coverage is 87.09677% with 4 lines in your changes missing coverage. Please review.

Project coverage is 63.72%. Comparing base (194409a) to head (406d5b2).
Report is 140 commits behind head on develop.

Files with missing lines Patch % Lines
src/core/Database.cpp 85.71% 2 Missing ⚠️
...ui/dbsettings/DatabaseSettingsWidgetEncryption.cpp 66.67% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #10458      +/-   ##
===========================================
+ Coverage    63.70%   63.72%   +0.03%     
===========================================
  Files          362      362              
  Lines        44047    44061      +14     
===========================================
+ Hits         28056    28077      +21     
+ Misses       15991    15984       -7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@dimitris-personal
Copy link

Yup, circled back to this and I was indeed missing the patch application in the spec file. Built 2.7.7 plus this PR's patch and can no longer reproduce #10432.

I did have to disable tests in order for the rpm to build, probably what's also behind the CI failures here (no teamcity access on my end obviously).

@droidmonkey
Copy link
Member Author

droidmonkey commented Apr 2, 2024

Yah I just fixed the test problems, needed to revert some code changes I made that went a little too far.

@droidmonkey droidmonkey merged commit 6481ecc into develop Apr 13, 2024
@droidmonkey droidmonkey deleted the fix/database-lock-crash branch April 13, 2024 11:54
@benediktwerner
Copy link

Any chance we could get a release with this fix? The constant crashes are kinda annoying, so I was hoping we'd get an update soon.

@droidmonkey
Copy link
Member Author

Yes 2.7.8, but use https://snapshot.keepassxc.org for now if you like

@droidmonkey droidmonkey added the pr: backported Pull request backported to previous release label Apr 28, 2024
libf-de pushed a commit to libf-de/keepassxc-secretservice-dbus that referenced this pull request May 6, 2024
Release 2.7.8

Changes
- Add hotkey for showing search help [keepassxreboot#10591]
- Add hotkey for group switching (Ctrl+Shift+PgUp/PgDown) [keepassxreboot#10625]
- Add per-database auto-save delay setting [keepassxreboot#9100]
- Add setting to hide menubar [keepassxreboot#10341]
- Improve Bitwarden 1PUX import and support organization collections [keepassxreboot#10499]
- Show advanced settings checkbox only for settings that have them [keepassxreboot#6513]
- Remove obsolete setting for requiring repeated password entry [keepassxreboot#9722]
- Passkeys: Allow registering Passkeys to existing entries [keepassxreboot#10408]
- Passkeys: Show warning about data being unencrypted before Passkey export [keepassxreboot#10411]
- Passkeys: Support NFC and USB transports [keepassxreboot#10402]
- Passkeys: Pass extension JSON data to browser [keepassxreboot#10615]
- SSH Agent: Do not use entries from recycle bin [keepassxreboot#10518]
- Linux: Change hotkey sequence used for {CLEARFIELD} Auto-Type [keepassxreboot#10008]
- Windows: Improve DACL memory access protection [keepassxreboot#10618]

Fixes
- Fix crash when deleting history items [keepassxreboot#10451]
- Fix crash on screen lock or computer sleep [keepassxreboot#10458]
- Fix search field not being focused after unlock [keepassxreboot#10459]
- Fix loss of window focus when Auto-Type needs to unlock a database [keepassxreboot#10555]
- Fix inconsistent TOTP visibility on unlock [keepassxreboot#10009]
- Fix CSV import skipping over single-name groups [keepassxreboot#10575]
- Fix key file folder being remembered even if disabled in settings [keepassxreboot#10636]
- Fix issues with entry editing and database locking [keepassxreboot#10667]
- Fix key file text when provided on command line [keepassxreboot#10642]
- Fix issues with hardware key auto detection [keepassxreboot#10663]
- Do not override monospace font size [keepassxreboot#10282]
- Perform group sort only when group view is in focus [keepassxreboot#10202]
- Do not show decimals for attachment sizes in Bytes [keepassxreboot#10595]
- Prevent merging of global custom data when merging databases [keepassxreboot#10452]
- Fix minor translation issues [keepassxreboot#10635]
- Passkeys: Fix StrongBox incompatibility [keepassxreboot#10420]
- Passkeys: Set RP ID to effective domain if unset instead of returning an error [keepassxreboot#10384]
- Passkeys: Various UI fixes and improvements [keepassxreboot#10427, keepassxreboot#10608, keepassxreboot#10609]
- AppImage: Fix URL opening [keepassxreboot#10624]
- Flatpak: Fix application autostart [keepassxreboot#10563]
- Linux/macOS: Fix button sizes on modal alert popups [keepassxreboot#10500]
- Linux: Fix clipboard clear on Wayland [keepassxreboot#10500]
- Windows: Preserve file-hidden attribute [keepassxreboot#10343]

# -----BEGIN PGP SIGNATURE-----
#
# iQIzBAABCAAdFiEE6A9nU7OzJX8m9B8ILPQdKqhDj5kFAmY4A30ACgkQLPQdKqhD
# j5npgBAApBCGfhdugBE3X9iCkGQ69LKKWizgp44AzmezxU2ee7KEoZgSmZpOCPyO
# bg9EIgwac+3yCh4i4hJrTvnwIemrUKNsNLE18Kn/Uw3HJBCtsb40CeIFcZktOegu
# RQ5G7jhBtnAopnTKQhdwcwJ0Yq6ZSTSiSuo+miDAN22DjnWVd7BLMOioSBPgxFUT
# td+2MAPeydLoMdFRmkuBaDSStLWThdCz6DrWcBYQSK2b6Mu+3mzmtE24zNM1jCKu
# Tl0t6fRkOhqWSRyWBSMzIH3uMuV95yQNudjDMnuOVWVE9Ai+A1RPFHtf8Zj1ydh9
# n9JGPDyloWRcYQdDBgbn6lFHWnwSaYVCRpRPPmjpmXVwt5/AdtB8wN+6uGbcYTzw
# u9l0YYWx84W0kNPkJ0ZejF33qioQ7FaZruJv2ej++NtO0FJP48UVyrQ4EMG6V+17
# AcQ0aoSWWTb5AYhJXLjImDG7DNY1mbgW6deJLKVS7pkoRke1uSLGqYTUAJCFaXnq
# d9uZt4HRUUMeq6x8dvFNvIcZhsfRUaO/iXjp81nl8hlWIeTYNTj22eww3yapFs+S
# cdmdCmfGZAx5FWCXaszXwD3gLF8Bg6S63l9TvbjEHGR2riYKOO1IbFz8JXXjWpdN
# l4SIcWJfdO2mNz0MWfzNtmMYNu9LBfU2Hod5JHJQYiQh3dh4EG4=
# =MrBi
# -----END PGP SIGNATURE-----
# gpg: directory '/home/runner/.gnupg' created
# gpg: keybox '/home/runner/.gnupg/pubring.kbx' created
# gpg: Signature made Sun May  5 22:09:01 2024 UTC
# gpg:                using RSA key E80F6753B3B3257F26F41F082CF41D2AA8438F99
# gpg: Can't check signature: No public key
@phoerious phoerious added pr: bugfix Pull request fixes a bug and removed bug labels Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

crash 💥 high priority 🚨 pr: backported Pull request backported to previous release pr: bugfix Pull request fixes a bug

Projects

None yet

7 participants