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

Fix crash on screen lock or computer sleep #10458

Merged
merged 1 commit into from
Apr 13, 2024

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.

* 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.
Copy link

codecov bot commented Apr 2, 2024

Codecov Report

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

Project coverage is 63.72%. Comparing base (194409a) to head (406d5b2).

Files 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.

@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
11 checks passed
@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

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants