diff --git a/src/main/runtime/rpc/server/pairing/device-registry-disk-merge.ts b/src/main/runtime/rpc/server/pairing/device-registry-disk-merge.ts index 450bd6b0a18..d368e6453c0 100644 --- a/src/main/runtime/rpc/server/pairing/device-registry-disk-merge.ts +++ b/src/main/runtime/rpc/server/pairing/device-registry-disk-merge.ts @@ -1,4 +1,4 @@ -import { statSync } from 'node:fs' +import { readFileSync, statSync } from 'node:fs' import { writeDurableSecureJsonFile, writeSecureJsonFile } from '../../../../../shared/secure-file' import type { DeviceEntry } from './device-entry' import { isMissingFileError, quarantineCorruptRegistry } from './device-registry-corruption' @@ -6,6 +6,10 @@ import { readDeviceRegistryFile, type DeviceRegistryFileRead } from './device-re // Phones recheck auth on every frame and request; one stat a second bounds a disk revoke's lag. const EXTERNAL_EDIT_CHECK_INTERVAL_MS = 1000 +// Another writer landed between our read and rename: re-read and re-merge, a few times at most. +const MAX_WRITE_ATTEMPTS = 5 +// Non-atomic external writers (an admin's script) can be caught mid-write; give them a moment. +const TORN_READ_RETRY_MS = 25 /** * What a save may write over the file as it is now on disk. Another writer (an admin removing a @@ -102,7 +106,12 @@ export class DeviceRegistryWriter { : mergeDeviceRegistrySave(current, current, disk, this.removedIds) } - /** Returns what was written. Security transitions pass `durable`. */ + /** + * Returns what was written. Security transitions pass `durable`. The merge is published only if + * the file is still exactly what it was read from (compare-and-swap); otherwise another writer + * landed in between, so the save re-reads and re-merges. Without that, a save that read the file + * before an admin's revoke and renamed after it would put the revoked row back. + */ write( before: readonly DeviceEntry[], next: readonly DeviceEntry[], @@ -114,23 +123,56 @@ export class DeviceRegistryWriter { this.removedIds.add(deviceId) } } - const disk = readDeviceRegistryFile(this.registryPath) - // After our own quarantine the file's absence is no removal evidence, but rows another writer - // (the pairing CLI) added since are still kept: merge with no known baseline. - const baseline = this.quarantinedUntilWritten ? [] : before - const merged = - disk.unreadable || disk.corruptFilePending - ? [...next] - : mergeDeviceRegistrySave(baseline, next, disk, this.removedIds) - const write = options.durable ? writeDurableSecureJsonFile : writeSecureJsonFile // Stat after our rename can't tell our write from one right after it; re-read once instead. this.readStamp = null - write(this.registryPath, merged) - this.quarantinedUntilWritten = false - return merged + for (let attempt = 1; ; attempt++) { + const lastAttempt = attempt >= MAX_WRITE_ATTEMPTS + const snapshot = readRawRegistry(this.registryPath) + const disk = readDeviceRegistryFile(this.registryPath) + if ( + disk.corruptFilePending && + !lastAttempt && + snapshot !== readRawRegistry(this.registryPath) + ) { + // Still being written by someone else; a torn read proves nothing, so wait and re-read. + sleepSync(TORN_READ_RETRY_MS) + continue + } + // After our own quarantine the file's absence is no removal evidence, but rows another writer + // (the pairing CLI) added since are still kept: merge with no known baseline. + const baseline = this.quarantinedUntilWritten ? [] : before + const merged = + disk.unreadable || disk.corruptFilePending + ? [...next] + : mergeDeviceRegistrySave(baseline, next, disk, this.removedIds) + let unchanged = true + const write = options.durable ? writeDurableSecureJsonFile : writeSecureJsonFile + write(this.registryPath, merged, { + // Last check before the rename; a lost race discards the stage and merges again. + shouldPublish: () => + (unchanged = lastAttempt || readRawRegistry(this.registryPath) === snapshot) + }) + if (unchanged) { + this.quarantinedUntilWritten = false + return merged + } + } } } +/** Raw file text for compare-and-swap; null when absent, and a marker no file can equal when unreadable. */ +function readRawRegistry(registryPath: string): string | null { + try { + return readFileSync(registryPath, 'utf-8') + } catch (error) { + return isMissingFileError(error) ? null : `\u0000unreadable:${String(error)}` + } +} + +function sleepSync(ms: number): void { + Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, ms) +} + function fileStamp(registryPath: string): string { try { const stat = statSync(registryPath, { bigint: true }) diff --git a/src/main/runtime/rpc/server/pairing/device-registry-save-race.test.ts b/src/main/runtime/rpc/server/pairing/device-registry-save-race.test.ts new file mode 100644 index 00000000000..61a88ea1ce7 --- /dev/null +++ b/src/main/runtime/rpc/server/pairing/device-registry-save-race.test.ts @@ -0,0 +1,103 @@ +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type * as FileReadModule from './device-registry-file-read' +import { DeviceRegistry } from './device-registry' +import { DEVICE_REGISTRY_FILENAME } from './mobile-pairing-files' + +// Runs once right after the saving process has read the file: another process's write lands then. +const afterRead = vi.hoisted(() => { + const state: { hook: (() => void) | null } = { hook: null } + return state +}) +vi.mock('./device-registry-file-read', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + readDeviceRegistryFile: (path: string) => { + const read = actual.readDeviceRegistryFile(path) + const hook = afterRead.hook + afterRead.hook = null + hook?.() + return read + } + } +}) + +let dir: string +let registryPath: string + +beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), 'orca-registry-race-')) + registryPath = join(dir, DEVICE_REGISTRY_FILENAME) +}) + +afterEach(() => { + afterRead.hook = null + rmSync(dir, { recursive: true, force: true }) +}) + +function diskIds(): string[] { + const rows: { deviceId: string }[] = JSON.parse(readFileSync(registryPath, 'utf8')) + return rows.map((row) => row.deviceId) +} + +/** An admin's script, as in the docs: rewrite the file without the row (not atomic, no lock). */ +function adminRemoves(deviceId: string): void { + const rows: { deviceId: string }[] = JSON.parse(readFileSync(registryPath, 'utf8')) + writeFileSync(registryPath, JSON.stringify(rows.filter((row) => row.deviceId !== deviceId))) +} + +function pairedDevices(registry: DeviceRegistry) { + const kept = registry.addDevice('Kept', 'runtime') + const revoked = registry.addDevice('Revoked', 'runtime') + registry.updateLastSeen(kept.deviceId) + registry.updateLastSeen(revoked.deviceId) + return { kept, revoked } +} + +describe('device registry save racing another writer', () => { + it('a lastSeen flush that read before an admin revoke does not write the row back', () => { + const registry = new DeviceRegistry(dir) + const { kept, revoked } = pairedDevices(registry) + registry.updateLastSeenDeferred(revoked.deviceId) + + afterRead.hook = () => adminRemoves(revoked.deviceId) + registry.flushPendingLastSeen() + + expect(diskIds()).toEqual([kept.deviceId]) + expect(registry.validateToken(revoked.token)).toBeNull() + expect(new DeviceRegistry(dir).validateToken(revoked.token)).toBeNull() + expect(registry.validateToken(kept.token)?.deviceId).toBe(kept.deviceId) + }) + + it('keeps both writers changes: a row another process added during the save survives', () => { + const registry = new DeviceRegistry(dir) + const { kept } = pairedDevices(registry) + const other = new DeviceRegistry(dir) + + afterRead.hook = () => other.addDevice('From the CLI', 'runtime') + const added = registry.addDevice('From the desktop', 'runtime') + + expect(new Set(diskIds())).toEqual( + new Set([kept.deviceId, added.deviceId, ...other.listDevices().map((d) => d.deviceId)]) + ) + expect(diskIds()).toHaveLength(4) + }) + + it('waits out an admin write caught half-way instead of treating it as a corrupt file', () => { + const registry = new DeviceRegistry(dir) + const { kept, revoked } = pairedDevices(registry) + registry.updateLastSeenDeferred(revoked.deviceId) + const full: { deviceId: string }[] = JSON.parse(readFileSync(registryPath, 'utf8')) + const finished = JSON.stringify(full.filter((row) => row.deviceId !== revoked.deviceId)) + + // The save reads a truncated file; the admin's write finishes right after. + writeFileSync(registryPath, finished.slice(0, 10)) + afterRead.hook = () => writeFileSync(registryPath, finished) + registry.flushPendingLastSeen() + + expect(diskIds()).toEqual([kept.deviceId]) + }) +}) diff --git a/src/shared/secure-file.ts b/src/shared/secure-file.ts index 756ecbf442d..af4c5a7e65c 100644 --- a/src/shared/secure-file.ts +++ b/src/shared/secure-file.ts @@ -98,13 +98,21 @@ function hardenExistingWindowsFile(targetPath: string): boolean { } /** Returns false when the file was written but its permissions could not be restricted. */ -export function writeSecureJsonFile(targetPath: string, value: unknown): boolean { - return writeSecureFile(targetPath, JSON.stringify(value, null, 2)) +export function writeSecureJsonFile( + targetPath: string, + value: unknown, + options: { shouldPublish?: () => boolean } = {} +): boolean { + return writeSecureFile(targetPath, JSON.stringify(value, null, 2), options) } /** Returns false when the file was written but its permissions could not be restricted. */ -export function writeDurableSecureJsonFile(targetPath: string, value: unknown): boolean { - return writeSecureFile(targetPath, JSON.stringify(value, null, 2), { durable: true }) +export function writeDurableSecureJsonFile( + targetPath: string, + value: unknown, + options: { shouldPublish?: () => boolean } = {} +): boolean { + return writeSecureFile(targetPath, JSON.stringify(value, null, 2), { ...options, durable: true }) } /** Publish a staged file and return whether user-only file permissions hold. Windows addon creates restricted or throws;