From f0adb7695eb7e43a6460fe8252784c6c1338b082 Mon Sep 17 00:00:00 2001 From: Jeff See Date: Thu, 22 Oct 2026 15:22:53 -0700 Subject: [PATCH] fix: publish native addon cache files atomically Stage extracted files in exclusively created sibling temporary files before renaming them into the shared native-addon cache. Preserve mapped destination files, clean up failed writes and renames, and only accept an identical concurrent winner. Add deterministic regression tests for package and standalone addon extraction, descriptor cleanup, rename races, temporary-file ownership, and long filenames. --- prelude/bootstrap-shared.js | 54 ++++- test/unit/native-addon-cache.test.ts | 312 +++++++++++++++++++++++++++ 2 files changed, 362 insertions(+), 4 deletions(-) create mode 100644 test/unit/native-addon-cache.test.ts diff --git a/prelude/bootstrap-shared.js b/prelude/bootstrap-shared.js index 044b490ef..3cf65df12 100644 --- a/prelude/bootstrap-shared.js +++ b/prelude/bootstrap-shared.js @@ -8,7 +8,7 @@ // SEA bootstrap: bundled by esbuild via require('./bootstrap-shared'). var childProcess = require('child_process'); -var { createHash } = require('crypto'); +var { createHash, randomBytes } = require('crypto'); var fs = require('fs'); var path = require('path'); var zlib = require('zlib'); @@ -85,6 +85,52 @@ function pickDecompressorAsync(compression) { // NATIVE ADDON EXTRACTION ///////////////////////////////////////// // ///////////////////////////////////////////////////////////////// +// Publish a complete file without truncating a cached addon that another +// process may already have mapped into memory. The sibling temp file keeps +// rename on the same filesystem; randomness also separates worker threads +// that share a process.pid. +function writeFileAtomic(dest, content) { + // Do not extend the destination basename: it may already be near NAME_MAX. + var temporary = path.join( + path.dirname(dest), + '.pkg-' + process.pid + '-' + randomBytes(6).toString('hex') + '.tmp', + ); + // Only enter cleanup once we own the file. An EEXIST must not remove + // another writer's temporary file. + var fd = fs.openSync(temporary, 'wx'); + var renamed = false; + try { + try { + fs.writeFileSync(fd, content); + } finally { + fs.closeSync(fd); + } + try { + fs.renameSync(temporary, dest); + renamed = true; + } catch (error) { + // Windows can refuse to replace an addon another process has loaded. + // Accept a concurrent winner only if it published the exact same bytes. + var identical = false; + try { + identical = fs.readFileSync(dest).equals(content); + } catch (_) { + // Keep the original rename error if the destination cannot be read. + } + if (!identical) throw error; + } + } finally { + // Also remove partial files left by a failed write, not just failed renames. + if (!renamed) { + try { + fs.unlinkSync(temporary); + } catch (_) { + // Best-effort cleanup must not hide the original write or rename error. + } + } + } +} + // Recursively copy src -> dest. For existing destination files, compare // SHA-256 hashes and skip identical ones to avoid redundant writes. // @@ -152,7 +198,7 @@ function cpRecursive(src, dest) { return; } } - fs.writeFileSync(dest, srcContent); + writeFileAtomic(dest, srcContent); } /** @@ -211,10 +257,10 @@ function patchDlopen(insideSnapshot) { var dContent = fs.readFileSync(tmpModulePath); var dHash = createHash('sha256').update(dContent).digest('hex'); if (hash !== dHash) { - fs.writeFileSync(tmpModulePath, moduleContent); + writeFileAtomic(tmpModulePath, moduleContent); } } else { - fs.writeFileSync(tmpModulePath, moduleContent); + writeFileAtomic(tmpModulePath, moduleContent); } newPath = tmpModulePath; diff --git a/test/unit/native-addon-cache.test.ts b/test/unit/native-addon-cache.test.ts new file mode 100644 index 000000000..b0b5d334d --- /dev/null +++ b/test/unit/native-addon-cache.test.ts @@ -0,0 +1,312 @@ +import assert from 'node:assert/strict'; +import { createHash } from 'node:crypto'; +import fs from 'node:fs'; +import { createRequire } from 'node:module'; +import os from 'node:os'; +import path from 'node:path'; +import { describe, it } from 'node:test'; +import type { TestContext } from 'node:test'; +import vm from 'node:vm'; + +const sharedPath = path.join(__dirname, '../../prelude/bootstrap-shared.js'); +const sharedSource = fs.readFileSync(sharedPath, 'utf8'); +const requireBuiltin = createRequire(__filename); +// Synthetic bytes only: the fake dlopen below never loads a native library. +const content = Buffer.from([0, 1, 2, 3, 4, 5, 255, 0]); +const previous = Buffer.from([8, 7, 6, 5, 4, 3, 2, 1]); +const different = Buffer.from([0, 1, 2, 3, 4, 6, 255, 0]); +type Layout = 'node_modules' | 'standalone'; + +function fixture( + t: TestContext, + layout: Layout, + options: { + fail?: 'write' | 'rename' | 'open'; + winner?: Buffer; + filename?: string; + } = {}, +) { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'pkg-native-cache-')); + const liveDescriptors = new Set(); + t.after(() => { + // Also release leaked descriptors when testing a broken implementation. + for (const fd of liveDescriptors) fs.closeSync(fd); + fs.rmSync(root, { recursive: true, force: true }); + }); + + const snapshot = path.join(root, 'snapshot'); + const cache = path.join(root, 'cache'); + const filename = options.filename ?? 'addon.node'; + const relative = + layout === 'node_modules' + ? path.join('sample-addon', 'build', 'Release', filename) + : filename; + const source = path.join( + snapshot, + layout === 'node_modules' ? 'node_modules' : '', + relative, + ); + const hash = createHash('sha256').update(content).digest('hex'); + const destination = path.join(cache, 'pkg', hash, relative); + fs.mkdirSync(path.dirname(source), { recursive: true }); + fs.writeFileSync(source, content); + + const opens: { file: string; flags: string; fd?: number }[] = []; + const writes: (string | number)[] = []; + const closes: number[] = []; + const renames: { from: string; to: string; before?: Buffer }[] = []; + const loads: { file: string; bytes: Buffer }[] = []; + const error = Object.assign(new Error('injected cache failure'), { + code: options.fail === 'write' ? 'ENOSPC' : 'EPERM', + }); + let collision: string | undefined; + + // Only the VM sees these wrappers; fixture setup and inspection use real fs. + const wrappedFs = { + ...fs, + openSync(file: string, flags: string) { + const attempt: (typeof opens)[number] = { file, flags }; + opens.push(attempt); + if (options.fail === 'open') { + // Another writer owns this exact candidate before our exclusive open. + collision = file; + fs.writeFileSync(file, different); + } + const fd = fs.openSync(file, flags); + attempt.fd = fd; + liveDescriptors.add(fd); + return fd; + }, + writeFileSync(file: string | number, bytes: Buffer) { + writes.push(file); + if (options.fail === 'write') { + fs.writeFileSync(file, bytes.subarray(0, 3)); + throw error; + } + fs.writeFileSync(file, bytes); + }, + closeSync(fd: number) { + fs.closeSync(fd); + liveDescriptors.delete(fd); + closes.push(fd); + }, + renameSync(from: string, to: string) { + renames.push({ + from, + to, + before: fs.existsSync(to) ? fs.readFileSync(to) : undefined, + }); + assert.deepEqual(fs.readFileSync(from), content); + assert.equal(liveDescriptors.size, 0, 'close before publishing'); + if (options.fail === 'rename') { + // Deterministic Windows-style race: a winner appears, then rename fails. + // No timing, child processes, or host-specific rename semantics needed. + if (options.winner) fs.writeFileSync(to, options.winner); + throw error; + } + fs.renameSync(from, to); + }, + }; + const fakeProcess = { + env: { PKG_NATIVE_CACHE_PATH: cache }, + pid: process.pid, + dlopen(_module: object, file: string) { + loads.push({ file, bytes: fs.readFileSync(file) }); + return 'loaded'; + }, + }; + const shared = { + exports: {} as { + patchDlopen: (_inside: (_file: string) => boolean) => void; + }, + }; + // Evaluate the actual shared prelude, without importing a generated SEA bundle + // or patching the test runner's process.dlopen / filesystem module. + vm.runInNewContext( + sharedSource, + { + Buffer, + process: fakeProcess, + module: shared, + require: (id: string) => (id === 'fs' ? wrappedFs : requireBuiltin(id)), + }, + { filename: sharedPath }, + ); + shared.exports.patchDlopen((file) => file.startsWith(snapshot + path.sep)); + + return { + destination, + opens, + writes, + closes, + renames, + loads, + error, + load: () => fakeProcess.dlopen({}, source), + seed(bytes: Buffer) { + fs.mkdirSync(path.dirname(destination), { recursive: true }); + fs.writeFileSync(destination, bytes); + }, + assertLoaded() { + assert.deepEqual(loads, [{ file: destination, bytes: content }]); + }, + assertCache(bytes?: Buffer, extra: string[] = []) { + assert.deepEqual( + fs.readdirSync(path.dirname(destination)).sort(), + [...(bytes ? [path.basename(destination)] : []), ...extra].sort(), + 'no temporary files left behind', + ); + if (bytes) assert.deepEqual(fs.readFileSync(destination), bytes); + assert.equal(liveDescriptors.size, 0, 'no leaked descriptors'); + }, + assertStagedWrite() { + assert.equal(opens.length, 1, 'one exclusive temporary-file open'); + const [{ file, flags, fd }] = opens; + assert.equal(flags, 'wx'); + assert.notEqual( + file, + destination, + 'never open the destination for writing', + ); + assert.equal(path.dirname(file), path.dirname(destination)); + assert.equal(typeof fd, 'number'); + assert.deepEqual(writes, [fd], 'write only through the temporary fd'); + assert.deepEqual(closes, [fd], 'close the temporary fd exactly once'); + assert.throws(() => fs.fstatSync(fd!), { code: 'EBADF' }); + return file; + }, + getCollision() { + assert.ok(collision, 'exclusive open must have been attempted'); + return collision; + }, + }; +} + +for (const layout of ['node_modules', 'standalone'] as const) { + describe(`native addon cache (${layout})`, () => { + for (const existing of [false, true]) { + const state = existing ? 'replacement' : 'cold'; + + it(`publishes a ${state} cache via a complete sibling file`, (t) => { + const f = fixture(t, layout); + if (existing) f.seed(previous); + + assert.equal(f.load(), 'loaded'); + f.assertLoaded(); + const temporary = f.assertStagedWrite(); + assert.deepEqual(f.renames, [ + { + from: temporary, + to: f.destination, + before: existing ? previous : undefined, + }, + ]); + f.assertCache(content); + }); + + it(`cleans up a partial ENOSPC write for a ${state} cache`, (t) => { + const f = fixture(t, layout, { fail: 'write' }); + if (existing) f.seed(previous); + + assert.throws(f.load, (err) => err === f.error); + assert.deepEqual(f.loads, [], 'must not load after a failed write'); + assert.deepEqual(f.renames, [], 'must not publish partial bytes'); + f.assertCache(existing ? previous : undefined); + f.assertStagedWrite(); + }); + + it(`cleans up a failed rename for a ${state} cache`, (t) => { + const f = fixture(t, layout, { fail: 'rename' }); + if (existing) f.seed(previous); + + assert.throws(f.load, (err) => err === f.error); + assert.deepEqual(f.loads, [], 'must not load after a failed rename'); + assert.equal(f.renames.length, 1); + f.assertCache(existing ? previous : undefined); + f.assertStagedWrite(); + }); + } + + it('publishes an addon with a 240-byte basename', (t) => { + // A valid basename near NAME_MAX must not be extended with a temp suffix. + const filename = `${'a'.repeat(235)}.node`; + const f = fixture(t, layout, { filename }); + assert.equal(Buffer.byteLength(path.basename(f.destination)), 240); + + assert.equal(f.load(), 'loaded'); + f.assertLoaded(); + const temporary = f.assertStagedWrite(); + assert.ok(Buffer.byteLength(path.basename(temporary)) <= 255); + assert.deepEqual(f.renames, [ + { from: temporary, to: f.destination, before: undefined }, + ]); + f.assertCache(content); + }); + + it('uses a fresh sibling filename for each cache repair', (t) => { + const f = fixture(t, layout); + for (let attempt = 0; attempt < 2; attempt += 1) { + f.seed(previous); + assert.equal(f.load(), 'loaded'); + } + + assert.equal(f.opens.length, 2); + assert.notEqual(f.opens[0].file, f.opens[1].file); + for (const { file, flags } of f.opens) { + assert.equal(flags, 'wx'); + assert.notEqual(file, f.destination); + assert.equal(path.dirname(file), path.dirname(f.destination)); + } + assert.equal(f.renames.length, 2); + f.assertCache(content); + }); + + it('does not write an identical warm cache', (t) => { + const f = fixture(t, layout); + f.seed(content); + + assert.equal(f.load(), 'loaded'); + f.assertLoaded(); + assert.deepEqual(f.opens, []); + assert.deepEqual(f.writes, []); + assert.deepEqual(f.closes, []); + assert.deepEqual(f.renames, []); + f.assertCache(content); + }); + + for (const matching of [true, false]) { + it(`${matching ? 'accepts' : 'rejects'} a Windows-style rename race with ${matching ? 'matching' : 'different'} winner bytes`, (t) => { + const winner = matching ? content : different; + const f = fixture(t, layout, { fail: 'rename', winner }); + + if (matching) { + assert.equal(f.load(), 'loaded'); + f.assertLoaded(); + } else { + assert.throws(f.load, (err) => err === f.error); + assert.deepEqual(f.loads, [], 'must not load a different winner'); + } + assert.equal(f.renames.length, 1, 'exercise the rename failure'); + f.assertCache(winner); + f.assertStagedWrite(); + }); + } + + it("does not remove another writer's temporary file on open EEXIST", (t) => { + const f = fixture(t, layout, { fail: 'open' }); + f.seed(previous); + + assert.throws(f.load, { code: 'EEXIST' }); + const collision = f.getCollision(); + assert.notEqual(collision, f.destination); + assert.equal(path.dirname(collision), path.dirname(f.destination)); + assert.deepEqual(f.opens, [{ file: collision, flags: 'wx' }]); + assert.deepEqual(fs.readFileSync(collision), different); + assert.deepEqual(f.writes, []); + assert.deepEqual(f.closes, []); + assert.deepEqual(f.renames, []); + assert.deepEqual(f.loads, []); + f.assertCache(previous, [path.basename(collision)]); + }); + }); +}