From 7b093bfd19be2f3eb42d26892cd8c37e3d71d40e Mon Sep 17 00:00:00 2001 From: giulioAZ Date: Tue, 3 Feb 2026 23:32:15 +0100 Subject: [PATCH 1/2] worker: eliminate race condition in process.cwd() Fixes a race condition in worker thread cwd caching where the counter is incremented before the directory change completes, allowing workers to cache stale directory values. In lib/internal/worker.js, the main thread's process.chdir() wrapper previously incremented the shared counter before calling the actual chdir(), creating a race window where workers could read the old directory but cache it with the new counter value. This caused subsequent cwd() calls to return incorrect paths until the next chdir(). This fix reorders the operations to change the directory first, then increment the counter, ensuring workers are only notified after the directory change completes. Before fix: 54.28% error rate (311/573 races) After fix: 0% error rate (0/832 races) Refs: https://hackerone.com/reports/3407207 Co-authored-by: Giulio Comi Co-authored-by: Caleb Everett Co-authored-by: Utku Yildirim --- lib/internal/worker.js | 2 +- .../test-worker-cwd-race-condition.mts | 226 ++++++++++++++++++ 2 files changed, 227 insertions(+), 1 deletion(-) create mode 100644 test/parallel/test-worker-cwd-race-condition.mts diff --git a/lib/internal/worker.js b/lib/internal/worker.js index 08e87d07e7eb80..2a4caed82cf7c5 100644 --- a/lib/internal/worker.js +++ b/lib/internal/worker.js @@ -112,8 +112,8 @@ if (isMainThread) { cwdCounter = new Uint32Array(constructSharedArrayBuffer(4)); const originalChdir = process.chdir; process.chdir = function(path) { - AtomicsAdd(cwdCounter, 0, 1); originalChdir(path); + AtomicsAdd(cwdCounter, 0, 1); }; } diff --git a/test/parallel/test-worker-cwd-race-condition.mts b/test/parallel/test-worker-cwd-race-condition.mts new file mode 100644 index 00000000000000..caa5e22e4c8e1a --- /dev/null +++ b/test/parallel/test-worker-cwd-race-condition.mts @@ -0,0 +1,226 @@ +import { Worker, isMainThread, workerData, threadName } from 'worker_threads' +import fs from 'fs/promises' +import path from 'path' + +// Number of iterations for each test +const n = 1000; + +// Artificial delays to add in mock process +const mockDelayMs = 1; + +async function main() { + const { barrier, mockCWD, mockCWDCount } = await setup() + if (isMainThread) { + console.log("Node", process.version) + } + test("real process", barrier, process) + test("mock process", barrier, fakeProcess(mockCWDCount, mockCWD)) + test("mock fixed process", barrier, fixedProcess(mockCWDCount, mockCWD)) +} + +function test(label: string, barrier: Barrier, testProcess: TestProcess) { + let racesWon = 0; + let problems = 0; + + for (let i = 0; i < n; i++) { + const expected = dir(i) + let first = '' + + barrier.wait() + + // The race! + // main thread changes directory while tester checks what the directory is + if (isMainThread) { + testProcess.chdir(expected) + } else if (threadName === 'tester') { + first = testProcess.cwd() + } + + barrier.wait() + + // We know the main thread has completed chdir so tester cwd should equal expected, + // but we think there's a TOCTOU bug. If tester won the race and checked cwd simultaneously + // with chdir then cwd will continue to return the stale value. + if (threadName === 'tester') { + const actual = testProcess.cwd() + if (first != expected) { + racesWon++ + if (actual == first) problems++ + } else { + if (actual != expected) throw Error("Bug in tests") + } + } + } + + //barrier.wait() + + if (threadName === 'tester') { + console.log("Test case:", label) + const wonPercent = (racesWon * 100 / n).toFixed(2) + if (racesWon == 0) { + console.log(" ⚠️The tester never called cwd before main thread chdir.") + console.log(" Results are inconclusive, bad luck or bad test code.") + } else if (problems > 0) { + const problemPercent = (problems * 100 / racesWon).toFixed(2) + console.log(` ❌ cwd in worker thread reflected stale value`) + console.log(` errors: ${problemPercent}% ${problems}/${racesWon}`) + } else { + const percent = (racesWon * 100 / n).toFixed(2) + console.log(` ✅ cwd in worker thread always had expected value`) + } + console.log(` races won: ${wonPercent}% ${racesWon}/${n}`) + } +} + +// Create directories to chdir to, start worker threads, and create shared resources. +async function setup() { + const shraedMemSize = Barrier.BYTES_PER_ELEMENT * 1 + Int32Array.BYTES_PER_ELEMENT * 2 + const sharedBuffer = isMainThread ? new SharedArrayBuffer(shraedMemSize) : workerData.sharedBuffer; + let off = 0; + + const barrier = new Barrier(sharedBuffer, off, 2); + off += Barrier.BYTES_PER_ELEMENT; + + const mockCWDCount = new Int32Array(sharedBuffer, off, 1) + off += mockCWDCount.byteLength + + const mockCWD = new Int32Array(sharedBuffer, off, 1) + off += mockCWD.byteLength + + if (isMainThread) { + // Create test directories + for (let i = 0; i < n; i++) { + await fs.mkdir(dir(i), { recursive: true }) + } + + // Spawn the worker + new Worker(import.meta.filename, { workerData: { sharedBuffer }, name: 'coordinator' }) + new Worker(import.meta.filename, { workerData: { sharedBuffer }, name: 'tester' }) + } + return { barrier, mockCWD, mockCWDCount } +} + + +// simulation of the real process +function fakeProcess(mockCWDCount: Int32Array, mockCWD: Int32Array) { + let cachedCwd = ''; + let lastCounter = 0; + + return { + chdir: (dir: string) => { + const cwd = Number(path.basename(dir)) + + // Increment counter, then store dir - like node's real chdir. + busyWait(Math.random() * mockDelayMs) + lastCounter = Atomics.add(mockCWDCount, 0, 1) + 1; + busyWait(Math.random() * mockDelayMs) + Atomics.store(mockCWD, 0, cwd); + }, + cwd: () => { + busyWait(Math.random() * mockDelayMs) + const currentCounter = Atomics.load(mockCWDCount, 0); + if (currentCounter === lastCounter) { + return cachedCwd; + } + busyWait(Math.random() * mockDelayMs) + lastCounter = currentCounter; + cachedCwd = dir(Atomics.load(mockCWD, 0)) + return cachedCwd; + } + } +} + +// same as fakeProcess but counter order changed, this should fix the bug +function fixedProcess(mockCWDCount: Int32Array, mockCWD: Int32Array) { + let cachedCwd = ''; + let lastCounter = 0; + + return { + chdir: (dir: string) => { + const cwd = Number(path.basename(dir)) + + // store dir, then increment counter which should fix the bug + busyWait(Math.random() * mockDelayMs) + Atomics.store(mockCWD, 0, cwd); + busyWait(Math.random() * mockDelayMs) + lastCounter = Atomics.add(mockCWDCount, 0, 1) + 1; + }, + cwd: () => { + busyWait(Math.random() * mockDelayMs) + const currentCounter = Atomics.load(mockCWDCount, 0); + if (currentCounter === lastCounter) { + return cachedCwd; + } + busyWait(Math.random() * mockDelayMs) + lastCounter = currentCounter; + cachedCwd = dir(Atomics.load(mockCWD, 0)) + return cachedCwd; + } + } +} + +const originalCwd = process.cwd() + +// Return expected dir for Nth test +function dir(n: number): string { + return `${originalCwd}/.tmp/${n}` +} + +// block for a number of ms, including fractional sub-ms values. +// this is used in the mock processes so we don't have 100% races won or problems +// not necessary but it makes me more confident in the test/mock setup. +function busyWait(ms: number) { + const end = performance.now() + ms; + do { + // @ts-ignore-error + Atomics.pause() + } while (performance.now() < end); +} + +// class to synchronize threads +// this version relies on a 'coordinator' thread that releases +// the other threads, I think this makes the race more 'fair' +// as neither of the racers are holding the starting gun +// but you can reproduce the bug without a coordinator +class Barrier { + static ELEMENTS = 2 + static BYTES_PER_ELEMENT = Int32Array.BYTES_PER_ELEMENT * Barrier.ELEMENTS + + count: number + state: Int32Array + constructor(shared: SharedArrayBuffer, off: number, count: number) { + this.count = count; + this.state = new Int32Array(shared, off, Barrier.ELEMENTS) + } + + wait() { + // index for number of waiting threads + const WAITING = 0; + // index for how many times this barrier has been used + const GEN = 1; + + if (threadName === 'coordinator') { + // the coordinator will reset waiting, notifying the threads + // to resume + while (true) { + const waiting = Atomics.load(this.state, WAITING) + if (waiting >= this.count) { + Atomics.add(this.state, GEN, 1); + Atomics.store(this.state, WAITING, 0); + Atomics.notify(this.state, GEN); + return; + } + Atomics.wait(this.state, WAITING, waiting) + } + } else { + const counter = Atomics.load(this.state, GEN) + Atomics.add(this.state, WAITING, 1); + Atomics.notify(this.state, WAITING); + Atomics.wait(this.state, GEN, counter); + } + } +} + +type TestProcess = Pick + +await main() From e6f370061df5376a949ca70cb32a490ff5588c6d Mon Sep 17 00:00:00 2001 From: gcomi Date: Mon, 9 Feb 2026 18:44:59 +0100 Subject: [PATCH 2/2] test: replace TS race condition PoC with deterministic JS test Replace the heavy .mts proof-of-concept test with a lightweight deterministic .js test as suggested by @Renegade334. The new test uses SharedArrayBuffer and Atomics to directly simulate a worker calling process.cwd() during a slow chdir syscall, rather than pummeling workers to force race conditions. This is faster, more reliable, and follows Node.js test conventions. --- .../test-worker-cwd-race-condition.js | 70 ++++++ .../test-worker-cwd-race-condition.mts | 226 ------------------ 2 files changed, 70 insertions(+), 226 deletions(-) create mode 100644 test/parallel/test-worker-cwd-race-condition.js delete mode 100644 test/parallel/test-worker-cwd-race-condition.mts diff --git a/test/parallel/test-worker-cwd-race-condition.js b/test/parallel/test-worker-cwd-race-condition.js new file mode 100644 index 00000000000000..19394c85f0d1c4 --- /dev/null +++ b/test/parallel/test-worker-cwd-race-condition.js @@ -0,0 +1,70 @@ +// Flags: --expose-internals --no-warnings +'use strict'; + +const common = require('../common'); +const { isMainThread } = require('worker_threads'); + +if (!isMainThread) { + common.skip('process.chdir is not available in Workers'); +} + +const { internalBinding } = require('internal/test/binding'); + +const assert = require('assert'); +const { Worker } = require('worker_threads'); + +const processBinding = internalBinding('process_methods'); +const originalChdir = processBinding.chdir; + +const cwdOriginal = process.cwd(); +const i32 = new Int32Array(new SharedArrayBuffer(12)); + +processBinding.chdir = common.mustCall(function chdir(path) { + // Signal to the worker that we're inside the chdir call + Atomics.store(i32, 0, 1); + Atomics.notify(i32, 0); + + // Pause the chdir call while the worker calls process.cwd(), + // to simulate a race condition + Atomics.wait(i32, 1, 0); + + return originalChdir(path); +}); + +const worker = new Worker(` + const { + parentPort, + workerData: { i32 }, + } = require('worker_threads'); + + // Wait until the main thread has entered the chdir call + Atomics.wait(i32, 0, 0); + + const cwdDuringChdir = process.cwd(); + + // Signal the main thread to continue the chdir call + Atomics.store(i32, 1, 1); + Atomics.notify(i32, 1); + + // Wait until the main thread has left the chdir call + Atomics.wait(i32, 2, 0); + + const cwdAfterChdir = process.cwd(); + parentPort.postMessage({ cwdDuringChdir, cwdAfterChdir }); +`, { + eval: true, + workerData: { i32 }, +}); + +worker.on('exit', common.mustCall()); +worker.on('error', common.mustNotCall()); +worker.on('message', common.mustCall(({ cwdDuringChdir, cwdAfterChdir }) => { + assert.strictEqual(cwdDuringChdir, cwdOriginal); + assert.strictEqual(cwdAfterChdir, process.cwd()); +})); + +process.chdir('..'); + +// Signal to the worker that the chdir call is completed +Atomics.store(i32, 2, 1); +Atomics.notify(i32, 2); diff --git a/test/parallel/test-worker-cwd-race-condition.mts b/test/parallel/test-worker-cwd-race-condition.mts deleted file mode 100644 index caa5e22e4c8e1a..00000000000000 --- a/test/parallel/test-worker-cwd-race-condition.mts +++ /dev/null @@ -1,226 +0,0 @@ -import { Worker, isMainThread, workerData, threadName } from 'worker_threads' -import fs from 'fs/promises' -import path from 'path' - -// Number of iterations for each test -const n = 1000; - -// Artificial delays to add in mock process -const mockDelayMs = 1; - -async function main() { - const { barrier, mockCWD, mockCWDCount } = await setup() - if (isMainThread) { - console.log("Node", process.version) - } - test("real process", barrier, process) - test("mock process", barrier, fakeProcess(mockCWDCount, mockCWD)) - test("mock fixed process", barrier, fixedProcess(mockCWDCount, mockCWD)) -} - -function test(label: string, barrier: Barrier, testProcess: TestProcess) { - let racesWon = 0; - let problems = 0; - - for (let i = 0; i < n; i++) { - const expected = dir(i) - let first = '' - - barrier.wait() - - // The race! - // main thread changes directory while tester checks what the directory is - if (isMainThread) { - testProcess.chdir(expected) - } else if (threadName === 'tester') { - first = testProcess.cwd() - } - - barrier.wait() - - // We know the main thread has completed chdir so tester cwd should equal expected, - // but we think there's a TOCTOU bug. If tester won the race and checked cwd simultaneously - // with chdir then cwd will continue to return the stale value. - if (threadName === 'tester') { - const actual = testProcess.cwd() - if (first != expected) { - racesWon++ - if (actual == first) problems++ - } else { - if (actual != expected) throw Error("Bug in tests") - } - } - } - - //barrier.wait() - - if (threadName === 'tester') { - console.log("Test case:", label) - const wonPercent = (racesWon * 100 / n).toFixed(2) - if (racesWon == 0) { - console.log(" ⚠️The tester never called cwd before main thread chdir.") - console.log(" Results are inconclusive, bad luck or bad test code.") - } else if (problems > 0) { - const problemPercent = (problems * 100 / racesWon).toFixed(2) - console.log(` ❌ cwd in worker thread reflected stale value`) - console.log(` errors: ${problemPercent}% ${problems}/${racesWon}`) - } else { - const percent = (racesWon * 100 / n).toFixed(2) - console.log(` ✅ cwd in worker thread always had expected value`) - } - console.log(` races won: ${wonPercent}% ${racesWon}/${n}`) - } -} - -// Create directories to chdir to, start worker threads, and create shared resources. -async function setup() { - const shraedMemSize = Barrier.BYTES_PER_ELEMENT * 1 + Int32Array.BYTES_PER_ELEMENT * 2 - const sharedBuffer = isMainThread ? new SharedArrayBuffer(shraedMemSize) : workerData.sharedBuffer; - let off = 0; - - const barrier = new Barrier(sharedBuffer, off, 2); - off += Barrier.BYTES_PER_ELEMENT; - - const mockCWDCount = new Int32Array(sharedBuffer, off, 1) - off += mockCWDCount.byteLength - - const mockCWD = new Int32Array(sharedBuffer, off, 1) - off += mockCWD.byteLength - - if (isMainThread) { - // Create test directories - for (let i = 0; i < n; i++) { - await fs.mkdir(dir(i), { recursive: true }) - } - - // Spawn the worker - new Worker(import.meta.filename, { workerData: { sharedBuffer }, name: 'coordinator' }) - new Worker(import.meta.filename, { workerData: { sharedBuffer }, name: 'tester' }) - } - return { barrier, mockCWD, mockCWDCount } -} - - -// simulation of the real process -function fakeProcess(mockCWDCount: Int32Array, mockCWD: Int32Array) { - let cachedCwd = ''; - let lastCounter = 0; - - return { - chdir: (dir: string) => { - const cwd = Number(path.basename(dir)) - - // Increment counter, then store dir - like node's real chdir. - busyWait(Math.random() * mockDelayMs) - lastCounter = Atomics.add(mockCWDCount, 0, 1) + 1; - busyWait(Math.random() * mockDelayMs) - Atomics.store(mockCWD, 0, cwd); - }, - cwd: () => { - busyWait(Math.random() * mockDelayMs) - const currentCounter = Atomics.load(mockCWDCount, 0); - if (currentCounter === lastCounter) { - return cachedCwd; - } - busyWait(Math.random() * mockDelayMs) - lastCounter = currentCounter; - cachedCwd = dir(Atomics.load(mockCWD, 0)) - return cachedCwd; - } - } -} - -// same as fakeProcess but counter order changed, this should fix the bug -function fixedProcess(mockCWDCount: Int32Array, mockCWD: Int32Array) { - let cachedCwd = ''; - let lastCounter = 0; - - return { - chdir: (dir: string) => { - const cwd = Number(path.basename(dir)) - - // store dir, then increment counter which should fix the bug - busyWait(Math.random() * mockDelayMs) - Atomics.store(mockCWD, 0, cwd); - busyWait(Math.random() * mockDelayMs) - lastCounter = Atomics.add(mockCWDCount, 0, 1) + 1; - }, - cwd: () => { - busyWait(Math.random() * mockDelayMs) - const currentCounter = Atomics.load(mockCWDCount, 0); - if (currentCounter === lastCounter) { - return cachedCwd; - } - busyWait(Math.random() * mockDelayMs) - lastCounter = currentCounter; - cachedCwd = dir(Atomics.load(mockCWD, 0)) - return cachedCwd; - } - } -} - -const originalCwd = process.cwd() - -// Return expected dir for Nth test -function dir(n: number): string { - return `${originalCwd}/.tmp/${n}` -} - -// block for a number of ms, including fractional sub-ms values. -// this is used in the mock processes so we don't have 100% races won or problems -// not necessary but it makes me more confident in the test/mock setup. -function busyWait(ms: number) { - const end = performance.now() + ms; - do { - // @ts-ignore-error - Atomics.pause() - } while (performance.now() < end); -} - -// class to synchronize threads -// this version relies on a 'coordinator' thread that releases -// the other threads, I think this makes the race more 'fair' -// as neither of the racers are holding the starting gun -// but you can reproduce the bug without a coordinator -class Barrier { - static ELEMENTS = 2 - static BYTES_PER_ELEMENT = Int32Array.BYTES_PER_ELEMENT * Barrier.ELEMENTS - - count: number - state: Int32Array - constructor(shared: SharedArrayBuffer, off: number, count: number) { - this.count = count; - this.state = new Int32Array(shared, off, Barrier.ELEMENTS) - } - - wait() { - // index for number of waiting threads - const WAITING = 0; - // index for how many times this barrier has been used - const GEN = 1; - - if (threadName === 'coordinator') { - // the coordinator will reset waiting, notifying the threads - // to resume - while (true) { - const waiting = Atomics.load(this.state, WAITING) - if (waiting >= this.count) { - Atomics.add(this.state, GEN, 1); - Atomics.store(this.state, WAITING, 0); - Atomics.notify(this.state, GEN); - return; - } - Atomics.wait(this.state, WAITING, waiting) - } - } else { - const counter = Atomics.load(this.state, GEN) - Atomics.add(this.state, WAITING, 1); - Atomics.notify(this.state, WAITING); - Atomics.wait(this.state, GEN, counter); - } - } -} - -type TestProcess = Pick - -await main()