fix(persistence): atomic + owner-only settings/address-book writes
settings.json and addressbook.json were written in place with a bare ofstream — a crash or power loss mid-write truncated the file, and on the next launch the parse failure silently reset every preference (hidden/favorite addresses, labels, pool workers, language, theme, lite-server list) because the next save overwrote the corrupt file with defaults. Add Platform::writeFileAtomically() (temp file -> fsync -> atomic rename; dir fsync on POSIX, MoveFileEx on Windows; optional owner-only 0600) and route both saves through it. On a parse failure, quarantine the unreadable settings file to settings.json.corrupt-<ts> instead of clobbering it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -11,8 +11,10 @@
|
|||||||
#include <nlohmann/json.hpp>
|
#include <nlohmann/json.hpp>
|
||||||
#include <fstream>
|
#include <fstream>
|
||||||
#include <filesystem>
|
#include <filesystem>
|
||||||
|
#include <ctime>
|
||||||
|
|
||||||
#include "../util/logger.h"
|
#include "../util/logger.h"
|
||||||
|
#include "../util/platform.h"
|
||||||
|
|
||||||
#ifdef _WIN32
|
#ifdef _WIN32
|
||||||
#include <shlobj.h>
|
#include <shlobj.h>
|
||||||
@@ -291,6 +293,17 @@ bool Settings::load(const std::string& path)
|
|||||||
return true;
|
return true;
|
||||||
} catch (const std::exception& e) {
|
} catch (const std::exception& e) {
|
||||||
DEBUG_LOGF("Failed to parse settings: %s\n", e.what());
|
DEBUG_LOGF("Failed to parse settings: %s\n", e.what());
|
||||||
|
// The file exists but is unparseable (truncated/corrupt). Quarantine it so the
|
||||||
|
// next save() doesn't silently overwrite it with defaults — the user's data stays
|
||||||
|
// recoverable. Proceed with in-memory defaults.
|
||||||
|
file.close();
|
||||||
|
std::error_code ec;
|
||||||
|
const std::string quarantine =
|
||||||
|
path + ".corrupt-" + std::to_string(static_cast<long long>(std::time(nullptr)));
|
||||||
|
fs::rename(path, quarantine, ec);
|
||||||
|
if (!ec) {
|
||||||
|
DEBUG_LOGF("Quarantined corrupt settings to %s\n", quarantine.c_str());
|
||||||
|
}
|
||||||
return false;
|
return false;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -411,17 +424,11 @@ bool Settings::save(const std::string& path)
|
|||||||
}
|
}
|
||||||
|
|
||||||
try {
|
try {
|
||||||
// Ensure directory exists
|
// Atomic + durable: write to a temp file, fsync, then rename over the real file.
|
||||||
fs::path p(path);
|
// A crash mid-write can no longer truncate settings.json (which would silently
|
||||||
fs::create_directories(p.parent_path());
|
// reset every preference on the next launch). Owner-only (0600) — it carries the
|
||||||
|
// lite-server list and address metadata.
|
||||||
std::ofstream file(path);
|
return util::Platform::writeFileAtomically(path, j.dump(4), /*restrictPermissions=*/true);
|
||||||
if (!file.is_open()) {
|
|
||||||
return false;
|
|
||||||
}
|
|
||||||
|
|
||||||
file << j.dump(4);
|
|
||||||
return true;
|
|
||||||
} catch (const std::exception& e) {
|
} catch (const std::exception& e) {
|
||||||
DEBUG_LOGF("Failed to save settings: %s\n", e.what());
|
DEBUG_LOGF("Failed to save settings: %s\n", e.what());
|
||||||
return false;
|
return false;
|
||||||
|
|||||||
@@ -9,6 +9,7 @@
|
|||||||
#include <filesystem>
|
#include <filesystem>
|
||||||
|
|
||||||
#include "../util/logger.h"
|
#include "../util/logger.h"
|
||||||
|
#include "../util/platform.h"
|
||||||
|
|
||||||
#ifdef _WIN32
|
#ifdef _WIN32
|
||||||
#include <shlobj.h>
|
#include <shlobj.h>
|
||||||
@@ -113,20 +114,16 @@ bool AddressBook::save()
|
|||||||
j["entries"].push_back(e);
|
j["entries"].push_back(e);
|
||||||
}
|
}
|
||||||
|
|
||||||
// Ensure directory exists
|
// Atomic + durable: temp file + fsync + rename, so a crash mid-write can't
|
||||||
fs::path p(file_path_);
|
// truncate addressbook.json (which is fully rewritten on every entry change).
|
||||||
fs::create_directories(p.parent_path());
|
// Owner-only (0600) — it holds the user's saved contacts.
|
||||||
|
if (!util::Platform::writeFileAtomically(file_path_, j.dump(2), /*restrictPermissions=*/true)) {
|
||||||
std::ofstream file(file_path_);
|
DEBUG_LOGF("Could not write address book: %s\n", file_path_.c_str());
|
||||||
if (!file.is_open()) {
|
|
||||||
DEBUG_LOGF("Could not open address book for writing: %s\n", file_path_.c_str());
|
|
||||||
return false;
|
return false;
|
||||||
}
|
}
|
||||||
|
|
||||||
file << j.dump(2);
|
|
||||||
DEBUG_LOGF("Address book saved: %zu entries\n", entries_.size());
|
DEBUG_LOGF("Address book saved: %zu entries\n", entries_.size());
|
||||||
return true;
|
return true;
|
||||||
|
|
||||||
} catch (const std::exception& e) {
|
} catch (const std::exception& e) {
|
||||||
DEBUG_LOGF("Error saving address book: %s\n", e.what());
|
DEBUG_LOGF("Error saving address book: %s\n", e.what());
|
||||||
return false;
|
return false;
|
||||||
|
|||||||
@@ -30,6 +30,7 @@
|
|||||||
#include <tlhelp32.h>
|
#include <tlhelp32.h>
|
||||||
#else
|
#else
|
||||||
#include <unistd.h>
|
#include <unistd.h>
|
||||||
|
#include <fcntl.h>
|
||||||
#include <pwd.h>
|
#include <pwd.h>
|
||||||
#include <dirent.h>
|
#include <dirent.h>
|
||||||
#include <dlfcn.h>
|
#include <dlfcn.h>
|
||||||
@@ -239,6 +240,82 @@ bool Platform::deleteFile(const std::string& path)
|
|||||||
#endif
|
#endif
|
||||||
}
|
}
|
||||||
|
|
||||||
|
bool Platform::writeFileAtomically(const std::string& path,
|
||||||
|
const std::string& content,
|
||||||
|
bool restrictPermissions)
|
||||||
|
{
|
||||||
|
namespace fs = std::filesystem;
|
||||||
|
std::error_code ec;
|
||||||
|
fs::path target(path);
|
||||||
|
fs::path dir = target.parent_path();
|
||||||
|
if (!dir.empty()) {
|
||||||
|
fs::create_directories(dir, ec); // ignore ec; the open below will fail if truly unwritable
|
||||||
|
}
|
||||||
|
const std::string tmp = path + ".tmp";
|
||||||
|
|
||||||
|
#ifdef _WIN32
|
||||||
|
{
|
||||||
|
std::ofstream out(tmp, std::ios::binary | std::ios::trunc);
|
||||||
|
if (!out.is_open()) return false;
|
||||||
|
out.write(content.data(), static_cast<std::streamsize>(content.size()));
|
||||||
|
out.flush();
|
||||||
|
if (!out) { out.close(); DeleteFileA(tmp.c_str()); return false; }
|
||||||
|
}
|
||||||
|
// Flush the temp file's contents to stable storage before the rename.
|
||||||
|
HANDLE h = CreateFileA(tmp.c_str(), GENERIC_WRITE, 0, nullptr,
|
||||||
|
OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr);
|
||||||
|
if (h != INVALID_HANDLE_VALUE) {
|
||||||
|
FlushFileBuffers(h);
|
||||||
|
CloseHandle(h);
|
||||||
|
}
|
||||||
|
(void)restrictPermissions; // owner-only ACLs rely on the per-user profile dir on Windows
|
||||||
|
// MoveFileEx with REPLACE_EXISTING is atomic on NTFS; WRITE_THROUGH makes it durable.
|
||||||
|
if (MoveFileExA(tmp.c_str(), path.c_str(),
|
||||||
|
MOVEFILE_REPLACE_EXISTING | MOVEFILE_WRITE_THROUGH) == 0) {
|
||||||
|
DeleteFileA(tmp.c_str());
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
return true;
|
||||||
|
#else
|
||||||
|
const mode_t mode = restrictPermissions ? 0600 : 0644;
|
||||||
|
int fd = ::open(tmp.c_str(), O_WRONLY | O_CREAT | O_TRUNC, mode);
|
||||||
|
if (fd < 0) return false;
|
||||||
|
if (restrictPermissions) {
|
||||||
|
::fchmod(fd, 0600); // enforce even if the temp file pre-existed with looser perms
|
||||||
|
}
|
||||||
|
bool ok = true;
|
||||||
|
size_t written = 0;
|
||||||
|
const char* data = content.data();
|
||||||
|
while (written < content.size()) {
|
||||||
|
ssize_t n = ::write(fd, data + written, content.size() - written);
|
||||||
|
if (n < 0) {
|
||||||
|
if (errno == EINTR) continue;
|
||||||
|
ok = false;
|
||||||
|
break;
|
||||||
|
}
|
||||||
|
written += static_cast<size_t>(n);
|
||||||
|
}
|
||||||
|
if (ok && ::fsync(fd) != 0) ok = false;
|
||||||
|
::close(fd);
|
||||||
|
if (!ok) {
|
||||||
|
::remove(tmp.c_str());
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
if (::rename(tmp.c_str(), path.c_str()) != 0) {
|
||||||
|
::remove(tmp.c_str());
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
// fsync the directory so the rename (the metadata that makes the new file
|
||||||
|
// visible) survives a crash, not just the file contents.
|
||||||
|
int dfd = ::open(dir.empty() ? "." : dir.c_str(), O_RDONLY);
|
||||||
|
if (dfd >= 0) {
|
||||||
|
::fsync(dfd);
|
||||||
|
::close(dfd);
|
||||||
|
}
|
||||||
|
return true;
|
||||||
|
#endif
|
||||||
|
}
|
||||||
|
|
||||||
std::string Platform::getExecutableDirectory()
|
std::string Platform::getExecutableDirectory()
|
||||||
{
|
{
|
||||||
#ifdef _WIN32
|
#ifdef _WIN32
|
||||||
|
|||||||
@@ -74,6 +74,26 @@ public:
|
|||||||
* @return true if successful or file didn't exist
|
* @return true if successful or file didn't exist
|
||||||
*/
|
*/
|
||||||
static bool deleteFile(const std::string& path);
|
static bool deleteFile(const std::string& path);
|
||||||
|
|
||||||
|
/**
|
||||||
|
* @brief Write a file atomically and durably.
|
||||||
|
*
|
||||||
|
* Writes @p content to `<path>.tmp`, flushes it to stable storage
|
||||||
|
* (fsync / FlushFileBuffers), then atomically renames it over @p path
|
||||||
|
* (POSIX rename / Win32 MoveFileEx with REPLACE_EXISTING). A crash or power
|
||||||
|
* loss at any point leaves either the old file intact or the fully-written
|
||||||
|
* new one — never a truncated/corrupt file. The parent directory is created
|
||||||
|
* if missing, and on POSIX it is fsync'd so the rename itself is durable.
|
||||||
|
*
|
||||||
|
* @param path Destination file path.
|
||||||
|
* @param content Bytes to write.
|
||||||
|
* @param restrictPermissions When true, create the file owner-only (0600 on
|
||||||
|
* POSIX) before the rename so it is never briefly world-readable.
|
||||||
|
* @return true on success; on failure any pre-existing file is left untouched.
|
||||||
|
*/
|
||||||
|
static bool writeFileAtomically(const std::string& path,
|
||||||
|
const std::string& content,
|
||||||
|
bool restrictPermissions = false);
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* @brief Get the directory containing the executable
|
* @brief Get the directory containing the executable
|
||||||
|
|||||||
Reference in New Issue
Block a user