From ff7e462c4d228f4db942de2d0ab1d7889c0a6c22 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6ren=20Beye?= Date: Sat, 4 Oct 2025 16:32:52 +0200 Subject: [PATCH] fix(updater): Pass around a file descriptor to/and make things safer --- .../ValetudoUpdaterApplyPendingState.js | 2 + backend/lib/updater/Updater.js | 20 ++++++- .../lib/steps/ValetudoUpdaterApplyStep.js | 49 ++++++++++++++-- .../lib/steps/ValetudoUpdaterDownloadStep.js | 57 +++++++++++++++++-- 4 files changed, 115 insertions(+), 13 deletions(-) diff --git a/backend/lib/entities/core/updater/ValetudoUpdaterApplyPendingState.js b/backend/lib/entities/core/updater/ValetudoUpdaterApplyPendingState.js index a23a9e60..e715f82d 100644 --- a/backend/lib/entities/core/updater/ValetudoUpdaterApplyPendingState.js +++ b/backend/lib/entities/core/updater/ValetudoUpdaterApplyPendingState.js @@ -9,6 +9,7 @@ class ValetudoUpdaterApplyPendingState extends ValetudoUpdaterState { * @param {string} options.version The version (e.g. 2021.10.0) * @param {Date} options.releaseTimestamp The release date as found in the manifest * @param {string} options.downloadPath The path the new binary was downloaded to + * @param {number} options.downloadPathFd file descriptor * * @class */ @@ -18,6 +19,7 @@ class ValetudoUpdaterApplyPendingState extends ValetudoUpdaterState { this.version = options.version; this.releaseTimestamp = options.releaseTimestamp; this.downloadPath = options.downloadPath; + this.downloadPathFd = options.downloadPathFd; } } diff --git a/backend/lib/updater/Updater.js b/backend/lib/updater/Updater.js index d9209e5b..ca380a4e 100644 --- a/backend/lib/updater/Updater.js +++ b/backend/lib/updater/Updater.js @@ -110,7 +110,6 @@ class Updater { if (!(this.state instanceof States.ValetudoUpdaterApprovalPendingState)) { throw new Error("Downloads can only be started when there's pending approval"); } - const downloadPath = this.state.downloadPath; this.state.busy = true; const step = new Steps.ValetudoUpdaterDownloadStep({ @@ -138,8 +137,22 @@ class Updater { step.execute().then((state) => { this.state = state; + const downloadPath = state.downloadPath; + const downloadPathFd = state.downloadPathFd; + this.cleanupHandler = () => { - fs.unlinkSync(downloadPath); + try { + fs.closeSync(downloadPathFd); + } catch (e) { + /* intentional */ + } + + try { + fs.unlinkSync(downloadPath); + } catch (e) { + /* intentional */ + } + this.state = new States.ValetudoUpdaterIdleState({ currentVersion: this.updateProvider.getCurrentVersion() @@ -172,7 +185,8 @@ class Updater { this.state.busy = true; const step = new Steps.ValetudoUpdaterApplyStep({ - downloadPath: this.state.downloadPath + downloadPath: this.state.downloadPath, + downloadPathFd: this.state.downloadPathFd }); step.execute().catch(err => { //no .then() required as the system will reboot diff --git a/backend/lib/updater/lib/steps/ValetudoUpdaterApplyStep.js b/backend/lib/updater/lib/steps/ValetudoUpdaterApplyStep.js index abf853bf..20c89fa3 100644 --- a/backend/lib/updater/lib/steps/ValetudoUpdaterApplyStep.js +++ b/backend/lib/updater/lib/steps/ValetudoUpdaterApplyStep.js @@ -1,27 +1,64 @@ const fs = require("fs"); +const Logger = require("../../../Logger"); +const path = require("path"); const ValetudoUpdaterStep = require("./ValetudoUpdaterStep"); +const {pipeline} = require("stream/promises"); const {spawnSync} = require("child_process"); class ValetudoUpdaterApplyStep extends ValetudoUpdaterStep { /** * @param {object} options * @param {string} options.downloadPath + * @param {number} options.downloadPathFd */ constructor(options) { super(); this.downloadPath = options.downloadPath; + this.downloadPathFd = options.downloadPathFd; } // @ts-ignore - Because spawnSync("reboot") ends our process, we don't have to return anything async execute() { - fs.unlinkSync(process.argv0); - fs.copyFileSync(this.downloadPath, process.argv0); - fs.chmodSync(process.argv0, fs.constants.S_IXUSR | fs.constants.S_IXGRP | fs.constants.S_IXOTH); - fs.unlinkSync(this.downloadPath); + const destinationDir = path.dirname(process.argv0); + const destinationFilename = path.basename(process.argv0); + const tmpDestination = path.join(destinationDir, `${destinationFilename}.upd`); - spawnSync("sync"); - spawnSync("reboot"); + try { + const downloadedUpdateReadStream = fs.createReadStream(null, { + fd: this.downloadPathFd, + start: 0, + autoClose: false + }); + const tmpWriteStream = fs.createWriteStream(tmpDestination, { + flush: true, + mode: fs.constants.S_IXUSR | fs.constants.S_IXGRP | fs.constants.S_IXOTH + }); + + await pipeline(downloadedUpdateReadStream, tmpWriteStream); + + fs.closeSync(this.downloadPathFd); + + try { + fs.unlinkSync(this.downloadPath); + } catch (e) { + // intentionally ignored as this is what happens when the file got deleted mid-update + // we still have the fd and the downloadLocation is usually a ramdisk anyway, so we do not really care + } + + fs.renameSync(tmpDestination, process.argv0); + + spawnSync("sync"); + spawnSync("reboot"); + } catch (err) { + try { + fs.unlinkSync(tmpDestination); + } catch (e) { + Logger.warn(`Error while deleting tmp file ${tmpDestination}`, e); + } + + throw err; + } } } diff --git a/backend/lib/updater/lib/steps/ValetudoUpdaterDownloadStep.js b/backend/lib/updater/lib/steps/ValetudoUpdaterDownloadStep.js index 91587e6a..998a141b 100644 --- a/backend/lib/updater/lib/steps/ValetudoUpdaterDownloadStep.js +++ b/backend/lib/updater/lib/steps/ValetudoUpdaterDownloadStep.js @@ -31,8 +31,12 @@ class ValetudoUpdaterDownloadStep extends ValetudoUpdaterStep { this.onProgressUpdate = () => {}; } - + /** + * @returns {Promise} + */ async execute() { + let fd; + try { const downloadResponse = await get(this.downloadUrl, {responseType: "stream"}); const expectedDownloadSize = parseInt(downloadResponse.headers?.["content-length"]); @@ -50,14 +54,30 @@ class ValetudoUpdaterDownloadStep extends ValetudoUpdaterStep { } }); + fd = fs.openSync(this.downloadPath, "w+"); + await pipeline( downloadResponse.data, progressTracker, - fs.createWriteStream(this.downloadPath) + fs.createWriteStream(null, {fd: fd, flush: true, autoClose: false}) ); } catch (e) { Logger.error("Error while downloading release binary", e); + if (fd !== undefined) { + try { + fs.closeSync(fd); + } catch (e) { + /* intentional */ + } + + try { + fs.unlinkSync(this.downloadPath); + } catch (e) { + /* intentional */ + } + } + throw new ValetudoUpdaterError( ValetudoUpdaterError.ERROR_TYPE.DOWNLOAD_FAILED, "Error while downloading release binary" @@ -69,7 +89,7 @@ class ValetudoUpdaterDownloadStep extends ValetudoUpdaterStep { try { checksum = await new Promise((resolve, reject) => { const hash = crypto.createHash("sha256"); - const readStream = fs.createReadStream(this.downloadPath); + const readStream = fs.createReadStream(null, {fd: fd, start: 0, autoClose: false}); readStream.on("error", err => { reject(err); @@ -86,6 +106,20 @@ class ValetudoUpdaterDownloadStep extends ValetudoUpdaterStep { } catch (e) { Logger.error("Error while calculating downloaded binary checksum", e); + if (fd !== undefined) { + try { + fs.closeSync(fd); + } catch (e) { + /* intentional */ + } + + try { + fs.unlinkSync(this.downloadPath); + } catch (e) { + /* intentional */ + } + } + throw new ValetudoUpdaterError( ValetudoUpdaterError.ERROR_TYPE.UNKNOWN, "Error while calculating downloaded binary checksum" @@ -93,6 +127,20 @@ class ValetudoUpdaterDownloadStep extends ValetudoUpdaterStep { } if (checksum !== this.expectedHash) { + if (fd !== undefined) { + try { + fs.closeSync(fd); + } catch (e) { + /* intentional */ + } + + try { + fs.unlinkSync(this.downloadPath); + } catch (e) { + /* intentional */ + } + } + throw new ValetudoUpdaterError( ValetudoUpdaterError.ERROR_TYPE.INVALID_CHECKSUM, `Expected Checksum: ${this.expectedHash}. Actual: ${checksum}` @@ -101,7 +149,8 @@ class ValetudoUpdaterDownloadStep extends ValetudoUpdaterStep { return new States.ValetudoUpdaterApplyPendingState({ version: this.version, releaseTimestamp: this.releaseTimestamp, - downloadPath: this.downloadPath + downloadPath: this.downloadPath, + downloadPathFd: fd }); } }