From e23d07e965a25e6246f9cd10353dbef181c5c657 Mon Sep 17 00:00:00 2001 From: Hornwitser Date: Wed, 3 Nov 2021 16:14:18 +0100 Subject: [PATCH] Use safe file writing pattern (#421) Write new file contents to a temporary file and then rename over the target file to avoid data loss in the unlikely event there's an error writing the new file contents or the process is interrupted. Resolves #416. --- packages/create/create.js | 22 ++++++++++++++-------- packages/lib/database.js | 5 +++-- packages/lib/factorio/patch.js | 4 +++- packages/lib/factorio/server.js | 11 ++++++----- packages/lib/file_ops.js | 22 ++++++++++++++++++++++ packages/lib/shared_commands.js | 7 ++++--- packages/master/master.js | 7 +++++-- packages/master/src/Master.js | 7 ++++--- packages/master/src/UserManager.js | 3 ++- packages/master/src/routes.js | 4 ++-- packages/slave/slave.js | 10 +++++----- plugins/inventory_sync/master.js | 3 ++- plugins/research_sync/master.js | 3 ++- plugins/subspace_storage/master.js | 3 ++- test/lib/file_ops.js | 18 ++++++++++++++++++ 15 files changed, 94 insertions(+), 35 deletions(-) diff --git a/packages/create/create.js b/packages/create/create.js index 1b3a1bbe..3497ec9d 100755 --- a/packages/create/create.js +++ b/packages/create/create.js @@ -85,6 +85,12 @@ class LineSplitter extends stream.Transform { class InstallError extends Error { } +async function safeOutputFile(file, data, options={}) { + let temporary = `${file}.tmp`; + await fs.outputFile(temporary, data, options); + await fs.rename(temporary, file); +} + async function execFile(cmd, args) { logger.verbose(`executing ${cmd} ${args.join(" ")}`); const asyncExec = util.promisify(child_process.execFile); @@ -165,7 +171,7 @@ async function validateInstallDir() { async function installClusterio(mode, plugins) { try { - await fs.writeFile("package.json", JSON.stringify({ + await safeOutputFile("package.json", JSON.stringify({ name: "clusterio-install", private: true, }, null, 2), { flag: "wx" }); @@ -213,7 +219,7 @@ async function installClusterio(mode, plugins) { } } try { - await fs.writeFile("plugin-list.json", JSON.stringify([...pluginList], null, 4)); + await safeOutputFile("plugin-list.json", JSON.stringify([...pluginList], null, 4)); } catch (err) { throw new InstallError(`Error writing plugin-list.json: ${err.message}`); } @@ -223,17 +229,17 @@ async function installClusterio(mode, plugins) { async function writeScripts(mode) { if (["standalone", "master"].includes(mode)) { if (process.platform === "win32") { - await fs.writeFile( + await safeOutputFile( "run-master.cmd", "@echo off\n.\\node_modules\\.bin\\clusteriomaster.cmd run\n" ); } else { - await fs.writeFile( + await safeOutputFile( "run-master.sh", "#!/bin/sh\nexec ./node_modules/.bin/clusteriomaster run\n", { mode: 0o755 }, ); - await fs.outputFile( + await safeOutputFile( "systemd/clusteriomaster.service", `[Unit] Description=Clusterio Master @@ -254,17 +260,17 @@ WantedBy=multi-user.target if (["standalone", "slave"].includes(mode)) { if (process.platform === "win32") { - await fs.writeFile( + await safeOutputFile( "run-slave.cmd", "@echo off\n.\\node_modules\\.bin\\clusterioslave.cmd run\n" ); } else { - await fs.writeFile( + await safeOutputFile( "run-slave.sh", "#!/bin/sh\nexec ./node_modules/.bin/clusterioslave run\n", { mode: 0o755 }, ); - await fs.outputFile( + await safeOutputFile( "systemd/clusterioslave.service", `[Unit] Description=Clusterio Slave diff --git a/packages/lib/database.js b/packages/lib/database.js index 665cf58a..87d3ea5e 100644 --- a/packages/lib/database.js +++ b/packages/lib/database.js @@ -8,6 +8,7 @@ "use strict"; const fs = require("fs-extra"); +const libFileOps = require("./file_ops"); const { basicType } = require("./helpers"); @@ -78,7 +79,7 @@ async function loadJsonAsMap(filePath) { */ async function saveMapAsJson(filePath, map) { let obj = mapToObject(map); - await fs.outputFile(filePath, JSON.stringify(obj, null, 4)); + await libFileOps.safeOutputFile(filePath, JSON.stringify(obj, null, 4)); } /** @@ -138,7 +139,7 @@ async function loadJsonArrayAsMap(filePath) { * @throws {Error} if an error occured writing to the file. */ async function saveMapAsJsonArray(filePath, map) { - await fs.outputFile(filePath, JSON.stringify([...map.values()], null, 4)); + await libFileOps.safeOutputFile(filePath, JSON.stringify([...map.values()], null, 4)); } diff --git a/packages/lib/factorio/patch.js b/packages/lib/factorio/patch.js index 59df7b83..f96e931a 100644 --- a/packages/lib/factorio/patch.js +++ b/packages/lib/factorio/patch.js @@ -284,9 +284,11 @@ async function patch(savePath, modules) { root.file("clusterio.json", JSON.stringify(patchInfo, null, 4)); // Write back the save + let tempSavePath = `${savePath}.tmp`; let stream = zip.generateNodeStream({ compression: "DEFLATE" }); - let pipe = stream.pipe(fs.createWriteStream(savePath)); + let pipe = stream.pipe(fs.createWriteStream(tempSavePath)); await events.once(pipe, "finish"); + await fs.rename(tempSavePath, savePath); } module.exports = { diff --git a/packages/lib/factorio/server.js b/packages/lib/factorio/server.js index ec2d4766..d680aae6 100644 --- a/packages/lib/factorio/server.js +++ b/packages/lib/factorio/server.js @@ -6,10 +6,11 @@ const path = require("path"); const events = require("events"); const util = require("util"); const crypto = require("crypto"); - -const libIni = require("../ini"); const rconClient = require("rcon-client"); + const libErrors = require("../errors"); +const libFileOps = require("../file_ops"); +const libIni = require("../ini"); const { logger } = require("../logging"); const { escapeRegExp } = require("../helpers"); @@ -1173,17 +1174,17 @@ class FactorioServer extends events.EventEmitter { "write-data": this.writePath(), }, }); - await fs.outputFile(this.writePath("config.ini"), content); + await libFileOps.safeOutputFile(this.writePath("config.ini"), content); } async _writeMapSettings(mapGenSettings, mapSettings) { if (mapGenSettings) { - await fs.outputFile( + await libFileOps.safeOutputFile( this.writePath("map-gen-settings.json"), JSON.stringify(mapGenSettings, null, 4) ); } if (mapSettings) { - await fs.outputFile( + await libFileOps.safeOutputFile( this.writePath("map-settings.json"), JSON.stringify(mapSettings, null, 4) ); } diff --git a/packages/lib/file_ops.js b/packages/lib/file_ops.js index bc5fe91e..4460fe1b 100644 --- a/packages/lib/file_ops.js +++ b/packages/lib/file_ops.js @@ -69,7 +69,29 @@ async function findUnusedName(directory, name, extension = "") { } } +/** + * Safely write data to a file + * + * Same as fs-extra.outputFile except the data is written to a temporary + * file that's renamed over the target file. The name of the temporary file + * is the same as the target file with the suffix `.tmp` added. + * + * If the operation fails it may leave behind the temporary file. This + * should not be too much of an issue as the next time the same file is + * written the temporary will be overwritten and renamed to the target file. + * + * @param {string} file - Path to file to write. + * @param {string|Buffer} data - Content to write. + * @param {object|string} options - see fs.writeFile, `flag` must not be set. + */ +async function safeOutputFile(file, data, options={}) { + let temporary = `${file}.tmp`; + await fs.outputFile(temporary, data, options); + await fs.rename(temporary, file); +} + module.exports = { getNewestFile, findUnusedName, + safeOutputFile, }; diff --git a/packages/lib/shared_commands.js b/packages/lib/shared_commands.js index 51155323..1a2f62a3 100644 --- a/packages/lib/shared_commands.js +++ b/packages/lib/shared_commands.js @@ -7,6 +7,7 @@ const fs = require("fs-extra"); const path = require("path"); const libConfig = require("./config"); +const libFileOps = require("./file_ops"); const { logger } = require("./logging"); const libHelpers = require("./helpers"); @@ -70,7 +71,7 @@ async function handlePluginCommand(args, pluginList, pluginListPath) { } pluginList.set(pluginInfo.name, pluginPath); - await fs.outputFile(pluginListPath, JSON.stringify([...pluginList], null, 4)); + await libFileOps.safeOutputFile(pluginListPath, JSON.stringify([...pluginList], null, 4)); print(`Added ${pluginInfo.name}`); } else if (command === "remove") { @@ -80,7 +81,7 @@ async function handlePluginCommand(args, pluginList, pluginListPath) { return; } - await fs.outputFile(pluginListPath, JSON.stringify([...pluginList], null, 4)); + await libFileOps.safeOutputFile(pluginListPath, JSON.stringify([...pluginList], null, 4)); print(`Removed ${args.name}`); } else if (command === "list") { @@ -155,7 +156,7 @@ async function handleConfigCommand(args, instance, configPath) { try { instance.set(args.field, args.value); - await fs.outputFile(configPath, JSON.stringify(instance.serialize(), null, 4)); + await libFileOps.safeOutputFile(configPath, JSON.stringify(instance.serialize(), null, 4)); } catch (err) { if (err instanceof libConfig.InvalidField || err instanceof libConfig.InvalidValue) { logger.error(err.message); diff --git a/packages/master/master.js b/packages/master/master.js index f1a06899..89d60bf8 100755 --- a/packages/master/master.js +++ b/packages/master/master.js @@ -26,6 +26,7 @@ const jwt = require("jsonwebtoken"); // homebrew modules const libErrors = require("@clusterio/lib/errors"); +const libFileOps = require("@clusterio/lib/file_ops"); const libPluginLoader = require("@clusterio/lib/plugin_loader"); const libPrometheus = require("@clusterio/lib/prometheus"); const libConfig = require("@clusterio/lib/config"); @@ -162,7 +163,7 @@ async function handleBootstrapCommand(args, masterConfig) { console.log(content); } else { logger.info(`Writing ${args.output}`); - await fs.outputFile(args.output, content); + await libFileOps.safeOutputFile(args.output, content); } } } @@ -294,7 +295,9 @@ async function initialize() { let asyncRandomBytes = util.promisify(crypto.randomBytes); let bytes = await asyncRandomBytes(256); parameters.masterConfig.set("master.auth_secret", bytes.toString("base64")); - await fs.outputFile(parameters.masterConfigPath, JSON.stringify(parameters.masterConfig.serialize(), null, 4)); + await libFileOps.safeOutputFile( + parameters.masterConfigPath, JSON.stringify(parameters.masterConfig.serialize(), null, 4) + ); } if (command === "config") { diff --git a/packages/master/src/Master.js b/packages/master/src/Master.js index 52711027..c4f2ee6e 100644 --- a/packages/master/src/Master.js +++ b/packages/master/src/Master.js @@ -10,6 +10,7 @@ const path = require("path"); const libConfig = require("@clusterio/lib/config"); const libErrors = require("@clusterio/lib/errors"); +const libFileOps = require("@clusterio/lib/file_ops"); const libLink = require("@clusterio/lib/link"); const libPlugin = require("@clusterio/lib/plugin"); const libPluginLoader = require("@clusterio/lib/plugin_loader"); @@ -241,7 +242,7 @@ class Master { logger.info("Stopping master"); logger.info("Saving config"); - await fs.outputFile(this.configPath, JSON.stringify(this.config.serialize(), null, 4)); + await libFileOps.safeOutputFile(this.configPath, JSON.stringify(this.config.serialize(), null, 4)); if (this.devMiddleware) { await new Promise((resolve, reject) => { this.devMiddleware.close(resolve); }); @@ -297,7 +298,7 @@ class Master { } static async saveSlaves(filePath, slaves) { - await fs.outputFile(filePath, JSON.stringify([...slaves.entries()], null, 4)); + await libFileOps.safeOutputFile(filePath, JSON.stringify([...slaves.entries()], null, 4)); } static async loadInstances(filePath) { @@ -329,7 +330,7 @@ class Master { serialized.push(instance.config.serialize()); } - await fs.outputFile(filePath, JSON.stringify(serialized, null, 4)); + await libFileOps.safeOutputFile(filePath, JSON.stringify(serialized, null, 4)); } static addAppRoutes(app, pluginInfos) { diff --git a/packages/master/src/UserManager.js b/packages/master/src/UserManager.js index e0fa8288..3c00a85e 100644 --- a/packages/master/src/UserManager.js +++ b/packages/master/src/UserManager.js @@ -1,6 +1,7 @@ "use strict"; const fs = require("fs-extra"); +const libFileOps = require("@clusterio/lib/file_ops"); const libUsers = require("@clusterio/lib/users"); /** @@ -62,7 +63,7 @@ class UserManager { users: serializedUsers, roles: serializedRoles, }; - await fs.outputFile(filePath, JSON.stringify(serialized, null, 4)); + await libFileOps.safeOutputFile(filePath, JSON.stringify(serialized, null, 4)); } /** diff --git a/packages/master/src/routes.js b/packages/master/src/routes.js index 749b2b55..c0217d20 100644 --- a/packages/master/src/routes.js +++ b/packages/master/src/routes.js @@ -2,13 +2,13 @@ const Busboy = require("busboy"); const crypto = require("crypto"); const events = require("events"); -const fs = require("fs-extra"); const JSZip = require("jszip"); const jwt = require("jsonwebtoken"); const path = require("path"); const util = require("util"); const libErrors = require("@clusterio/lib/errors"); +const libFileOps = require("@clusterio/lib/file_ops"); const libHelpers = require("@clusterio/lib/helpers"); const libLink = require("@clusterio/lib/link"); const libPlugin = require("@clusterio/lib/plugin"); @@ -187,7 +187,7 @@ async function uploadExport(req, res) { } let name = path.posix.basename(filePath); - await fs.outputFile(path.join("static", "export", name), await file.async("nodebuffer")); + await libFileOps.safeOutputFile(path.join("static", "export", name), await file.async("nodebuffer")); } res.sendStatus(200); diff --git a/packages/slave/slave.js b/packages/slave/slave.js index a4db4b7e..c7b4b456 100755 --- a/packages/slave/slave.js +++ b/packages/slave/slave.js @@ -376,7 +376,7 @@ class Instance extends libLink.Link { */ async writeServerSettings() { let serverSettings = await this.resolveServerSettings(this.config.get("factorio.settings")); - await fs.writeFile( + await libFileOps.safeOutputFile( this.server.writePath("server-settings.json"), JSON.stringify(serverSettings, null, 4) ); @@ -410,7 +410,7 @@ class Instance extends libLink.Link { if (this.config.get("factorio.sync_adminlist")) { this.logger.verbose("Writing server-adminlist.json"); - fs.outputFile( + libFileOps.safeOutputFile( this.server.writePath("server-adminlist.json"), JSON.stringify([...this._slave.adminlist], null, 4) ); @@ -418,7 +418,7 @@ class Instance extends libLink.Link { if (this.config.get("factorio.sync_banlist")) { this.logger.verbose("Writing server-banlist.json"); - fs.outputFile( + libFileOps.safeOutputFile( this.server.writePath("server-banlist.json"), JSON.stringify([...this._slave.banlist].map( ([username, reason]) => ({ username, reason }) @@ -428,7 +428,7 @@ class Instance extends libLink.Link { if (this.config.get("factorio.sync_whitelist")) { this.logger.verbose("Writing server-whitelist.json"); - fs.outputFile( + libFileOps.safeOutputFile( this.server.writePath("server-whitelist.json"), JSON.stringify([...this._slave.whitelist], null, 4) ); @@ -1299,7 +1299,7 @@ class Slave extends libLink.Link { _warning: "Changes to this file will be overwritten by the master server's copy.", ...instanceInfo.config.serialize(), }; - await fs.outputFile( + await libFileOps.safeOutputFile( path.join(instanceInfo.path, "instance.json"), JSON.stringify(warnedOutput, null, 4) ); diff --git a/plugins/inventory_sync/master.js b/plugins/inventory_sync/master.js index 907102a1..6ef37d28 100644 --- a/plugins/inventory_sync/master.js +++ b/plugins/inventory_sync/master.js @@ -2,6 +2,7 @@ const fs = require("fs-extra"); const path = require("path"); +const libFileOps = require("@clusterio/lib/file_ops"); const libPlugin = require("@clusterio/lib/plugin"); const libErrors = require("@clusterio/lib/errors"); @@ -26,7 +27,7 @@ async function saveDatabase(masterConfig, playerDatastore, logger) { let file = path.resolve(masterConfig.get("master.database_directory"), "inventories.json"); logger.verbose(`writing ${file}`); let content = JSON.stringify(Array.from(playerDatastore)); - await fs.outputFile(file, content); + await libFileOps.safeOutputFile(file, content); } } diff --git a/plugins/research_sync/master.js b/plugins/research_sync/master.js index cec2295e..461c3edc 100644 --- a/plugins/research_sync/master.js +++ b/plugins/research_sync/master.js @@ -2,6 +2,7 @@ const fs = require("fs-extra"); const path = require("path"); +const libFileOps = require("@clusterio/lib/file_ops"); const libPlugin = require("@clusterio/lib/plugin"); const RateLimiter = require("@clusterio/lib/RateLimiter"); @@ -25,7 +26,7 @@ async function loadTechnologies(masterConfig, logger) { async function saveTechnologies(masterConfig, technologies, logger) { let filePath = path.join(masterConfig.get("master.database_directory"), "technologies.json"); logger.verbose(`writing ${filePath}`); - await fs.outputFile(filePath, JSON.stringify([...technologies.entries()], null, 4)); + await libFileOps.safeOutputFile(filePath, JSON.stringify([...technologies.entries()], null, 4)); } class MasterPlugin extends libPlugin.BaseMasterPlugin { diff --git a/plugins/subspace_storage/master.js b/plugins/subspace_storage/master.js index 77a1ee1b..dc8e32d7 100644 --- a/plugins/subspace_storage/master.js +++ b/plugins/subspace_storage/master.js @@ -3,6 +3,7 @@ const fs = require("fs-extra"); const path = require("path"); const libDatabase = require("@clusterio/lib/database"); +const libFileOps = require("@clusterio/lib/file_ops"); const libPlugin = require("@clusterio/lib/plugin"); const { Counter, Gauge } = require("@clusterio/lib/prometheus"); const RateLimiter = require("@clusterio/lib/RateLimiter"); @@ -49,7 +50,7 @@ async function saveDatabase(masterConfig, items, logger) { let file = path.resolve(masterConfig.get("master.database_directory"), "items.json"); logger.verbose(`writing ${file}`); let content = JSON.stringify(items.serialize()); - await fs.outputFile(file, content); + await libFileOps.safeOutputFile(file, content); } else if (items) { logger.error(`Item database too large, not saving (${items.size})`); } diff --git a/test/lib/file_ops.js b/test/lib/file_ops.js index 5b02656d..21823231 100644 --- a/test/lib/file_ops.js +++ b/test/lib/file_ops.js @@ -10,6 +10,8 @@ describe("lib/file_ops", function() { async function setupTestingEnv() { await fs.ensureDir(path.join(baseDir, "test", "folder")); await fs.ensureDir(path.join(baseDir, "test", "another folder")); + await fs.remove(path.join(baseDir, "safe")); + await fs.ensureDir(path.join(baseDir, "safe")); await fs.outputFile(path.join(baseDir, "test", "file.txt"), "contents"); await fs.outputFile(path.join(baseDir, "test", "another file.txt"), "more contents"); @@ -66,4 +68,20 @@ describe("lib/file_ops", function() { } }); }); + + describe("safeOutputFile()", function() { + it("should write new target file", async function() { + let target = path.join(baseDir, "safe", "simple.txt"); + await libFileOps.safeOutputFile(target, "a text file", "utf8"); + assert(!await fs.pathExists(`${target}.tmp`), "temporary was left behind"); + assert.equal(await fs.readFile(target, "utf8"), "a text file"); + }); + it("should overwrite existing target file", async function() { + let target = path.join(baseDir, "safe", "exists.txt"); + await fs.outputFile(target, "previous", "utf8"); + await libFileOps.safeOutputFile(target, "current", "utf8"); + assert(!await fs.pathExists(`${target}.tmp`), "temporary was left behind"); + assert.equal(await fs.readFile(target, "utf8"), "current"); + }); + }); });