fs.cp: preserve symlink target instead of linking back to source path by robobun · Pull Request #30073 · oven-sh/bun

robobun · 2026-05-01T19:15:29Z

What

fs.cp / fs.cpSync on Linux and FreeBSD copied every symlink as a link whose target was the path of the source symlink rather than what that symlink pointed at. Deleting the source directory left every copied link dangling.

Repro

import fs from 'fs';
import { join } from 'path';

fs.mkdirSync('from', { recursive: true });
fs.writeFileSync('target.txt', 'hello');
fs.symlinkSync(join(process.cwd(), 'target.txt'), 'from/link');

fs.cpSync('from', 'to', { recursive: true });

console.log(fs.readlinkSync('to/link'));
// Bun (before): "<cwd>/from/link"   ← points back at the source symlink
// Node:         "<cwd>/target.txt"  ← points at the original target

Cause

_copySingleFileSync (src/bun.js/node/node_fs.zig) opens the source with O_NOFOLLOW. When the source is a symlink, that fails with ELOOP and the fallback did:

return Syscall.symlink(src, dest);

symlink(target, linkpath) takes the target string first, so this created dest with a target of src — the source symlink's path — instead of what the source symlink pointed at.

Fix

Added _cpSymlink(src, dest), called from both the Linux and FreeBSD branches:

readlink(src) to get the source link's target
If the target is relative, resolve it against dirname(src) (matching Node's default / non-verbatimSymlinks behaviour — the native path is only taken when that option is unset)
symlink(target, dest)

Also accept EMLINK in the FreeBSD branch since FreeBSD's open(2) returns that (not ELOOP) for O_NOFOLLOW on a symlink.

Verification

New test in test/js/node/fs/cp.test.ts (symlinks - copied link target is the original target, not the source link path) for both cpSync and fs.promises.cp:

asserts readlinkSync(copied) !== srcLinkPath
asserts the copied link resolves to the original target
deletes the source tree and asserts the copied link still reads

USE_SYSTEM_BUN=1 bun test test/js/node/fs/cp.test.ts -t 'copied link target'  → 2 fail
bun bd test test/js/node/fs/cp.test.ts -t 'copied link target'                → 2 pass
bun bd test test/js/node/fs/cp.test.ts                                         → 39 pass / 2 skip
bun run zig:check-all                                                          → all targets OK

Touches the same lines as #28998 (which adds a parent-dir retry around this symlink call) — whichever lands second will need a trivial rebase.

When _copySingleFileSync on Linux/FreeBSD hit ELOOP from open(src, O_NOFOLLOW), it fell back to Syscall.symlink(src, dest), creating a link at dest whose target string was the path of the source symlink rather than what that symlink pointed at. Every symlink copied by fs.cp/fs.cpSync therefore pointed back into the source directory, and removing the source left them dangling. Read the source link's target with readlink() first and, matching Node's default (non-verbatimSymlinks) behaviour, resolve a relative target against dirname(src) before creating the new link. Also accept EMLINK in the FreeBSD branch since FreeBSD's open(2) returns that for O_NOFOLLOW on a symlink.

coderabbitai · 2026-05-01T19:15:35Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 4366f2a5-1e9d-46cb-8372-3316f07bafb5

📥 Commits

Reviewing files that changed from the base of the PR and between 38cacd8 and e915d32.

📒 Files selected for processing (1)

test/js/node/fs/cp-symlink-target.test.ts

Walkthrough

Adds NodeFS._cpSymlink(src, dest) to recreate symlinks by reading the source link target and recreating the destination with a correctly resolved target; integrates it into cp fallback paths for Linux and FreeBSD; adds tests verifying copied symlink targets and behavior after removing the source.

Changes

Symlink Copy Behavior

Layer / File(s)	Summary
Data Shape / New helper `src/bun.js/node/node_fs.zig` (lines 6373–6424)	Adds `NodeFS._cpSymlink(src, dest)`: reads the source symlink target; preserves absolute targets; resolves relative targets against `dirname(src)` using a bounds-checked absolute join (returns `ENAMETOOLONG` on overflow); falls back to recreating `dest` with the original target if `cwd` resolution fails.
Core Integration `src/bun.js/node/node_fs.zig` (lines 6566, 6713–6717)	Replaces prior fallback `Syscall.symlink(src, dest)` usage when `open(..., O_NOFOLLOW)` fails: on Linux `.LOOP` now calls `_cpSymlink`; on FreeBSD `.MLINK` and `.LOOP` now call `_cpSymlink`.
Tests / Validation `test/js/node/fs/cp.test.ts` (lines 221–260), `test/js/node/fs/cp-symlink-target.test.ts`	Adds tests that create absolute and relative source symlinks, run `fs.cpSync` and `fs.promises.cp` with `{ recursive: true }`, assert copied entries remain symlinks whose `readlink` targets are not the source link path and whose `realpathSync` resolves to the original target, and verify the copied absolute symlink still reads expected contents after the source is removed.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main fix: preserving symlink targets instead of linking back to the source path, which is the core issue addressed by this PR.
Description check	✅ Passed	The description comprehensively covers both required sections: 'What' explains the bug with a clear example showing the incorrect behavior, and 'How did you verify' documents thorough testing including new test cases and verification steps.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

_{Review rate limit: 4/5 reviews remaining, refill in 12 minutes.}

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

robobun · 2026-05-01T19:15:41Z

^{Updated 7:18 AM PT - May 2nd, 2026}

❌ @robobun, your commit e915d32 has 2 failures in Build #50169 (All Failures):

test/cli/run/no-orphans.test.ts - timeout on 🍎 14 x64

Details

test/cli/run/no-orphans.test.ts - timeout on 🍎 26 aarch64

test/bake/dev-and-prod.test.ts - code 1 on 🪟 2019 x64-baseline

🧪 To try this PR locally:

bunx bun-pr 30073

That installs a local version of the PR into your bun-30073 executable, so you can run:

bun-30073 --bun

…ve link targets in the test Use joinAbsStringBufChecked for the dirname(src)+target resolve so a near-PATH_MAX relative symlink target surfaces ENAMETOOLONG instead of overflowing resolved_buf. Extend the cp.test.ts case to exercise both the isAbsolute early return and the relative-target resolve branch.

claude

Thanks — the bounds-checked join and the relative-target test coverage both look right now, and I have nothing further; leaving final sign-off to a human since this is new path-resolution logic in the native fs layer (and it'll need coordinating with #28998).

Extended reasoning...

Overview

This PR fixes fs.cp/fs.cpSync on Linux/FreeBSD where the O_NOFOLLOW → ELOOP fallback created symlinks pointing back at the source link path instead of the source link's target. The fix adds a ~50-line _cpSymlink helper in src/bun.js/node/node_fs.zig that readlinks the source, resolves relative targets against dirname(src), and creates the destination link. Two callsites in _copySingleFileSync are updated (Linux + FreeBSD branches, the latter now also accepting EMLINK). A new test in test/js/node/fs/cp.test.ts covers both absolute- and relative-target symlinks for both cpSync and fs.promises.cp.

What changed since my last review

Commit 374190d directly addresses the two issues I raised on the previous revision:

🔴 stack overflow — joinAbsStringBufZ replaced with joinAbsStringBufChecked, passing resolved_buf[0..len-1] to reserve a byte for the manually-written NUL sentinel, and returning ENAMETOOLONG (with the source path attached) on overflow. Verified against resolve_path.zig:1337 and sys.zig:2575 — types line up and the sentinel write is in bounds.
🟡 relative-target coverage — the test now creates both abs_link (absolute target) and rel_link (../target.txt) and asserts realpathSync matches for each, exercising both branches of _cpSymlink.

The 🟣 pre-existing EEXIST/force:true gap remains, but was explicitly flagged as out of scope and is not a regression.

Security risks

Symlink handling in a recursive copy is security-adjacent (path traversal, link spoofing), but fs.cp is already a fully-trusted local-fs operation — a caller who can invoke it can invoke fs.symlink directly. The change strictly reads the existing link target and recreates it at the destination, which is what Node does. The earlier user-controlled-length overflow risk is now closed with the checked join. I don't see new security exposure.

Level of scrutiny

Moderate-to-high. This is hand-written Zig in the native node:fs implementation with manual buffer management and platform-specific errno handling, and the PR description notes it overlaps with #28998 on the same lines. It is a focused, well-tested bug fix and I believe it's correct, but it's not a mechanical change — a human reviewer familiar with node_fs.zig should confirm the getcwd fallback semantics and the FreeBSD EMLINK addition.

Other factors

Bug-hunter found no issues on this revision.
CI failures on the prior commit (no-orphans.test.ts on macOS, dev-and-prod.test.ts on Windows aarch64) are unrelated to this Linux/FreeBSD-only code path.
Test suite for cp.test.ts passes per the PR description; zig:check-all is green.

Jarred-Sumner

Write a test that uses node:test and can run in both Node.js and Bun. Verify the tests pass.

robobun

Added in cc2a49c — test/js/node/fs/cp-symlink-target.test.ts uses node:test + node:assert only.

$ node --experimental-strip-types --test test/js/node/fs/cp-symlink-target.test.ts
▶ fs.cp preserves symlink targets instead of linking back to the source path
  ✔ fs.cpSync (2.0ms)
  ✔ fs.promises.cp (6.7ms)
ℹ pass 2 / fail 0

$ bun bd test test/js/node/fs/cp-symlink-target.test.ts
(pass) ... > fs.cpSync
(pass) ... > fs.promises.cp
2 pass / 0 fail

$ USE_SYSTEM_BUN=1 bun test test/js/node/fs/cp-symlink-target.test.ts
(fail) ... > fs.cpSync          # readlink returned the source symlink path
(fail) ... > fs.promises.cp
0 pass / 2 fail

coderabbitai

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@test/js/node/fs/cp-symlink-target.test.ts`:
- Around line 1-3: Update the header comment that currently says "This test runs
under `bun test`..." to reference the correct run command "bun bd test <file>"
instead; locate the top-of-file comment string in cp-symlink-target.test.ts and
replace the instance of "`bun test`" with "`bun bd test <...test file>`" (or
"`bun bd test <file>`") so it conforms to the repository guideline for test
commands.
- Around line 76-96: Move the describe block "fs.cp preserves symlink targets
instead of linking back to the source path" (including the contained tests
"fs.cpSync" and "fs.promises.cp") out of
test/js/node/fs/cp-symlink-target.test.ts and add it into the existing
test/js/node/fs/cp.test.ts alongside related cp tests; ensure the tests still
call makeFixture() and check(...) and use the same imports (fs, fsp, path) and
the same cleanup logic (fs.rmSync(...)) in their finally blocks; after moving,
remove the now-empty cp-symlink-target.test.ts file so new coverage lives in the
existing cp.test.ts per test guidelines.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: f0edbceb-c20a-4f44-9d44-a7da35a20ab9

📥 Commits

Reviewing files that changed from the base of the PR and between 374190d and cc2a49c.

📒 Files selected for processing (1)

test/js/node/fs/cp-symlink-target.test.ts

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@test/js/node/fs/cp-symlink-target.test.ts`:
- Around line 21-23: Replace manual mkdtempSync usage by creating the test
workspace with harness.tempDir: update makeFixture to accept a base directory
parameter (e.g., makeFixture(base)) instead of calling
fs.mkdtempSync/fs.realpathSync internally, import tempDir from "harness" in the
test, and in the test body create the temp dir via “using base =
tempDir('cp-symlink-')” then call makeFixture(base) to get
origTarget/srcAbs/srcRel; keep fs.cpSync(...) and check(...) unchanged. Ensure
all references to fs.mkdtempSync and os.tmpdir() are removed and the test
follows the existing pattern (test "fs.cpSync", makeFixture, check).

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 6287bd1e-3854-470a-a9ba-238fdbf13c58

📥 Commits

Reviewing files that changed from the base of the PR and between cc2a49c and 38cacd8.

📒 Files selected for processing (1)

test/js/node/fs/cp-symlink-target.test.ts

claude

No new issues found — the buffer-overflow and test-coverage points from the earlier pass were addressed in 374190d. Deferring to a human since this is hand-rolled path/buffer logic in the native fs.cp symlink path, and the last CI report (cc2a49c) was still red across build-zig targets.

Extended reasoning...

Overview

This PR adds NodeFS._cpSymlink (~50 lines of Zig in src/bun.js/node/node_fs.zig) to fix fs.cp/fs.cpSync on Linux/FreeBSD so copied symlinks point at the original target rather than back at the source link path. It wires the helper into the ELOOP/EMLINK fallback in _copySingleFileSync and adds tests in cp.test.ts plus a dual-runtime cp-symlink-target.test.ts.

Security risks

Symlink handling and path resolution are classically security-adjacent (traversal, buffer bounds). The first revision had a reachable stack overflow via joinAbsStringBufZ on long relative targets, now fixed with joinAbsStringBufChecked + ENAMETOOLONG. The current code looks bounds-safe (sentinel byte reserved, error path copies into sync_error_buf), but the fact that one memory-safety issue surfaced during review argues for human eyes on the final state.

Level of scrutiny

High. This is production-critical native runtime code on a hot Node-compat path, with manual stack PathBuffer management, a behavioural choice about how relative targets are resolved (against dirname(src) to match Node's non-verbatimSymlinks default), and a noted overlap with #28998 that whichever lands second will need to rebase around.

Other factors

All prior inline comments (mine and CodeRabbit's) are resolved. The CodeRabbit style suggestions on the new test file were correctly declined for dual-runtime portability. The robobun build-status comment still shows widespread build-zig failures at cc2a49c; the only commit since is an autofix, so CI status on the current head is unconfirmed. Given the surface area and CI uncertainty, a maintainer should sign off rather than auto-approving.

GetFinalPathNameByHandleW returns the filesystem's canonical case (C:\Windows\Temp) while paths built from os.tmpdir() carry the environment's case (C:\Windows\TEMP).

claude

The buffer-overflow and test-coverage concerns from the earlier round are addressed and I found nothing new, but this is non-trivial native path-resolution logic in node_fs.zig (and overlaps with #28998), so it's worth a human sign-off.

Extended reasoning...

Overview

Adds NodeFS._cpSymlink (~50 lines of Zig) that readlinks the source link, resolves relative targets against dirname(src) via joinAbsStringBufChecked, and recreates the link at dest. Wires it into the Linux .LOOP and FreeBSD .MLINK/.LOOP fallbacks in _copySingleFileSync. Adds coverage in cp.test.ts (abs + rel targets) and a new dual-runtime cp-symlink-target.test.ts runnable under node --test.

Security risks

No new attack surface beyond what fs.cp already exposes — the change makes copied links point at the same thing the source link pointed at (Node's behaviour) instead of back into the source tree. The earlier stack-overflow risk from unchecked joinAbsStringBufZ was fixed in 374190d by switching to joinAbsStringBufChecked with a reserved sentinel byte and ENAMETOOLONG on overflow.

Level of scrutiny

Medium-high. This is production native code in a core node:fs path with manual buffer management and path arithmetic across Linux/FreeBSD. The relative-target resolution (cwd + dirname(src) + target) is the kind of logic where off-by-ones or platform quirks surface later, and the first review pass already caught a real memory-safety bug. Not a mechanical change.

Other factors

All prior inline comments (mine and CodeRabbit's) are resolved. The pre-existing EEXIST/force:true gap was correctly scoped out as a follow-up. The PR description notes line overlap with #28998, so whoever merges should be aware a rebase is needed for whichever lands second. Tests cover both branches and pass per the description; zig:check-all passes.

…oven-sh#30073) ## What `fs.cp` / `fs.cpSync` on Linux and FreeBSD copied every symlink as a link whose target was the *path of the source symlink* rather than what that symlink pointed at. Deleting the source directory left every copied link dangling. ## Repro ```js import fs from 'fs'; import { join } from 'path'; fs.mkdirSync('from', { recursive: true }); fs.writeFileSync('target.txt', 'hello'); fs.symlinkSync(join(process.cwd(), 'target.txt'), 'from/link'); fs.cpSync('from', 'to', { recursive: true }); console.log(fs.readlinkSync('to/link')); // Bun (before): "<cwd>/from/link" ← points back at the source symlink // Node: "<cwd>/target.txt" ← points at the original target ``` ## Cause `_copySingleFileSync` (src/bun.js/node/node_fs.zig) opens the source with `O_NOFOLLOW`. When the source is a symlink, that fails with `ELOOP` and the fallback did: ```zig return Syscall.symlink(src, dest); ``` `symlink(target, linkpath)` takes the *target string* first, so this created `dest` with a target of `src` — the source symlink's path — instead of what the source symlink pointed at. ## Fix Added `_cpSymlink(src, dest)`, called from both the Linux and FreeBSD branches: 1. `readlink(src)` to get the source link's target 2. If the target is relative, resolve it against `dirname(src)` (matching Node's default / non-`verbatimSymlinks` behaviour — the native path is only taken when that option is unset) 3. `symlink(target, dest)` Also accept `EMLINK` in the FreeBSD branch since FreeBSD's `open(2)` returns that (not `ELOOP`) for `O_NOFOLLOW` on a symlink. ## Verification New test in `test/js/node/fs/cp.test.ts` (`symlinks - copied link target is the original target, not the source link path`) for both `cpSync` and `fs.promises.cp`: - asserts `readlinkSync(copied) !== srcLinkPath` - asserts the copied link resolves to the original target - deletes the source tree and asserts the copied link still reads ``` USE_SYSTEM_BUN=1 bun test test/js/node/fs/cp.test.ts -t 'copied link target' → 2 fail bun bd test test/js/node/fs/cp.test.ts -t 'copied link target' → 2 pass bun bd test test/js/node/fs/cp.test.ts → 39 pass / 2 skip bun run zig:check-all → all targets OK ``` Touches the same lines as oven-sh#28998 (which adds a parent-dir retry around this symlink call) — whichever lands second will need a trivial rebase. --------- Co-authored-by: robobun <robobun@users.noreply.github.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>

github-actions Bot added the claude label May 1, 2026

claude Bot reviewed May 1, 2026

View reviewed changes

Comment thread src/bun.js/node/node_fs.zig Outdated

Comment thread test/js/node/fs/cp.test.ts Outdated

Comment thread src/bun.js/node/node_fs.zig

claude Bot reviewed May 2, 2026

View reviewed changes

Jarred-Sumner requested changes May 2, 2026

View reviewed changes

add node:test-based cp symlink test runnable in both Node and Bun

cc2a49c

robobun commented May 2, 2026

View reviewed changes

[autofix.ci] apply automated fixes

38cacd8

coderabbitai Bot reviewed May 2, 2026

View reviewed changes

Comment thread test/js/node/fs/cp-symlink-target.test.ts

Comment thread test/js/node/fs/cp-symlink-target.test.ts

coderabbitai Bot reviewed May 2, 2026

View reviewed changes

Comment thread test/js/node/fs/cp-symlink-target.test.ts

claude Bot reviewed May 2, 2026

View reviewed changes

cp-symlink-target.test: compare paths case-insensitively on Windows

e915d32

GetFinalPathNameByHandleW returns the filesystem's canonical case (C:\Windows\Temp) while paths built from os.tmpdir() carry the environment's case (C:\Windows\TEMP).

claude Bot reviewed May 2, 2026

View reviewed changes

Jarred-Sumner merged commit d9ac859 into main May 3, 2026
74 of 77 checks passed

Jarred-Sumner deleted the farm/69c6c19f/fix-cp-symlink-target branch May 3, 2026 00:24

claude Bot mentioned this pull request May 3, 2026

fs: create parent dirs when cpSync copies a symlink #28998

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fs.cp: preserve symlink target instead of linking back to source path#30073

fs.cp: preserve symlink target instead of linking back to source path#30073
Jarred-Sumner merged 5 commits into
mainfrom
farm/69c6c19f/fix-cp-symlink-target

robobun commented May 1, 2026

Uh oh!

coderabbitai Bot commented May 1, 2026 •

edited

Loading

Uh oh!

robobun commented May 1, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

claude Bot left a comment

Uh oh!

Jarred-Sumner left a comment

Uh oh!

robobun left a comment

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

claude Bot left a comment

Uh oh!

claude Bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

robobun commented May 1, 2026

What

Repro

Cause

Fix

Verification

Uh oh!

coderabbitai Bot commented May 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

robobun commented May 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Overview

What changed since my last review

Security risks

Level of scrutiny

Other factors

Uh oh!

Jarred-Sumner left a comment

Choose a reason for hiding this comment

Uh oh!

robobun left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Overview

Security risks

Level of scrutiny

Other factors

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Overview

Security risks

Level of scrutiny

Other factors

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai Bot commented May 1, 2026 •

edited

Loading

robobun commented May 1, 2026 •

edited

Loading