Make updates atomic again

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 signature = await writeSignature(
-
+        updatePath,
-      const signaturePath = getSignaturePath(updatePath);
+        version,
-      const signature = Buffer.from(await loadHexFromPath(signaturePath));
+        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) {

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<boolean> | 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<boolean> {
+    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<boolean> {
     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<string | undefined> {
+  ): Promise<DownloadUpdateResultType | undefined> {
     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 = [
+    const tempDir = await createTempDir();
-      targetUpdatePath,
+    const restoreDir = await createTempDir();
-      targetSignaturePath,
-      targetBlockMapPath,
-    ];
 
-    // List of files to be deleted on success
+    const tempUpdatePath = join(tempDir, fileName);
-    const oldFiles = (await readdir(cacheDir))
+    const tempBlockMapPath = join(tempDir, blockMapFileName);
-      .map(oldFileName => {
-        return join(cacheDir, oldFileName);
-      })
-      .filter(path => !targetPaths.includes(path));
 
     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();
+      const signature = Buffer.from(
-      await writeFile(targetSignaturePath, signature);
+        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
+      // Backup old files
-      await Promise.all(oldFiles.map(path => rimrafPromise(path)));
+      await rename(cacheDir, restoreDir);
 
-      return targetUpdatePath;
+      // Move the files into the final position
-    } catch (error) {
       try {
-        await Promise.all([targetPaths.map(path => rimrafPromise(path))]);
+        await rename(tempDir, cacheDir);
-      } catch (_) {
+      } catch (error) {
-        // Ignore error, this is a cleanup
+        // 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<string> {
 }
 
 export async function deleteTempDir(targetDir: string): Promise<void> {
-  const pathInfo = statSync(targetDir);
+  if (await pathExists(targetDir)) {
-  if (!pathInfo.isDirectory()) {
+    const pathInfo = await stat(targetDir);
-    throw new Error(
+    if (!pathInfo.isDirectory()) {
-      `deleteTempDir: Cannot delete path '${targetDir}' because it is not a directory`
+      throw new Error(
-    );
+        `deleteTempDir: Cannot delete path '${targetDir}' because it is not a directory`
+      );
+    }
   }
 
   const baseTempDir = getBaseTempDir();

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<void> {
   const input = await open(oldFile, 'r');
+  const output = await open(newFile, 'w');
 
-  const tempDir = await mkdtemp(path.join(tmpdir(), 'signal-temp-'));
+  const abortController = new AbortController();
-  await mkdir(tempDir, { recursive: true });
+  const { signal: abortSignal } = abortController;
-  const tempFile = path.join(tempDir, path.basename(newFile));
-
-  const output = await open(tempFile, 'w');
 
   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;
 

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<boolean> {
-  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<void> {
+): Promise<Buffer> {
   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<Buffer> {
   const hash = createHash('sha256');
-  const stream = createReadStream(updatePackagePath);
+  await pipeline(createReadStream(updatePackagePath), hash);
 
-  return new Promise((resolve, reject) => {
+  return hash.digest();
-    stream.on('data', data => {
-      hash.update(data);
-    });
-    stream.on('close', () => {
-      resolve(hash.digest());
-    });
-    stream.on('error', error => {
-      reject(error);
-    });
-  });
 }
 
 export function getSignatureFileName(fileName: string): string {

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<CheckIntegrityResultType> {
   try {
     const hash = createHash('sha512');
-    for await (const chunk of createReadStream(fileName)) {
+    await pipeline(createReadStream(fileName), hash);
-      hash.update(chunk);
-    }
 
     const actualSHA512 = hash.digest('base64');
     if (sha512 === actualSHA512) {