Repository navigation
fs.realpath: a backslash is part of the file name on POSIX, and each variant reads its path as Node does
#45423
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
b4d8809
8fc0368
550cf51
558b815
02b0847
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -213,8 +213,6 @@ pub(crate) use super::node_fs_binding::Binding; | |
| use bun_jsc::JSPromiseStrong; | ||
|
|
||
| use super::dir_iterator as DirIterator; | ||
| #[cfg(not(windows))] | ||
| use bun_resolver::fs::FileSystem; | ||
|
|
||
| // On POSIX the libuv-backed code paths (`UVFSRequest`, `uv_fs_*`) are absent: | ||
| // `UVFSRequest` aliases `AsyncFSTask` and every `uv::*` reference is gated | ||
|
|
@@ -7562,7 +7560,7 @@ impl NodeFS { | |
| // For `fs.realpath`, Node.js uses `lstat`, exposing the native system call under | ||
| // `fs.realpath.native`. In Bun, the system call is the default, but the error | ||
| // code must be changed to make it seem like it is using lstat (tests expect this), | ||
| // in addition, some more subtle things depend on the variant. | ||
| // and the path goes through `path.resolve` first, as it does in Node.js. | ||
| pub(crate) fn realpath_inner( | ||
| &mut self, | ||
| args: &args::Realpath, | ||
|
|
@@ -7595,17 +7593,8 @@ impl NodeFS { | |
| ..Default::default() | ||
| }); | ||
| } | ||
| let mut buf = unsafe { bun_core::ffi::cstr(ptr) }.to_bytes(); | ||
| if variant == RealpathVariant::Emulated { | ||
| // remove the trailing slash | ||
| // | ||
| // `buf` is an immutable view and every consumer below copies by | ||
| // length, so just shrink the slice — writing a NUL back through | ||
| // `ptr.cast_mut()` while `buf` is live would be Stacked-Borrows UB. | ||
| if buf.last() == Some(&b'\\') { | ||
| buf = &buf[..buf.len() - 1]; | ||
| } | ||
| } | ||
| let buf = unsafe { bun_core::ffi::cstr(ptr) }.to_bytes(); | ||
| let _ = variant; | ||
| if args.encoding == Encoding::Utf8 { | ||
| if let PathLike::String(s) = &args.path { | ||
| if strings::eql_long(s.slice(), buf, true) { | ||
|
|
@@ -7620,27 +7609,26 @@ impl NodeFS { | |
| { | ||
| let mut outbuf = bun_paths::path_buffer_pool::get(); | ||
| let inbuf = &mut self.sync_error_buf; | ||
| // SAFETY: single-threaded init flag (resolver/fs.rs). | ||
| debug_assert!( | ||
| bun_resolver::fs::INSTANCE_LOADED.load(core::sync::atomic::Ordering::Relaxed) | ||
| ); | ||
|
|
||
| let path_slice = args.path.slice(); | ||
| // SAFETY: instance() returns the leaked singleton; INSTANCE_LOADED checked above. | ||
| let fs = FileSystem::get(); | ||
| let parts = [fs.top_level_dir, path_slice]; | ||
| let inbuf_len = inbuf.len(); | ||
| let Some(joined) = fs.abs_buf_checked(&parts, &mut inbuf[..inbuf_len - 1]) else { | ||
| return Err(sys::Error { | ||
| errno: E::ENAMETOOLONG as _, | ||
| syscall: sys::Tag::realpath, | ||
| path: args.path.slice().into(), | ||
| ..Default::default() | ||
| }); | ||
| let path = match variant { | ||
| RealpathVariant::Native => args.path.slice_z(inbuf), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟣 pre-existing, not blocking: pre-existing: on POSIX, fs.watch, fs.watchFile and cpSync symlink copying still treat a backslash as a separator, so a file named a\b is watched or linked as a/b. They join the path with join_abs_string_buf::platform::Posix, whose Posix arm at src/paths/resolve_path.rs:1386 uses the loose normalizer, the mechanism this PR removes from realpath_inner. Fix: make the Posix normalizer split on '/' only, or route these joins through bun_node_path::resolve_posix_t as realpath_inner now does, which covers the 4 sites listed. Same pattern at 4 sites (node_fs_watcher.rs:1147, node_fs_stat_watcher.rs:822, node_fs.rs:8426, path_watcher.rs:606). Why this was flaggedA user on Linux or macOS calls fs.watch("a\b") or fs.watchFile("a\b") on a file literally named a\b, or runs fs.cpSync(src, dest, { recursive: true }) over a relative symlink whose target contains a backslash. FSWatcher::init at src/runtime/node/node_fs_watcher.rs:1145-1150 and StatWatcher::init at src/runtime/node/node_fs_stat_watcher.rs:819-822 join the input onto the cached top_level_dir with Path::join_abs_string_buf[_checked]::platform::Auto (Auto = Posix on unix); cp_symlink at src/runtime/node/node_fs.rs:8426 does the same with platform::Posix. normalize_string_buf_t's Platform::Posix arm at resolve_path.rs:1386-1388 dispatches to normalize_string_loose_buf_t, which rewrites every '\' as '/'. So the watcher is installed on cwd/a/b (ENOENT or a different file) and the copied symlink points at .../a/b instead of .../a\b. This is the exact mechanism the PR fixes for fs.realpath but the sibling node:fs entry points keep it; the base branch fails the same way, so this is pre-existing. The new test at test/js/node/fs/fs.test.ts:3241 only exercises the five realpath entry points. Verification: pre-existing. On Linux/macOS fs.watch("a\b") on a file named a\b watches a/b instead. All three sites join through the same normalizer the PR removed from realpath_inner: node_fs_watcher.rs:1147, node_fs_stat_watcher.rs:822, node_fs.rs:8426. normalize_string_buf_t routes Platform::Posix to normalize_string_loose_buf_t (resolve_path.rs:1386-1388). The base already fails the same way.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed for two of the four, not changed here. With a file 🤖 Addressed by Claude Code |
||
| RealpathVariant::Emulated => { | ||
| let parts = [args.path.slice()]; | ||
| let len = bun_node_path::resolve_buf_len(false, &parts); | ||
| let mut bufs = vec![0u8; 2 * len]; | ||
| let (buf, buf2) = bufs.split_at_mut(len); | ||
|
coderabbitai[bot] marked this conversation as resolved.
Resolved
|
||
| let resolved = bun_node_path::resolve_posix_t(&parts, buf, buf2)?; | ||
|
dylan-conway marked this conversation as resolved.
Resolved
Comment on lines
+7615
to
+7620
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟣 pre-existing, not blocking: pre-existing: a program that calls fs.realpath(relativePath, cb) and then process.chdir() can get the path resolved against the new cwd, or a torn cwd string, where Node resolves it synchronously against the cwd at call time. The Emulated arm at src/runtime/node/node_fs.rs:7620 runs resolve_posix_t on the work-pool thread, reading the cwd bytes that process.chdir rewrites in place. Fix: for the async Emulated flavor, capture the cwd (or run path.resolve) on the JS thread when the call is made, before the task is queued, or copy top_level_dir under the lock rather than handing out a slice into a buffer that chdir overwrites. Why this was flaggedA user calls fs.realpath("x", cb) (callback form, RealpathVariant::Emulated via realpath_cb at src/runtime/node/node_fs_binding.rs:409) and then process.chdir("other") on the same tick. The callback flavor runs NodeFS::dispatch with Flavor::Async on the work pool (src/runtime/node/node_fs.rs:1375), so realpath_inner reaches bun_node_path::resolve_posix_t at node_fs.rs:7620 on a pool thread. resolve_posix_t fetches the cwd through get_cwd_u8 at src/node_path/lib.rs:198-201, which copies top_level_dir(), a &'static [u8] pointing into fs.top_level_dir_buf. process.chdir on the JS thread calls set_process_cwd at src/jsc/VirtualMachine.rs:5822-5837, which overwrites top_level_dir_buf in place and then publishes a new slice; a pool thread that already holds the old slice reads bytes while they are being rewritten, and one that reads after the publish resolves "x" against the new cwd. Node's fs.realpath runs pathModule.resolve(p) synchronously inside the call, so the later chdir cannot affect it. The base branch did the same read on the pool thread through FileSystem::abs_buf_checked, so this is pre-existing. Verification: pre-existing. Trigger: a program calls fs.realpath(relativePath, cb) and then process.chdir() before the work-pool thread has run the task. The Emulated arm at node_fs.rs:7620 fetches the cwd on the pool thread; set_process_cwd rewrites fs.top_level_dir_buf in place at src/jsc/VirtualMachine.rs:5822-5835, so the task can read a torn cwd or resolve against the new cwd. The base branch did the same read on the same pool thread.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed, and the same before this PR, so not changed here. Running 🤖 Addressed by Claude Code |
||
| if resolved.len() >= inbuf.len() { | ||
| return Err(sys::Error { | ||
| errno: E::ENAMETOOLONG as _, | ||
| syscall: sys::Tag::realpath, | ||
| path: args.path.slice().into(), | ||
|
claude[bot] marked this conversation as resolved.
Resolved
|
||
| ..Default::default() | ||
| }); | ||
| } | ||
| paths::resolve_path::z(resolved, inbuf) | ||
| } | ||
| }; | ||
| let path_len = joined.len(); | ||
| inbuf[path_len] = 0; | ||
| let path = ZStr::from_buf(&inbuf[..], path_len); | ||
|
|
||
| #[cfg(any(target_os = "linux", target_os = "android"))] | ||
| let flags = sys::O::PATH; // O_PATH is faster | ||
|
|
@@ -7658,7 +7646,6 @@ impl NodeFS { | |
| Ok(buf_) => buf_, | ||
| }; | ||
|
|
||
| let _ = variant; | ||
| if args.encoding == Encoding::Utf8 { | ||
| if let PathLike::String(s) = &args.path { | ||
| if strings::eql_long(s.slice(), buf, true) { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.