Skip to content

worker: fix TOCTOU race in CWD caching#61725

Closed
mcollina wants to merge 1 commit intonodejs:mainfrom
mcollina:fix/worker-cwd-cache-toctou
Closed

worker: fix TOCTOU race in CWD caching#61725
mcollina wants to merge 1 commit intonodejs:mainfrom
mcollina:fix/worker-cwd-cache-toctou

Conversation

@mcollina
Copy link
Member

@mcollina mcollina commented Feb 7, 2026

The atomic counter used to signal CWD changes to worker threads was
being incremented before chdir() completed, creating a race window
where workers could cache stale directory paths with the new counter
value. This caused process.cwd() in workers to return incorrect values
until the next chdir() call.

Fix by reordering operations: call originalChdir() first, then
increment the counter. This ensures workers never cache stale data
while believing it is current.

A unit test for this fix is not feasible as it would be too flaky due to the timing-dependent nature of the race condition.

Reported-by: Giulio Comi
Reported-by: Caleb Everett

The atomic counter used to signal CWD changes to worker threads was
being incremented before chdir() completed, creating a race window
where workers could cache stale directory paths with the new counter
value. This caused process.cwd() in workers to return incorrect values
until the next chdir() call.

Fix by reordering operations: call originalChdir() first, then
increment the counter. This ensures workers never cache stale data
while believing it is current.

Reported-by: Giulio Comi
Reported-by: Caleb Everett
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. worker Issues and PRs related to Worker support. labels Feb 7, 2026
@mcollina
Copy link
Member Author

mcollina commented Feb 7, 2026

cc @nodejs/tsc

@mcollina mcollina added lts-watch-v20.x PRs that may need to be released in v20.x lts-watch-v22.x PRs that may need to be released in v22.x lts-watch-v24.x PRs that may need to be released in v24.x labels Feb 7, 2026
@Renegade334
Copy link
Member

FYI, a PR was opened by the community reporter at #61664.

Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch.

@codecov
Copy link

codecov bot commented Feb 8, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.75%. Comparing base (81e05e1) to head (4b045af).
⚠️ Report is 357 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #61725      +/-   ##
==========================================
+ Coverage   88.53%   89.75%   +1.21%     
==========================================
  Files         703      675      -28     
  Lines      208538   204525    -4013     
  Branches    40224    39306     -918     
==========================================
- Hits       184629   183565    -1064     
+ Misses      15912    13241    -2671     
+ Partials     7997     7719     -278     
Files with missing lines Coverage Δ
lib/internal/worker.js 96.52% <100.00%> (ø)

... and 199 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mcollina mcollina closed this Feb 8, 2026
@mcollina mcollina deleted the fix/worker-cwd-cache-toctou branch February 8, 2026 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lts-watch-v20.x PRs that may need to be released in v20.x lts-watch-v22.x PRs that may need to be released in v22.x lts-watch-v24.x PRs that may need to be released in v24.x needs-ci PRs that need a full CI run. worker Issues and PRs related to Worker support.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants