Repository navigation
Conversation
…ve operations readdir, recursive rm, writeFile and the mkdir -p existence probe open their argument through NtCreateFile relative to the cwd handle, which has no notion of a drive-relative path: "C:dir" named a stream "dir" on a file "C" there, so they failed with ENOENT (or, for writeFile, created that stream), while the libuv-backed operations let Win32 resolve the same spelling. Resolve such paths with GetFullPathNameW in the node:fs path slicers, and open the root of an async recursive readdir through the slicer like the other readdir flavors do.
|
Status: fix pushed, self-review in progress. Reproduced on Windows Server x64 with the 1.4.0 canary at the base commit ( |
|
Warning Review limit reached
Next review available in: 38 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
Comment |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes the shared Windows path slicer (slice_z_with_force_copy, os_path_kernel32) that every NT-backed node:fs operation flows through, and adds a new owned field with a detached-lifetime borrow to AsyncReaddirRecursiveTask, a human look would still be worthwhile.
Checked: is_drive_relative_windows correctly excludes ADS names (>1 char before :) and drive-absolute paths; the drive_relative_scratch pool guard outlives the shadowed sliced borrow, and the fall-through to the plain copy still sees the outer as-given sliced; get_full_path_name_w's n >= out.len() handles the buffer-too-small return per MSDN; root_open_path is only used for the root open — error .path and Dirent joins still read root_path; the new os_path_kernel32 branch reuses the same buffer roles as the rooted branch above it.
Extended reasoning...
Overview
This PR fixes drive-relative path handling (C:dir — drive letter + colon, no separator) on Windows for node:fs operations that open via bun's own NtCreateFile paths rather than libuv/Win32. It touches five files: a new GetFullPathNameW extern in src/windows_sys/externs.rs, a safe wrapper get_full_path_name_w plus two unit tests in src/sys/windows/mod.rs, two new #[cfg(windows)] helpers (is_drive_relative_windows, resolve_drive_relative_windows) integrated into PathLike::slice_z_with_force_copy and os_path_kernel32 in src/runtime/node/types.rs, a new root_open_path: Box<[u8]> field on AsyncReaddirRecursiveTask in src/runtime/node/node_fs.rs so the async recursive readdir opens its root through slice_z like every other flavor, and ~220 lines of Windows-gated tests in test/js/node/fs/fs.test.ts.
Security risks
None identified. The change resolves drive-relative paths through GetFullPathNameW, which is the same pure-string resolution CreateFileW (and therefore libuv-backed operations) already applies — it does not touch the filesystem and does not widen what the caller can name. The predicate is narrow (single ASCII drive letter, colon, no separator), so ADS names (file.txt:stream) and plain relative names are untouched. On resolution failure the argument passes through unchanged. Buffer handling uses the pooled PathBuffer/WPathBuffer and the wrapper's n == 0 || n >= out.len() check matches the documented API contract.
Level of scrutiny
High. slice_z_with_force_copy and os_path_kernel32 are the choke points that translate every node:fs path argument for the NT-level open paths on Windows; a mistake here affects readdir, writeFile, recursive rm, mkdir -p, opendir, and cpSync uniformly. The PR also documents behavioral side effects (mkdirSync recursive return value becomes the \\?\C:\... form, Dirent.parentPath for drive-relative sync readdir becomes the resolved path) and argues a specific layering choice (resolve in the slicer, not in normalize_path_windows, because the NT layer cannot know a directory handle's drive without the mount-manager query removed in #33874). These are correct-looking decisions but they are design decisions.
Other factors
The change is exceptionally well-documented and well-tested: the PR description traces the exact NT-relative-name misinterpretation for each affected operation, the tests cover every readdir flavor plus opendir, lowercase drive letters, bare C:, writeFile alongside a decoy file named after the drive letter, recursive rm/mkdir, a nonexistent-drive negative case, and an ADS guard. Tests follow harness conventions (tempDir, bunEnv, concurrent subprocess drain, it.concurrent, describe.if(isWindows)). I traced root_path vs root_open_path through readdir_with_entries_recursive_async and confirmed error paths and result joins still use root_path while only the root open uses root_open_path. The scratch-buffer lifetime in slice_z_with_force_copy is correct (declared before the shadowing let sliced), and the fall-through path outside the #[cfg(windows)] block still sees the original slice. No issues found — deferring only because this is a shared-helper change on a critical path, not a simple mechanical fix.
|
Updated 1:47 PM PT - Aug 13th, 2026
✅ @robobun, your commit a8b28a8831ea2451c5ca41932790dd56f9ebaa7d passed in 🧪 To try this PR locally: bunx bun-pr 38112That installs a local version of the PR into your bun-38112 --bun |
…selves Resolving the whole drive-relative string with GetFullPathNameW also applied Win32's name rules: a trailing dot or space was stripped and nul.txt became the NUL device, unlike the absolute spelling of the same name (and unlike node). Ask Win32 only which directory the bare designator stands for, then hand <dir>\name to the existing absolute branches. Pin the literal-name behavior and the rooted-without-drive async recursive readdir in the tests.
| /// What the root directory is actually opened with: `root_path` as | ||
| /// the slicer spells it for the syscalls (on Windows, rooted and | ||
| /// drive-relative arguments become absolute paths there, as they do | ||
| /// for every other readdir flavor via `readdir_inner`). `root_path` | ||
| /// itself stays as given, since it is what parentPath and error | ||
| /// messages are built from. Heap-owned, NUL-terminated like `root_path`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // The root subtask's basename is `root_open_path`, which must not | ||
| // leak into results; the caller passes `is_root` explicitly. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// `C:foo` or `C:`: a drive letter and colon not followed by a separator. | ||
| /// Win32 resolves such a path against that drive's current directory. An | ||
| /// alternate data stream name (`file.txt:stream`) has more than one character | ||
| /// before its colon and is not one of these. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Turn a drive-relative path (see [`is_drive_relative_windows`]) into the | ||
| /// unnormalized absolute path it stands for, `<directory of that drive>\name`, | ||
| /// written into `scratch`. The caller then treats it exactly like an absolute | ||
| /// argument. | ||
| /// | ||
| /// Node's fs resolves these before the syscall, and so, in effect, do bun's | ||
| /// libuv-backed operations, because Win32 resolves them itself. The operations | ||
| /// that open through bun's own `NtCreateFile` paths relative to the cwd handle | ||
| /// (readdir, recursive rm, writeFile, the mkdir -p existence probe) did not: | ||
| /// to NT, `C:foo` relative to a directory names a file `C` with an alternate | ||
| /// data stream `foo`, so they reported ENOENT or, for writeFile, created that | ||
| /// stream. | ||
| /// | ||
| /// Only the bare drive designator is handed to Win32: which directory `C:` | ||
| /// currently stands for is Win32 state (the cwd for the cwd's drive, otherwise | ||
| /// the drive's own current directory). Resolving the whole string with it | ||
| /// would also apply Win32's name rules, stripping trailing dots and spaces and | ||
| /// turning `nul.txt` into the NUL device, which neither the absolute branches | ||
| /// nor node apply; joining the name here keeps `C:foo` equivalent to the | ||
| /// absolute spelling of the same name. | ||
| /// | ||
| /// `None` when the directory cannot be determined or the result does not fit; | ||
| /// the caller then keeps its existing handling of the path as given. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // `MAX_PATH_BYTES` is sized so that any wide path re-encodes into a | ||
| // `PathBuffer`, so this cannot truncate. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // A drive-relative argument is rebound to the absolute path it | ||
| // stands for, so the branch below handles it like any other | ||
| // absolute argument. The scratch is declared before the shadowing | ||
| // `sliced` so it outlives the borrow; the plain copy at the bottom | ||
| // still sees the argument as given. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Same buffer roles as the rooted branch above. The result is | ||
| // absolute, so it gets the `\\?\` prefix like an absolute | ||
| // argument and mkdir -p's NT-level existence probe receives an | ||
| // absolute object name. If the drive's directory cannot be | ||
| // determined, the path is passed on as given, below. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// `GetFullPathNameW`: the resolution Win32 applies to a path before opening | ||
| /// it (`CreateFileW` and friends run the same routine), written into `out`. | ||
| /// For a drive-relative path (`C:foo`) that means the process cwd when `C:` | ||
| /// is the cwd's drive, otherwise that drive's own current directory (the | ||
| /// hidden `=C:` environment variable), otherwise the drive root; `.` and `..` | ||
| /// are collapsed as well. Pure string processing, no filesystem access. | ||
| /// `None` when the call fails or the result does not fit `out`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // 0 is failure; a value >= the buffer length is the size the result would | ||
| // need (including its NUL), i.e. it did not fit. Otherwise the result was | ||
| // written and NUL-terminated at `out[n]`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// The bare designator of the cwd's drive, in either case, stands for the | ||
| /// cwd itself; this is what `node:fs` resolves drive-relative paths with. | ||
| /// The test binary's cwd is always drive-letter rooted. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes the Windows path-resolution layer that every NT-backed node:fs operation goes through (slice_z, os_path_kernel32) and adds new FFI + buffer-join logic, a human look would still be worthwhile.
What was reviewed:
is_drive_relative_windowscorrectly excludes ADS names (file.txt:stream) and drive-absolute paths;resolve_drive_relative_windowsbounds-checks the join and falls through on overflow/failure.get_full_path_name_wreturn-value handling matches the Win32 contract (0 = fail, ≥ len = didn't fit).AsyncReaddirRecursiveTask::root_open_pathis heap-owned with the same lifetime asroot_path, so thedetach_lifetimeinrunis sound.- Tests spawn per-cwd children, cover the missing-drive case (must fail rather than act on cwd), and guard the ADS path unchanged.
Extended reasoning...
Overview
This PR fixes Windows drive-relative path handling (C:dir) for the node:fs operations that open through bun's own NtCreateFile code (readdir, recursive rm, writeFile, recursive mkdir's existence probe). It adds is_drive_relative_windows + resolve_drive_relative_windows in src/runtime/node/types.rs and wires them into slice_z_with_force_copy and os_path_kernel32; adds a GetFullPathNameW extern in src/windows_sys/externs.rs with a safe wrapper get_full_path_name_w in src/sys/windows/mod.rs; and gives AsyncReaddirRecursiveTask a separate root_open_path (via slice_z) so the async recursive readdir opens its root the same way the other flavors do. ~270 lines of Windows-gated tests exercise readdir/opendir/writeFile/rm/mkdir across sync/async/callback/promise, plus lowercase drive, bare C:, missing-drive, and ADS guard cases.
Security risks
Path resolution for filesystem operations is security-relevant. The pre-existing bug this fixes — Z:dir\sub on a nonexistent Z: silently acting on the cwd's drive — is itself the more concerning behavior; the fix routes drive-relative spellings through Win32's own GetFullPathNameW (only the two-char drive designator is passed, the name is joined afterward so trailing-dot/nul.txt normalization is not applied). The is_drive_relative_windows predicate is narrow (single ASCII drive letter, colon, no separator at index 2), so ADS names and ordinary relative paths are untouched. The join into scratch is bounds-checked against scratch.len() and returns None on overflow, falling back to the previous behavior. I did not spot a path-escape or buffer-overrun issue.
Level of scrutiny
High. PathLike::slice_z is on the hot path of every NT-backed fs call on Windows; a mistake here changes what file every readdir/rm/writeFile touches. The change also introduces new FFI surface (GetFullPathNameW) and reinterprets a wide-path buffer back to UTF-8 via from_w_path. The reasoning in the PR description is thorough and the test matrix is broad, but this is exactly the kind of platform-specific, layer-crossing change (Win32 vs NT semantics, per-drive cwd, WTF-8 encoding) that benefits from a maintainer familiar with bun's Windows path layer confirming the placement (slicer vs normalize_path_windows) and the Dirent.parentPath side effects called out in the description.
Other factors
The comment-cop bot has left nine outstanding inline warnings about long code comments that the author has not yet addressed; per the repo's own guidance those should be resolved or dismissed before merge. CI (#94771) was still building at the time of review. Given the scope, the outstanding automated feedback, and the security-adjacent nature of filesystem path resolution, I'm deferring rather than approving.
Problem
On Windows, a drive-relative path (
C:dir: drive letter and colon with no separator after them) works for mostnode:fsoperations but not for the ones that open through bun's own NT code. With the cwd onC:anddirinside it (bun 1.4.0 canary, node v26 as the reference):fs.readdirSync("C:dir"):ENOENT: no such file or directory, scandir 'C:dir'. Same forfs.promises.readdir, the callback form,{ recursive: true }in every flavor, andopendirSync. Node lists the directory.fs.writeFileSync("C:name", data): creates a file namedCin the cwd and writes the data into an alternate data streamnameon it. Node createsname. (A name containing a dot happened to work, see below.)fs.rmSync("C:dir", { recursive: true }):ENOENT; withforce: trueit silently leaves the tree in place.fs.mkdirSync("C:dir", { recursive: true })on an existing directory:EEXIST; node returns without error.Z:drive,fs.writeFileSync("Z:dir\\new.txt", "")createddir\new.txtin the cwd onC:andfs.readdirSync("Z:dir\\sub")listed the cwd'sdir\sub.Cause:
PathLike::slice_z(src/runtime/node/types.rs), which rewrites absolute and rooted-but-driveless arguments but copies everything else through as is, and then open it relative to the cwd handle withNtCreateFile(sys::open_dir_at_windows_a,sys::openat/unlinkatviazig_delete_tree,sys::openatinwrite_file; for recursive mkdir theCreateDirectoryWcall itself works but theEEXISTprobedirectory_exists_at_os_pathis NT-relative too). An NT name relative to a directory has no notion of drives:C:dirthere is the streamdirof a file namedC, so lookups fail andwriteFilecreates exactly that.normalize_path_windows(src/sys/lib.rs) strips a leading drive letter only on its multi-component branch, and then resolves against the cwd handle whatever the letter was. That is whyC:dir\subandC:wf.txt(the.routes to that branch) appeared to work, and whyZ:dir\subacted onC:.AsyncReaddirRecursiveTask::create) did not go throughslice_zat all, so it also failed for rooted-but-driveless arguments (/Users/.../dir:EINVAL) that the other readdir flavors accept.readFileSync,statSync,existsSync,unlinkSync,renameSync, ...) were unaffected because they hand the string to Win32, which resolves drive-relative paths itself.Fix
src/runtime/node/types.rs:slice_z_with_force_copyandos_path_kernel32resolve a drive-relative argument withGetFullPathNameW(resolve_drive_relative_windows) and then take their existing drive-absolute branches, so the NT-level operations receive the same\\?\C:\...object name an absolute argument would produce.is_drive_relative_windowsonly matches a single letter followed by:with no separator after it;file.txt:streamand plain relative names are untouched. If Win32 cannot resolve the path, the argument is passed through as before.src/sys/windows/mod.rs,src/windows_sys/externs.rs:get_full_path_name_w, a wrapper over the newGetFullPathNameWextern, next to the other kernel32 path wrappers.src/runtime/node/node_fs.rs: the async recursive readdir opens its root throughslice_z(root_open_path), likereaddir_innerdoes for the other flavors.root_pathstays as given becauseDirent.parentPathand error paths are built from it, so nothing else about its output changes.GetFullPathNameWis the resolutionCreateFileWapplies to it, so after this change one spelling names one file across all ofnode:fs(the libuv-backed operations already resolved it this way), and the result is what node produces (node's fs also reaches Win32 with the path; see Background for the one case where node's own pre-resolution differs). Fixing it in the slicer rather than innormalize_path_windowsis deliberate: the NT layer cannot know which drive a directory handle is on without the mount-manager query that Build Windows openat object names from NT device paths #33874 removed, and stripping the letter regardless is the wrong-drive behavior described above. Resolving before the path reaches it is correct for every drive and also fixes the multi-component case.mkdirSync("C:new\\sub", { recursive: true })now returns\\?\C:\<cwd>\newinstead ofC:new, which is what node returns.Dirent.parentPathfor a drive-relative argument to a non-recursive or sync-recursive readdir is the resolved path, following what bun already does for rooted-but-driveless arguments (node:fs: fix operations on Windows filenames with trailing dots or spaces #33034 changes that convention separately); the async recursive flavor reports it as given, like node.C:fooalone) and node:fs: strip trailing separators from Windows path arguments #38053 (trailing separators; its drive-relative form skips the directory operations because of this bug). Both compose with this change.test/js/node/fs/fs.test.ts,describe("windows drive-relative paths"), five fixtures run in a child whose cwd is a temp dir: readdir in every flavor plusopendir, a lowercase drive letter, bareC:,writeFilenext to a file named after the drive letter, recursivermof a tree and of a file,force: true, recursive mkdir on an existing directory and its return value, paths on a drive letter that does not exist (they must fail, not touch the cwd), and an alternate data stream name. On Windows x64, 4 of the 5 fail on the unfixed canary at the base commit and all 5 pass with this branch; the ADS one is a guard and passes both ways. The describe is skipped on other platforms, where the behavior does not exist.fs.test.ts(505 pass; the one failure,readSync > works on large files, is a 4.9 GB sparse write that times out on the debug build and does not involve paths),cp,dir,fs-mkdir,fs-path-length,promises,readdir-windows-ntstatus,readdirSync-recursive-error-leak,rm-windows-ntstatus,translate-uv-error-windows,fs-writeSync-stdio-windows,glob(181 pass, 0 fail); a 40-operation drive-relative matrix over the libuv-backed operations is byte-identical before and after apart fromcpSyncof a directory, which now works. On Linux:fs.test.ts,promises.test.js,readdirSync-recursive-error-leakwithbun bd(the only failure is thereaddirSync recursive x 100debug-build timeout; the sync path is untouched here).cargo clippy -p bun_runtimeon Linux is clean; the twobun_sysunit tests compile for the Windows target (cargo check --tests) and their assertions were confirmed against the API on the Windows box, sincecargo testfor these crates does not link without bun's native deps.Background
C:dirmeansdirinside the current directory of driveC; for the drive the process cwd is on, that is the process cwd. For other drives Win32 reads the hidden=X:environment variable that shells maintain, and falls back to the drive root.GetFullPathNameWperforms exactly this resolution (plus./..collapsing) as pure string processing; it is whatCreateFileWdoes internally before opening. Node's fs pre-resolves arguments in JS withpath.resolve, which consults=C:even for the cwd's drive, so node differs from Win32 (and from this change) only when a parent started the process with a cwd that disagrees with an inherited=C:; bun's libuv-backed operations have always used the Win32 answer, and this change makes the rest ofnode:fsagree with them.name:stream. This is why:is legal inside a relative NT name and why the unfixed code ended up creating a stream; it is also why the new check requires exactly one character before the colon.NtCreateFilewith either a full object name (\??\C:\...) or a name relative to a directory handle (RootDirectory). The relative form is resolved purely inside the filesystem, so none of Win32's path conventions (drive-relative paths, per-drive directories,..) apply to it.PathLike::slice_zis wherenode:fstranslates the Win32 conventions it accepts into something these calls understand; rooted-but-driveless paths (\dir) were already handled there.\\?\prefix: the Win32 form of an absolute path that skips Win32 normalization and theMAX_PATHlimit.slice_zalready produces it for every absolute argument;to_nt_pathturns it into\??\forNtCreateFile, and libuv passes it to Win32 unchanged.node vs bun before/after, cwd on C: (Windows Server, node v26.3.0, bun 1.4.0 canary at the base commit, then this branch)
Unchanged and identical before/after (drive-relative spelling): readFileSync, openSync, accessSync, existsSync, statSync, lstatSync, realpathSync.native, appendFileSync, truncateSync, utimesSync, chmodSync, copyFileSync, linkSync, symlinkSync (target stored as given), readlinkSync, renameSync, unlinkSync, non-recursive rmSync, non-recursive mkdirSync, rmdirSync, mkdtempSync, statfsSync, Bun.file, Bun.write, promises.readFile/stat/access, cpSync of a file, and error
.pathvalues (reported as given, as for every other bun fs error).