From 26100ea56224491a649477bcc4da010342c25217 Mon Sep 17 00:00:00 2001 From: Fedor Indutny <79877362+indutny-signal@users.noreply.github.com> Date: Thu, 3 Mar 2022 14:34:51 -0800 Subject: [PATCH] Make updates atomic again --- ts/test-node/updater/signature_test.ts | 63 +++++++++--- ts/updater/common.ts | 132 +++++++++++++++++-------- ts/updater/differential.ts | 34 +++---- ts/updater/signature.ts | 22 ++--- ts/updater/util.ts | 5 +- 5 files changed, 162 insertions(+), 94 deletions(-) diff --git a/ts/test-node/updater/signature_test.ts b/ts/test-node/updater/signature_test.ts index 26bd83de7..65826e409 100644 --- a/ts/test-node/updater/signature_test.ts +++ b/ts/test-node/updater/signature_test.ts @@ -68,12 +68,21 @@ describe('updater/signatures', () => { const { publicKey, privateKey } = keyPair(); await writeHexToPath(privateKeyPath, privateKey); - await writeSignature(updatePath, version, privateKeyPath); + const signature = await writeSignature( + updatePath, + version, + privateKeyPath + ); const signaturePath = getSignaturePath(updatePath); assert.strictEqual(existsSync(signaturePath), true); - const verified = await verifySignature(updatePath, version, publicKey); + const verified = await verifySignature( + updatePath, + version, + signature, + publicKey + ); assert.strictEqual(verified, true); } finally { if (tempDir) { @@ -99,11 +108,16 @@ describe('updater/signatures', () => { const { publicKey, privateKey } = keyPair(); await writeHexToPath(privateKeyPath, privateKey); - await writeSignature(updatePath, version, privateKeyPath); + const signature = await writeSignature( + updatePath, + version, + privateKeyPath + ); const verified = await verifySignature( updatePath, brokenVersion, + signature, publicKey ); assert.strictEqual(verified, false); @@ -130,14 +144,19 @@ describe('updater/signatures', () => { const { publicKey, privateKey } = keyPair(); await writeHexToPath(privateKeyPath, privateKey); - await writeSignature(updatePath, version, privateKeyPath); - - const signaturePath = getSignaturePath(updatePath); - const signature = Buffer.from(await loadHexFromPath(signaturePath)); + const signature = await writeSignature( + updatePath, + version, + privateKeyPath + ); signature[4] += 3; - await writeHexToPath(signaturePath, signature); - const verified = await verifySignature(updatePath, version, publicKey); + const verified = await verifySignature( + updatePath, + version, + signature, + publicKey + ); assert.strictEqual(verified, false); } finally { if (tempDir) { @@ -162,7 +181,11 @@ describe('updater/signatures', () => { const { publicKey, privateKey } = keyPair(); await writeHexToPath(privateKeyPath, privateKey); - await writeSignature(updatePath, version, privateKeyPath); + const signature = await writeSignature( + updatePath, + version, + privateKeyPath + ); const brokenSourcePath = join( __dirname, @@ -170,7 +193,12 @@ describe('updater/signatures', () => { ); await copy(brokenSourcePath, updatePath); - const verified = await verifySignature(updatePath, version, publicKey); + const verified = await verifySignature( + updatePath, + version, + signature, + publicKey + ); assert.strictEqual(verified, false); } finally { if (tempDir) { @@ -196,9 +224,18 @@ describe('updater/signatures', () => { const { privateKey } = keyPair(); await writeHexToPath(privateKeyPath, privateKey); - await writeSignature(updatePath, version, privateKeyPath); + const signature = await writeSignature( + updatePath, + version, + privateKeyPath + ); - const verified = await verifySignature(updatePath, version, publicKey); + const verified = await verifySignature( + updatePath, + version, + signature, + publicKey + ); assert.strictEqual(verified, false); } finally { if (tempDir) { diff --git a/ts/updater/common.ts b/ts/updater/common.ts index 595043aa8..221c0b42b 100644 --- a/ts/updater/common.ts +++ b/ts/updater/common.ts @@ -2,9 +2,9 @@ // SPDX-License-Identifier: AGPL-3.0-only /* eslint-disable no-console */ -import { createWriteStream, statSync } from 'fs'; +import { createWriteStream } from 'fs'; import { pathExists } from 'fs-extra'; -import { readdir, writeFile } from 'fs/promises'; +import { readdir, rename, stat, writeFile } from 'fs/promises'; import { promisify } from 'util'; import { execFile } from 'child_process'; import { join, normalize, extname } from 'path'; @@ -83,6 +83,11 @@ enum DownloadMode { Automatic = 'Automatic', } +type DownloadUpdateResultType = Readonly<{ + updateFilePath: string; + signature: Buffer; +}>; + export abstract class Updater { protected fileName: string | undefined; @@ -92,6 +97,8 @@ export abstract class Updater { private throttledSendDownloadingUpdate: (downloadedSize: number) => void; + private activeDownload: Promise | undefined; + constructor( protected readonly logger: LoggerType, private readonly settingsChannel: SettingsChannel, @@ -156,6 +163,23 @@ export abstract class Updater { private async downloadAndInstall( updateInfo: UpdateInformationType, mode: DownloadMode + ): Promise { + if (this.activeDownload) { + return this.activeDownload; + } + + try { + this.activeDownload = this.doDownloadAndInstall(updateInfo, mode); + + return await this.activeDownload; + } finally { + this.activeDownload = undefined; + } + } + + private async doDownloadAndInstall( + updateInfo: UpdateInformationType, + mode: DownloadMode ): Promise { const { logger } = this; @@ -163,19 +187,20 @@ export abstract class Updater { try { const oldVersion = this.version; - this.version = newVersion; - let updateFilePath: string | undefined; + let downloadResult: DownloadUpdateResultType | undefined; + try { - updateFilePath = await this.downloadUpdate(updateInfo, mode); + downloadResult = await this.downloadUpdate(updateInfo, mode); } catch (error) { // Restore state in case of download error this.version = oldVersion; + throw error; } - if (!updateFilePath) { + if (!downloadResult) { logger.warn('downloadAndInstall: no update was downloaded'); strictAssert( mode !== DownloadMode.Automatic && mode !== DownloadMode.FullOnly, @@ -184,10 +209,13 @@ export abstract class Updater { return false; } + const { updateFilePath, signature } = downloadResult; + const publicKey = hexToBinary(config.get('updatesPublicKey')); const verified = await verifySignature( updateFilePath, this.version, + signature, publicKey ); if (!verified) { @@ -403,7 +431,7 @@ export abstract class Updater { private async downloadUpdate( { fileName, sha512, differentialData }: UpdateInformationType, mode: DownloadMode - ): Promise { + ): Promise { const baseUrl = getUpdatesBase(); const updateFileUrl = `${baseUrl}/${fileName}`; @@ -414,43 +442,37 @@ export abstract class Updater { const signatureUrl = `${baseUrl}/${signatureFileName}`; const blockMapUrl = `${baseUrl}/${blockMapFileName}`; - const cacheDir = await createUpdateCacheDirIfNeeded(); + let cacheDir = await createUpdateCacheDirIfNeeded(); const targetUpdatePath = join(cacheDir, fileName); - const targetSignaturePath = join(cacheDir, signatureFileName); - const targetBlockMapPath = join(cacheDir, blockMapFileName); - const targetPaths = [ - targetUpdatePath, - targetSignaturePath, - targetBlockMapPath, - ]; + const tempDir = await createTempDir(); + const restoreDir = await createTempDir(); - // List of files to be deleted on success - const oldFiles = (await readdir(cacheDir)) - .map(oldFileName => { - return join(cacheDir, oldFileName); - }) - .filter(path => !targetPaths.includes(path)); + const tempUpdatePath = join(tempDir, fileName); + const tempBlockMapPath = join(tempDir, blockMapFileName); try { validatePath(cacheDir, targetUpdatePath); - validatePath(cacheDir, targetSignaturePath); - validatePath(cacheDir, targetBlockMapPath); + + validatePath(tempDir, tempUpdatePath); + validatePath(tempDir, tempBlockMapPath); this.logger.info(`downloadUpdate: Downloading signature ${signatureUrl}`); - const signature = await got(signatureUrl, getGotOptions()).buffer(); - await writeFile(targetSignaturePath, signature); + const signature = Buffer.from( + await got(signatureUrl, getGotOptions()).text(), + 'hex' + ); if (differentialData) { this.logger.info(`downloadUpdate: Saving blockmap ${blockMapUrl}`); - await writeFile(targetBlockMapPath, differentialData.newBlockMap); + await writeFile(tempBlockMapPath, differentialData.newBlockMap); } else { try { this.logger.info( `downloadUpdate: Downloading blockmap ${blockMapUrl}` ); const blockMap = await got(blockMapUrl, getGotOptions()).buffer(); - await writeFile(targetBlockMapPath, blockMap); + await writeFile(tempBlockMapPath, blockMap); } catch (error) { this.logger.warn( 'downloadUpdate: Failed to download blockmap, continuing', @@ -467,7 +489,17 @@ export abstract class Updater { `downloadUpdate: Not downloading update ${updateFileUrl}, ` + 'local file has the same hash' ); - gotUpdate = true; + + // Move file into downloads directory + try { + await rename(targetUpdatePath, tempUpdatePath); + gotUpdate = true; + } catch (error) { + this.logger.error( + 'downloadUpdate: failed to move already downloaded file', + Errors.toLogFormat(error) + ); + } } else { this.logger.error( 'downloadUpdate: integrity check failure', @@ -484,7 +516,7 @@ export abstract class Updater { ); try { - await downloadDifferentialData(targetUpdatePath, differentialData, { + await downloadDifferentialData(tempUpdatePath, differentialData, { statusCallback: updateOnProgress ? this.throttledSendDownloadingUpdate : undefined, @@ -505,9 +537,16 @@ export abstract class Updater { this.logger.info( `downloadUpdate: Downloading full update ${updateFileUrl}` ); + + // We could have failed to update differentially due to low free disk + // space. Remove all cached updates since we are doing a full download + // anyway. + await rimrafPromise(cacheDir); + cacheDir = await createUpdateCacheDirIfNeeded(); + await this.downloadAndReport( updateFileUrl, - targetUpdatePath, + tempUpdatePath, updateOnProgress ); gotUpdate = true; @@ -517,17 +556,22 @@ export abstract class Updater { return undefined; } - // Now that we successfully downloaded an update - remove old files - await Promise.all(oldFiles.map(path => rimrafPromise(path))); + // Backup old files + await rename(cacheDir, restoreDir); - return targetUpdatePath; - } catch (error) { + // Move the files into the final position try { - await Promise.all([targetPaths.map(path => rimrafPromise(path))]); - } catch (_) { - // Ignore error, this is a cleanup + await rename(tempDir, cacheDir); + } catch (error) { + // Attempt to restore old files + await rename(restoreDir, cacheDir); + + throw error; } - throw error; + + return { updateFilePath: targetUpdatePath, signature }; + } finally { + await Promise.all([deleteTempDir(tempDir), deleteTempDir(restoreDir)]); } } @@ -758,11 +802,13 @@ export async function createUpdateCacheDirIfNeeded(): Promise { } export async function deleteTempDir(targetDir: string): Promise { - const pathInfo = statSync(targetDir); - if (!pathInfo.isDirectory()) { - throw new Error( - `deleteTempDir: Cannot delete path '${targetDir}' because it is not a directory` - ); + if (await pathExists(targetDir)) { + const pathInfo = await stat(targetDir); + if (!pathInfo.isDirectory()) { + throw new Error( + `deleteTempDir: Cannot delete path '${targetDir}' because it is not a directory` + ); + } } const baseTempDir = getBaseTempDir(); diff --git a/ts/updater/differential.ts b/ts/updater/differential.ts index 7f9b9b87e..85794d365 100644 --- a/ts/updater/differential.ts +++ b/ts/updater/differential.ts @@ -2,11 +2,9 @@ // SPDX-License-Identifier: AGPL-3.0-only import type { FileHandle } from 'fs/promises'; -import { readFile, open, mkdtemp, mkdir, rename, unlink } from 'fs/promises'; +import { readFile, open } from 'fs/promises'; import { promisify } from 'util'; import { gunzip as nativeGunzip } from 'zlib'; -import { tmpdir } from 'os'; -import path from 'path'; import got from 'got'; import { chunk as lodashChunk } from 'lodash'; import pMap from 'p-map'; @@ -254,12 +252,10 @@ export async function download( { statusCallback, logger, gotOptions }: DownloadOptionsType = {} ): Promise { const input = await open(oldFile, 'r'); + const output = await open(newFile, 'w'); - const tempDir = await mkdtemp(path.join(tmpdir(), 'signal-temp-')); - await mkdir(tempDir, { recursive: true }); - const tempFile = path.join(tempDir, path.basename(newFile)); - - const output = await open(tempFile, 'w'); + const abortController = new AbortController(); + const { signal: abortSignal } = abortController; const copyActions = diff.filter(({ action }) => action === 'copy'); @@ -278,15 +274,16 @@ export async function download( `Not enough data to read from offset=${readOffset} size=${size}` ); + if (abortSignal?.aborted) { + return; + } + await output.write(chunk, 0, chunk.length, writeOffset); }) ); const downloadActions = diff.filter(({ action }) => action === 'download'); - const abortController = new AbortController(); - const { signal: abortSignal } = abortController; - try { let downloadedSize = 0; @@ -314,16 +311,8 @@ export async function download( await Promise.all([input.close(), output.close()]); } - const checkResult = await checkIntegrity(tempFile, sha512); + const checkResult = await checkIntegrity(newFile, sha512); strictAssert(checkResult.ok, checkResult.error ?? ''); - - // Finally move the file into its final location - try { - await unlink(newFile); - } catch (_) { - // ignore errors - } - await rename(tempFile, newFile); } export async function downloadRanges( @@ -389,6 +378,11 @@ export async function downloadRanges( `newChunk=${chunk.length} ` + `maxSize=${diff.size}` ); + + if (abortSignal?.aborted) { + return; + } + await output.write(chunk, 0, chunk.length, offset + diff.writeOffset); offset += chunk.length; diff --git a/ts/updater/signature.ts b/ts/updater/signature.ts index bd22574e9..d752e9f03 100644 --- a/ts/updater/signature.ts +++ b/ts/updater/signature.ts @@ -7,6 +7,7 @@ import { readFile as readFileCallback, writeFile as writeFileCallback, } from 'fs'; +import { pipeline } from 'stream/promises'; import { basename, dirname, join, resolve as resolvePath } from 'path'; import pify from 'pify'; @@ -30,10 +31,9 @@ export async function generateSignature( export async function verifySignature( updatePackagePath: string, version: string, + signature: Buffer, publicKey: Buffer ): Promise { - const signaturePath = getSignaturePath(updatePackagePath); - const signature = await loadHexFromPath(signaturePath); const message = await generateMessage(updatePackagePath, version); return verify(publicKey, message, signature); @@ -55,7 +55,7 @@ export async function writeSignature( updatePackagePath: string, version: string, privateKeyPath: string -): Promise { +): Promise { const signaturePath = getSignaturePath(updatePackagePath); const signature = await generateSignature( updatePackagePath, @@ -63,23 +63,15 @@ export async function writeSignature( privateKeyPath ); await writeHexToPath(signaturePath, signature); + + return signature; } export async function _getFileHash(updatePackagePath: string): Promise { const hash = createHash('sha256'); - const stream = createReadStream(updatePackagePath); + await pipeline(createReadStream(updatePackagePath), hash); - return new Promise((resolve, reject) => { - stream.on('data', data => { - hash.update(data); - }); - stream.on('close', () => { - resolve(hash.digest()); - }); - stream.on('error', error => { - reject(error); - }); - }); + return hash.digest(); } export function getSignatureFileName(fileName: string): string { diff --git a/ts/updater/util.ts b/ts/updater/util.ts index bb1e786d9..b1b038e34 100644 --- a/ts/updater/util.ts +++ b/ts/updater/util.ts @@ -2,6 +2,7 @@ // SPDX-License-Identifier: AGPL-3.0-only import { createReadStream } from 'fs'; +import { pipeline } from 'stream/promises'; import { createHash } from 'crypto'; import * as Errors from '../types/errors'; @@ -23,9 +24,7 @@ export async function checkIntegrity( ): Promise { try { const hash = createHash('sha512'); - for await (const chunk of createReadStream(fileName)) { - hash.update(chunk); - } + await pipeline(createReadStream(fileName), hash); const actualSHA512 = hash.digest('base64'); if (sha512 === actualSHA512) {