TOCTOU race in database file swap (acknowledged as "racy as hell")

[grisutheguru]

Summary

The database file swap operation in commitDbSwap() performs a non-atomic close() -> remove() -> rename() sequence that can lose the database on crash.

Details

src/persistence/db/rawdatabaseimpl.cpp:536-550:

// This is racy as hell, but nobody will race with us since we hold the profile lock
close();
QFile::remove(path);                    // original DB deleted
QFile::rename(path + ".tmp", path);     // if crash here, DB is gone

If the application crashes between QFile::remove(path) and QFile::rename(path + ".tmp", path):

• The original database is deleted
• The .tmp file is still named .tmp
• On next startup, the recovery logic at lines 89-93 checks for .tmp files, providing partial mitigation

Additionally, the return values of QFile::remove() and QFile::rename() are not checked.

Suggested Fix

1. Check return values and log errors:

if (!QFile::remove(path)) {
    qWarning() << "Failed to remove old database:" << path;
}
if (!QFile::rename(path + ".tmp", path)) {
    qCritical() << "Failed to rename temp database! Recovery needed from:" << path + ".tmp";
}

2. On POSIX, use rename() which is atomic on the same filesystem -- skip the remove() step entirely since rename replaces the target:

close();
#ifdef Q_OS_UNIX
// rename() is atomic on POSIX when src and dst are on the same filesystem
if (::rename((path + ".tmp").toUtf8().constData(), path.toUtf8().constData()) != 0) {
    qCritical() << "Atomic rename failed:" << strerror(errno);
}
#else
QFile::remove(path);
QFile::rename(path + ".tmp", path);
#endif

3. On Windows, consider using ReplaceFileW() for atomic replacement.

Impact

Database loss on crash during encryption/decryption operations. The existing .tmp recovery logic mitigates but requires user awareness.