Fix crashes on database save

* Add saving mutex to database class to prevent re-entrant saving
* Prevent saving multiple times to the same file if the database is not marked as modified
* Prevent locking the database while saving. This also prevents closing the application and database tab while saving.
* FileWatcher: only perform async checksum calculations when triggered by timer (prevents random GUI freezes)
* Re-attempt database lock when requested during save operation
* Prevent database tabs from closing before all databases are locked on quit

src/cli/Create.cpp

@@ -121,6 +121,7 @@ int Create::execute(const QStringList& arguments)
 
     QSharedPointer<Database> db(new Database);
     db->setKey(key);
+    db->setInitialized(true);
 
     if (decryptionTime != 0) {
         auto kdf = db->kdf();

src/cli/Import.cpp

@@ -84,6 +84,7 @@ int Import::execute(const QStringList& arguments)
     Database db;
     db.setKdf(KeePass2::uuidToKdf(KeePass2::KDF_ARGON2));
     db.setKey(key);
+    db.setInitialized(true);
 
     if (!db.import(xmlExportPath, &errorMessage)) {
         errorTextStream << QObject::tr("Unable to import XML database: %1").arg(errorMessage) << endl;

src/core/Database.cpp

@@ -18,6 +18,7 @@
 
 #include "Database.h"
 
+#include "core/AsyncTask.h"
 #include "core/Clock.h"
 #include "core/FileWatcher.h"
 #include "core/Group.h"
@@ -159,6 +160,15 @@ bool Database::open(const QString& filePath, QSharedPointer<const CompositeKey>
     return true;
 }
 
+bool Database::isSaving()
+{
+    bool locked = m_saveMutex.tryLock();
+    if (locked) {
+        m_saveMutex.unlock();
+    }
+    return !locked;
+}
+
 /**
  * Save the database to the current file path. It is an error to call this function
  * if no file path has been defined.
@@ -201,6 +211,25 @@ bool Database::save(QString* error, bool atomic, bool backup)
  */
 bool Database::saveAs(const QString& filePath, QString* error, bool atomic, bool backup)
 {
+    // Disallow overlapping save operations
+    if (isSaving()) {
+        if (error) {
+            *error = tr("Database save is already in progress.");
+        }
+        return false;
+    }
+
+    // Never save an uninitialized database
+    if (!m_initialized) {
+        if (error) {
+            *error = tr("Could not save, database has not been initialized!");
+        }
+        return false;
+    }
+
+    // Prevent destructive operations while saving
+    QMutexLocker locker(&m_saveMutex);
+
     if (filePath == m_data.filePath) {
         // Disallow saving to the same file if read-only
         if (m_data.isReadOnly) {
@@ -226,7 +255,7 @@ bool Database::saveAs(const QString& filePath, QString* error, bool atomic, bool
     m_fileWatcher->stop();
 
     auto& canonicalFilePath = QFileInfo::exists(filePath) ? QFileInfo(filePath).canonicalFilePath() : filePath;
-    bool ok = performSave(canonicalFilePath, error, atomic, backup);
+    bool ok = AsyncTask::runAndWaitForFuture([&] { return performSave(canonicalFilePath, error, atomic, backup); });
     if (ok) {
         markAsClean();
         setFilePath(filePath);
@@ -389,6 +418,9 @@ bool Database::import(const QString& xmlExportPath, QString* error)
 
 void Database::releaseData()
 {
+    // Prevent data release while saving
+    QMutexLocker locker(&m_saveMutex);
+
     s_uuidMap.remove(m_uuid);
     m_uuid = QUuid();
 
@@ -397,22 +429,20 @@ void Database::releaseData()
     }
 
     m_data.clear();
+    m_metadata->clear();
 
     if (m_rootGroup && m_rootGroup->parent() == this) {
         delete m_rootGroup;
     }
-    if (m_metadata) {
-        delete m_metadata;
-    }
-    if (m_fileWatcher) {
-        delete m_fileWatcher;
-    }
+
+    m_fileWatcher->stop();
 
     m_deletedObjects.clear();
     m_commonUsernames.clear();
 
     m_initialized = false;
     m_modified = false;
+    m_modifiedTimer.stop();
 }
 
 /**

src/core/Database.h

@@ -21,6 +21,7 @@
 
 #include <QDateTime>
 #include <QHash>
+#include <QMutex>
 #include <QPointer>
 #include <QScopedPointer>
 #include <QTimer>
@@ -85,6 +86,7 @@ public:
     void setEmitModified(bool value);
     bool isReadOnly() const;
     void setReadOnly(bool readOnly);
+    bool isSaving();
 
     QUuid uuid() const;
     QString filePath() const;
@@ -208,6 +210,7 @@ private:
     QPointer<Group> m_rootGroup;
     QList<DeletedObject> m_deletedObjects;
     QTimer m_modifiedTimer;
+    QMutex m_saveMutex;
     QPointer<FileWatcher> m_fileWatcher;
     bool m_initialized = false;
     bool m_modified = false;

src/core/FileWatcher.cpp

@@ -43,6 +43,11 @@ FileWatcher::FileWatcher(QObject* parent)
     m_fileIgnoreDelayTimer.setSingleShot(true);
 }
 
+FileWatcher::~FileWatcher()
+{
+    stop();
+}
+
 void FileWatcher::start(const QString& filePath, int checksumIntervalSeconds, int checksumSizeKibibytes)
 {
     stop();
@@ -120,8 +125,7 @@ void FileWatcher::checkFileChanged()
     // Prevent reentrance
     m_ignoreFileChange = true;
 
-    // Only trigger the change notice if there is a checksum mismatch
-    auto checksum = calculateChecksum();
+    auto checksum = AsyncTask::runAndWaitForFuture([this]() -> QByteArray { return calculateChecksum(); });
     if (checksum != m_fileChecksum) {
         m_fileChecksum = checksum;
         m_fileChangeDelayTimer.start(0);
@@ -132,21 +136,19 @@ void FileWatcher::checkFileChanged()
 
 QByteArray FileWatcher::calculateChecksum()
 {
-    return AsyncTask::runAndWaitForFuture([this]() -> QByteArray {
-        QFile file(m_filePath);
-        if (file.open(QFile::ReadOnly)) {
-            QCryptographicHash hash(QCryptographicHash::Sha256);
-            if (m_fileChecksumSizeBytes > 0) {
-                hash.addData(file.read(m_fileChecksumSizeBytes));
-            } else {
-                hash.addData(&file);
-            }
-            return hash.result();
+    QFile file(m_filePath);
+    if (file.open(QFile::ReadOnly)) {
+        QCryptographicHash hash(QCryptographicHash::Sha256);
+        if (m_fileChecksumSizeBytes > 0) {
+            hash.addData(file.read(m_fileChecksumSizeBytes));
+        } else {
+            hash.addData(&file);
         }
-        // If we fail to open the file return the last known checksum, this
-        // prevents unnecessary merge requests on intermittent network shares
-        return m_fileChecksum;
-    });
+        return hash.result();
+    }
+    // If we fail to open the file return the last known checksum, this
+    // prevents unnecessary merge requests on intermittent network shares
+    return m_fileChecksum;
 }
 
 BulkFileWatcher::BulkFileWatcher(QObject* parent)

src/core/FileWatcher.h

@@ -29,6 +29,7 @@ class FileWatcher : public QObject
 
 public:
     explicit FileWatcher(QObject* parent = nullptr);
+    ~FileWatcher() override;
 
     void start(const QString& path, int checksumIntervalSeconds = 0, int checksumSizeKibibytes = -1);
     void stop();

src/core/Metadata.cpp

@@ -31,7 +31,13 @@ Metadata::Metadata(QObject* parent)
     , m_customData(new CustomData(this))
     , m_updateDatetime(true)
 {
-    m_data.generator = "KeePassXC";
+    init();
+    connect(m_customData, SIGNAL(customDataModified()), SIGNAL(metadataModified()));
+}
+
+void Metadata::init()
+{
+    m_data.generator = QStringLiteral("KeePassXC");
     m_data.maintenanceHistoryDays = 365;
     m_data.masterKeyChangeRec = -1;
     m_data.masterKeyChangeForce = -1;
@@ -52,8 +58,17 @@ Metadata::Metadata(QObject* parent)
     m_entryTemplatesGroupChanged = now;
     m_masterKeyChanged = now;
     m_settingsChanged = now;
+}
 
-    connect(m_customData, SIGNAL(customDataModified()), this, SIGNAL(metadataModified()));
+void Metadata::clear()
+{
+    init();
+    m_customIcons.clear();
+    m_customIconCacheKeys.clear();
+    m_customIconScaledCacheKeys.clear();
+    m_customIconsOrder.clear();
+    m_customIconsHashes.clear();
+    m_customData->clear();
 }
 
 template <class P, class V> bool Metadata::set(P& property, const V& value)

src/core/Metadata.h

@@ -62,6 +62,9 @@ public:
         bool protectNotes;
     };
 
+    void init();
+    void clear();
+
     QString generator() const;
     QString name() const;
     QDateTime nameChanged() const;

src/gui/DatabaseTabWidget.cpp

@@ -353,13 +353,17 @@ bool DatabaseTabWidget::closeDatabaseTab(DatabaseWidget* dbWidget)
  */
 bool DatabaseTabWidget::closeAllDatabaseTabs()
 {
-    while (count() > 0) {
-        if (!closeDatabaseTab(0)) {
-            return false;
+    // Attempt to lock all databases first to prevent closing only a portion of tabs
+    if (lockDatabases()) {
+        while (count() > 0) {
+            if (!closeDatabaseTab(0)) {
+                return false;
+            }
         }
+        return true;
     }
 
-    return true;
+    return false;
 }
 
 bool DatabaseTabWidget::saveDatabase(int index)
@@ -597,15 +601,26 @@ DatabaseWidget* DatabaseTabWidget::currentDatabaseWidget()
     return qobject_cast<DatabaseWidget*>(currentWidget());
 }
 
-void DatabaseTabWidget::lockDatabases()
+/**
+ * Attempt to lock all open databases
+ *
+ * @return return true if all databases are locked
+ */
+bool DatabaseTabWidget::lockDatabases()
 {
+    int numLocked = 0;
     for (int i = 0, c = count(); i < c; ++i) {
         auto dbWidget = databaseWidgetFromIndex(i);
-        if (dbWidget->lock() && dbWidget->database()->filePath().isEmpty()) {
-            // If we locked a database without a file close the tab
-            closeDatabaseTab(dbWidget);
+        if (dbWidget->lock()) {
+            ++numLocked;
+            if (dbWidget->database()->filePath().isEmpty()) {
+                // If we locked a database without a file close the tab
+                closeDatabaseTab(dbWidget);
+            }
         }
     }
+
+    return numLocked == count();
 }
 
 /**

src/gui/DatabaseTabWidget.h

@@ -71,7 +71,7 @@ public slots:
     void exportToCsv();
     void exportToHtml();
 
-    void lockDatabases();
+    bool lockDatabases();
     void closeDatabaseFromSender();
     void unlockDatabaseInDialog(DatabaseWidget* dbWidget, DatabaseOpenDialog::Intent intent);
     void unlockDatabaseInDialog(DatabaseWidget* dbWidget, DatabaseOpenDialog::Intent intent, const QString& filePath);

src/gui/DatabaseWidget.cpp

@@ -255,15 +255,15 @@ QSharedPointer<Database> DatabaseWidget::database() const
 DatabaseWidget::Mode DatabaseWidget::currentMode() const
 {
     if (currentWidget() == nullptr) {
-        return DatabaseWidget::Mode::None;
+        return Mode::None;
     } else if (currentWidget() == m_mainWidget) {
-        return DatabaseWidget::Mode::ViewMode;
+        return Mode::ViewMode;
     } else if (currentWidget() == m_databaseOpenWidget || currentWidget() == m_keepass1OpenWidget) {
-        return DatabaseWidget::Mode::LockedMode;
+        return Mode::LockedMode;
     } else if (currentWidget() == m_csvImportWizard) {
-        return DatabaseWidget::Mode::ImportMode;
+        return Mode::ImportMode;
     } else {
-        return DatabaseWidget::Mode::EditMode;
+        return Mode::EditMode;
     }
 }
 
@@ -272,6 +272,11 @@ bool DatabaseWidget::isLocked() const
     return currentMode() == Mode::LockedMode;
 }
 
+bool DatabaseWidget::isSaving() const
+{
+    return m_db->isSaving();
+}
+
 bool DatabaseWidget::isSearchActive() const
 {
     return m_entryView->inSearchMode();
@@ -1380,6 +1385,12 @@ bool DatabaseWidget::lock()
         return true;
     }
 
+    // Don't try to lock the database while saving, this will cause a deadlock
+    if (m_db->isSaving()) {
+        QTimer::singleShot(200, this, SLOT(lock()));
+        return false;
+    }
+
     emit databaseLockRequested();
 
     clipboard()->clearCopiedText();
@@ -1660,7 +1671,6 @@ bool DatabaseWidget::save()
 
     auto focusWidget = qApp->focusWidget();
 
-    // TODO: Make this async
     // Lock out interactions
     m_entryView->setDisabled(true);
     m_groupView->setDisabled(true);

src/gui/DatabaseWidget.h

@@ -80,6 +80,7 @@ public:
 
     DatabaseWidget::Mode currentMode() const;
     bool isLocked() const;
+    bool isSaving() const;
     bool isSearchActive() const;
     bool isEntryEditActive() const;
     bool isGroupEditActive() const;