Keepassxc: KeepassXC deletes database file after unsafe save on different file system

[jankatins]

Expected Behavior

I want my datafiles save on a google drive for business (Drive File Stream) folder.

Current Behavior

I've my keepass file on a google drive for business virtual drive (Drive File Stream) in my mac (updated to latest version). When I started with keepassxc 2.4 this morning, it told me that it needed to move my browser settings into the database settings (i clicked yes). Afterwards I got an error message that it couldn't save the file ("Writing the database failed, cross device link"). Afterwards my keepass file was gone.

Possible Solution

Steps to Reproduce

1. Add a pre keepass 2.4 (with browser settings) file to a google drive for business folder
2. open it, click yes to the browser migration question
3. observe: the file is gone

Debug Info

KeePassXC - Version 2.4.0
Revision: c51752d

Libraries:

• Qt 5.12.2
• libgcrypt 1.8.4

Operating system: macOS Mojave (10.14)
CPU architecture: x86_64
Kernel: darwin 18.5.0

Enabled extensions:

• Auto-Type
• Browser Integration
• SSH Agent
• KeeShare (only unsigned sharing)
• YubiKey
• TouchID

[droidmonkey]

Did you actually reproduce this or only experienced it once? The migration of settings has nothing to do with file saving. When it was done the database is dirty and if you have autosave after every change enabled, the database will be saved as usual.

Do you have safe file saves disabled?

Since Google Drive saves a version history you can easily restore your database file (look in the trash).

[phoerious]

That looks to me like you do not have safe saves enabled and somehow your database was on a different file system than your temporary savefile.

[jankatins]

I have "Savely save databases files (may be incompatible with dropbox...)" off. Before, in 2.3.x, the backup file was directly next to the active keepass file (I've "backup database file before saving" and "automatically save after every change" on).

I can reproduce it with the re-downloaded file from the drive website (so restoring was possible). tried it three times now.

[droidmonkey]

Does the same thing happen if you just edit an entry and save?

[jankatins]

Yes: I opened the file, clicked no to browser setting migration, added a new entry, named it 'try', clicked ok (Which closed the "Edit entry" dialog) and then got the error message ("Writing the database failed, cross device link") and the file was gone. The funny thing is, it doesn't even say it needs to be saved after this error: the save menu is greyed out and command+s doesn't do anything. Only when I do another change, I get the error again.

[droidmonkey]

Does your database have the standard kdbx extension? And no backup file is being created?

Just need to tease out a couple more details to narrow the scope.

[moobyfr]

I have the same problem when upgraded to 2.4.0, macOS 10.14.3
I don't use google drive (but seafile, something like dropbox)
On migration message (yes or no),clicking on "yes", message "Writing the database failed, cross device link" and the file was gone, no backup file. the file was named MDB.kdbx

I tried with the file stored in the $HOME, same behavior.

[droidmonkey]

I just did this and could not replicate the behavior. The only place in the code where we delete a database file is:

1. When safe saves are disabled and we remove the existing database just prior to copying the temporary new database. https://github.com/keepassxreboot/keepassxc/blob/d7660dad3774ee5192e3ce1728cb4945b1e834dc/src/core/Database.cpp#L243

2. When we are removing the old backup database file. https://github.com/keepassxreboot/keepassxc/blob/d7660dad3774ee5192e3ce1728cb4945b1e834dc/src/core/Database.cpp#L332

This leads me to believe that the movement of the temporary file fails for some reason in between deleting the old DB and move the new one in place. If you have backups enabled, that also means the old DB cannot be copied either (this occurs prior to deletion, obviously).

This can occur if you have run out of disk space, have incorrect permissions, or something really crazy is blocking the copy operation.

[SunTsu]

Same happened to me - I was storing the database file on a VeraCrypt drive. Now it's gone. I have plenty of disk space, I have the correct permissions, it was working with 2.3.x. 2.4 deleted it. There are no backups although "Backup database file before saving" is enabled. And yes, I got the same "cross device link" error as well.

[droidmonkey]

@moobyfr is your $HOME mounted on a different file system? Perhaps an encrypted volume?

The cross-device link error occurs when trying to rename a file from one file system to another.

I think the issue here is that, on Linux only, we copy the temp file to it's final destination before renaming. We should be doing the same on MacOS as well. I do consider this a Qt bug because Qt should smartly copy the file first if trying to rename across file systems.

Responsible code block:

https://github.com/keepassxreboot/keepassxc/blob/d7660dad3774ee5192e3ce1728cb4945b1e834dc/src/core/Database.cpp#L243-L256

For now, I absolutely recommend enabling safe saves to get around this issue.

[droidmonkey]

Yes, the behavior of Qt is at fault here. Their documentation states rename will try to copy the file first if simple rename fails.

https://doc.qt.io/qt-5/qfile.html#rename

If the rename operation fails, Qt will attempt to copy this file's contents to newName, and then remove this file, keeping only newName. If that copy operation fails or this file can't be removed, the destination file newName is removed to restore the old state.

I will file a Qt bug report if one doesn't already exist.

[droidmonkey]

Oh wow, this is unfortunate. It looks like QTemporaryFile::rename(...) has a different, undocumented, implementation than QFile::rename(...).

https://bugreports.qt.io/browse/QTBUG-71782

Ridiculous! I will fix this issue at once.

[SunTsu]

Thanks for fixing it! But could somebody please add a warning to the release notes? If you don't have a recent copy of your database this is a fatal problem and people need to be aware.

[droidmonkey]

I updated release notes and sent a tweet.

[moobyfr]

my $HOME is stored on another disk (classical mount). TMPDIR is set to /var/folders/.... which isn't on the same disk.

[jankatins]

Another thing (apart from #2889) could be that after not being able to safe, it still greys out the "File > save" menu entry, leaving you with no indicator that a (automatic) save was not happening (the error goes away after a few seconds, so if you press ok and switch away from the window, you might miss that). This might be related to the fact that I have 'automatic saving after any changes' on.

[droidmonkey]

That is automatic save, the toolbar button will always be disabled

[jankatins]

I get that, I just think this is not what you get trained for in other apps: usually save is only greyed out, if the latest state is saved to the disk. In this case it is not, but still the "Database -> Save database" entry is greyed out.

[droidmonkey]

This is a good point, and that is certainly a bug

[phoerious]

We should remove the save button if autosave is on. It makes no sense to have it. Only confuses users.

[jankatins]

The problem is more that there is no indicator that your changed data needs a save if you have autosave on, but anything goes wrong during the save. I usually view a greyed out save button as "everything is saved', so I expected that the greyed icon/menu entry is because the save happened, not because the autosave setting is on.

[phoerious]

That's a totally different problem and should not happen at all.

[stysedo]

I updated release notes and sent a tweet.

Might be worth adding the warning info to the downloads page of the main keepassxc.org page at https://keepassxc.org/download/#mac

[droidmonkey]

We'll be releasing the fix this week. Going to let it ride for now.

[beks6]

Hanging in here, just for completeness.
We are using our Database through SMB, everyone is already on KeePassXC 2.4 and when answering the Browser Integration question with Yes on opening the database, KeePassXC deletes the database.

[droidmonkey]

I'd like to stress that if you have safe saves enabled this bug is nullified.

[beks6]

no, safe save is not active, since it says (may be not compatible with dropbox etc.)

[mike-source]

I am seeing the same issue here with Mac OSX & Google Drive... when will the fix be merged into the stable version? When I check for updates in keepassXC it says I have the latest.

Is there any workaround for whilst we wait for a fix to be pushed or am I being stupid? Thanks!

[droidmonkey]

Just enable safe saves in the settings.

[beks6]

ok, is there something we have to pay attention because of "may be incompatible with Dropbox etc."?

[droidmonkey]

Actually no because the macOS distribution is using Qt 5.12 which doesn't have the bug that caused us to make an unsafe save in the first place.

[SunTsu]

Then there might be another issue, because I use Mac and I fell prey to a vanishing database. And this user reports the same problem using MacOS: https://github.com/keepassxreboot/keepassxc/issues/2888#issuecomment-481240035

[droidmonkey]

There is not another issue. The code path that leads to databases vanishing was only activated when save safes were disabled. Just enable save safes and you won't have any issues:

[SunTsu]

Yes, understood, that's what I've been doing the last 14 days. I just wondering if there might be another issue because you've said that the MacOS distribution doesn't have the bug in the first place. Well, if it doesn't have the bug - why did it happen to me? But maybe I just misunderstood what you were saying.

[droidmonkey]

That is not what I meant, sorry. The bug that forced us to create unsafe saves in the first place was in the QSaveFile class. It did not properly hold a lock on the file while it was performing its function which allowed DropBox, Drive, etc to acquire a lock while attempting to sync the file. With Qt 5.11+, this bug in QSaveFile no longer exists (allegedly) so you can use save safes again without causing file locking issues.