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 broken safe saves across file systems #2889

Merged
merged 3 commits into from Apr 7, 2019

Conversation

Projects
None yet
3 participants
@droidmonkey
Copy link
Member

commented Mar 26, 2019

Type of change

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

Description and Context

  • Fix #2888

  • Qt has an undocumented rename implementation for QTemporaryFile that does not fallback to the copy implementation. Forcing the use of QFile::rename(...) allows for this fallback and protects against cross-device link errors.

  • Improve behaviors when the database fails to save properly.

    • Mark database dirty if saving fails
    • Restore database file from backup if unsafe save fails between deleting database file and copying temporary file into place
    • Improve error message display for opening and saving database files
    • Do not automatically retry saving after failure. This prevents deletion of the backup database file and improves user awareness of issues.

This was tested against databases with various extensions and forced failure modes.

Checklist:

  • I have read the CONTRIBUTING document. [REQUIRED]
  • My code follows the code style of this project. [REQUIRED]
  • All new and existing tests passed. [REQUIRED]
  • I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]
  • My change requires a change to the documentation, and I have updated it accordingly.
  • I have added tests to cover my changes.
@droidmonkey

This comment has been minimized.

Copy link
Member Author

commented Mar 26, 2019

Tested using VeraCrypt container.

2.4.0 -> database is deleted and cross-link error thrown
This PR -> database is saved correctly and backup file created

@droidmonkey droidmonkey force-pushed the hotfix/unsafe-save-rename branch from a19f894 to 22ce07c Mar 28, 2019

droidmonkey added some commits Mar 26, 2019

Fix broken safe saves across file systems
* Fix #2888
* Qt has an undocumented rename implementation for QTemporaryFile that does not fallback to the copy implementation. Forcing the use of QFile::rename(...) allows for this fallback and protects against cross-device link errors.
Correct behaviors when saving database fails
* Mark database dirty if saving fails
* Restore database file from backup if unsafe save fails between deleting database file and copying temporary file into place
* Improve error message display for opening and saving database files
* Do not automatically retry saving after failure. This prevents deletion of the backup database file and improves user awareness of issues.

@droidmonkey droidmonkey force-pushed the hotfix/unsafe-save-rename branch from 22ce07c to a95e9f2 Apr 3, 2019

@droidmonkey

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2019

Added a lot better handling of failed saving in the latest commit

Show resolved Hide resolved src/core/Database.cpp Outdated
Show resolved Hide resolved src/gui/DatabaseWidget.cpp
Show resolved Hide resolved src/core/Database.cpp Outdated
Additional layer of protection for unsafe saves
* Attempt to restore database, if that fails retain the temporary file and tell the user where it is located

@droidmonkey droidmonkey force-pushed the hotfix/unsafe-save-rename branch from 36fb7c9 to ab79608 Apr 7, 2019

@droidmonkey droidmonkey merged commit 791b796 into release/2.4.1 Apr 7, 2019

4 checks passed

CodeFactor No issues found.
Details
MacOS (KeepassXC) TeamCity build finished
Details
Ubuntu Linux (KeepassXC) TeamCity build finished
Details
Windows 10 (KeepassXC) TeamCity build finished
Details

@droidmonkey droidmonkey deleted the hotfix/unsafe-save-rename branch Apr 7, 2019

droidmonkey added a commit that referenced this pull request Apr 12, 2019

Release 2.4.1
- Fix database deletion when using unsafe saves to a different file system [#2889]
- Fix opening databases with legacy key files that contain '/' [#2872]
- Fix opening database files from the command line [#2919]
- Fix crash when editing master key [#2836]
- Fix multiple issues with apply button behavior [#2947]
- Fix issues on application startup (tab order, --pw-stdin, etc.) [#2830]
- Fix building without WITH_XC_KEESHARE
- Fix reference entry coloring on macOS dark mode [#2984]
- Hide window when performing entry auto-type on macOS [#2969]
- Improve UX of update checker; reduce checks to every 7 days [#2968]
- KeeShare improvements [#2946, #2978, #2824]
- Re-enable Ctrl+C to copy password from search box [#2947]
- Add KeePassXC-Browser integration for Brave browser [#2933]
- SSH Agent: Re-Add keys on database unlock [#2982]
- SSH Agent: Only remove keys on app exit if they are removed on lock [#2985]
- CLI: Add --no-password option [#2708]
- CLI: Improve database extraction to XML [#2698]
- CLI: Don't call mandb on build [#2774]
- CLI: Add debug info [#2714]
- Improve support for Snap theming [#2832]
- Add support for building on Haiku OS [#2859]
- Ctrl+PgDn now goes to the next tab and Ctrl+PgUp to the previous
- Fix compiling on GCC 5 / Xenial [#2990]
- Add .gitrev output to tarball for third-party builds [#2970]
- Add WITH_XC_UPDATECHECK compile flag to toggle the update checker [#2968]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.