From f4b6db5def62b09d4ea6df41192de29212e531d7 Mon Sep 17 00:00:00 2001
From: Matt Hardcastle <m@hardcastle.com>
Date: Sun, 2 Dec 2018 17:08:27 -0800
Subject: [PATCH] Use QSaveFile when persisting domain settings to disk

Prior to this change the `DomainServerSettingsManager::persistToFile()`
method wrote directly to the settings file. It also did limited error
checking. These limitations could lead to a situation where a crashed
domain-server process, a file system backup, a settings file copy, or a
hardware error could corrupt the setting file.

This change swaps the `QFile` class for `QSaveFile` and uses its atomic
saving features, which writes the changes to the settings file to a file
in the same directory as the settings file and then does a rename to
replace the original. If the rename, or any of the file operations, fail
the original settings file remains in place.
---
 .../src/DomainServerSettingsManager.cpp       | 57 ++++++++++++-------
 1 file changed, 37 insertions(+), 20 deletions(-)

diff --git a/domain-server/src/DomainServerSettingsManager.cpp b/domain-server/src/DomainServerSettingsManager.cpp
index 780fad15f2..0f6807d5a0 100644
--- a/domain-server/src/DomainServerSettingsManager.cpp
+++ b/domain-server/src/DomainServerSettingsManager.cpp
@@ -22,6 +22,7 @@
 #include <QtCore/QThread>
 #include <QtCore/QUrl>
 #include <QtCore/QUrlQuery>
+#include <QSaveFile>
 
 #include <AccountManager.h>
 #include <Assignment.h>
@@ -1714,28 +1715,44 @@ void DomainServerSettingsManager::sortPermissions() {
 }
 
 void DomainServerSettingsManager::persistToFile() {
-    sortPermissions();
-
-    // make sure we have the dir the settings file is supposed to live in
-    QFileInfo settingsFileInfo(_configMap.getUserConfigFilename());
-
-    if (!settingsFileInfo.dir().exists()) {
-        settingsFileInfo.dir().mkpath(".");
-    }
-
-    QFile settingsFile(_configMap.getUserConfigFilename());
-
-    if (settingsFile.open(QIODevice::WriteOnly)) {
-        // take a read lock so we can grab the config and write it to file
-        QReadLocker locker(&_settingsLock);
-        settingsFile.write(QJsonDocument::fromVariant(_configMap.getConfig()).toJson());
-    } else {
-        qCritical("Could not write to JSON settings file. Unable to persist settings.");
-
-        // failed to write, reload whatever the current config state is
-        // with a write lock since we're about to overwrite the config map
+    QString settingsFilename = _configMap.getUserConfigFilename();
+    QDir settingsDir = QFileInfo(settingsFilename).dir();
+    if (!settingsDir.exists() && !settingsDir.mkpath(".")) {
+        // If the path already exists when the `mkpath` method is
+        // called, it will return true. It will only return false if the
+        // path doesn't exist after the call returns.
+        qCritical("Could not create the settings file parent directory. Unable to persist settings.");
         QWriteLocker locker(&_settingsLock);
         _configMap.loadConfig();
+        return;
+    }
+    QSaveFile settingsFile(settingsFilename);
+    if (!settingsFile.open(QIODevice::WriteOnly)) {
+        qCritical("Could not open the JSON settings file. Unable to persist settings.");
+        QWriteLocker locker(&_settingsLock);
+        _configMap.loadConfig();
+        return;
+    }
+
+    sortPermissions();
+
+    QVariantMap conf;
+    {
+        QReadLocker locker(&_settingsLock);
+        conf = _configMap.getConfig();
+    }
+    QByteArray json = QJsonDocument::fromVariant(conf).toJson();
+    if (settingsFile.write(json) == -1) {
+        qCritical("Could not write to JSON settings file. Unable to persist settings.");
+        QWriteLocker locker(&_settingsLock);
+        _configMap.loadConfig();
+        return;
+    }
+    if (!settingsFile.commit()) {
+        qCritical("Could not commit writes to JSON settings file. Unable to persist settings.");
+        QWriteLocker locker(&_settingsLock);
+        _configMap.loadConfig();
+        return; // defend against future code
     }
 }