Skip to content
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 22 additions & 9 deletions src/runtime/node/node_fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2164,6 +2164,14 @@ mod _async_tasks {
/// Heap-owned, NUL-terminated (`[path.., 0]`); freed on drop.
pub(crate) root_path: Box<[u8]>,

/// 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`.
Comment on lines +2167 to +2172

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

pub(crate) root_open_path: Box<[u8]>,

pub(crate) pending_err: Option<sys::Error>,
pub(crate) pending_err_mutex: bun_threading::Mutex,
}
Expand All @@ -2190,16 +2198,16 @@ mod _async_tasks {
) -> Option<bun_jsc::Completion<Self>> {
this.done = Some(done);
let mut buf = PathBuffer::uninit();
let root_path_z = {
let root_open_path_z = {
let bytes: &'static [u8] =
// SAFETY: `root_path` is a NUL-terminated `Box<[u8]>` fixed for the
// task's lifetime; `perform_work` mutates other fields only.
unsafe { bun_ptr::detach_lifetime(&this.root_path[..]) };
// SAFETY: `root_open_path` is a NUL-terminated `Box<[u8]>` fixed
// for the task's lifetime; `perform_work` mutates other fields only.
unsafe { bun_ptr::detach_lifetime(&this.root_open_path[..]) };
ZStr::from_buf(bytes, bytes.len() - 1)
};
// May finish synchronously (no subdirectories) or fan out; the last
// subtask finishes the token.
this.perform_work(root_path_z, &mut buf, true);
this.perform_work(root_open_path_z, &mut buf, true);
None
}

Expand Down Expand Up @@ -2374,13 +2382,17 @@ mod _async_tasks {
};
// Subtasks read the root path outside the VM borrow, so it must be an
// owned copy rather than the (possibly JS-backed) argument. NUL-terminated.
let root_path = {
let src = args.path.slice();
let nul_terminated = |src: &[u8]| -> Box<[u8]> {
let mut owned = Vec::with_capacity(src.len() + 1);
owned.extend_from_slice(src);
owned.push(0);
owned.into_boxed_slice()
};
let root_path = nul_terminated(args.path.slice());
let root_open_path = {
let mut buf = paths::path_buffer_pool::get();
nul_terminated(args.path.slice_z(&mut buf).as_bytes())
};
let tracker = AsyncTaskTracker::init(vm);
tracker.did_schedule(global_object);
let promise = JSPromiseStrong::init(global_object);
Expand All @@ -2395,6 +2407,7 @@ mod _async_tasks {
has_result: AtomicBool::new(false),
subtask_count: AtomicUsize::new(1),
root_path,
root_open_path,
result_list,
result_list_count: AtomicUsize::new(0),
result_list_queue: UnboundedQueue::default(),
Expand Down Expand Up @@ -6543,8 +6556,8 @@ impl NodeFS {
};
let utf8_name = current.name.slice();

// The root subtask's basename *is* root_path; the caller passes
// `is_root` explicitly.
// The root subtask's basename is `root_open_path`, which must not
// leak into results; the caller passes `is_root` explicitly.
Comment on lines +6559 to +6560

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

let name_to_copy: &[u8] = if is_root {
utf8_name
} else {
Expand Down
76 changes: 76 additions & 0 deletions src/runtime/node/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -918,6 +918,51 @@ pub(crate) trait PathOrFdExt {
Self: Sized;
}

/// `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.
Comment on lines +921 to +924

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

#[cfg(windows)]
fn is_drive_relative_windows(path: &[u8]) -> bool {
path.len() >= 2
&& bun_paths::is_drive_letter(path[0])
&& path[1] == b':'
&& (path.len() == 2 || !bun_paths::is_sep_any(path[2]))
}

/// Resolve a drive-relative path (see [`is_drive_relative_windows`]) to the
/// absolute path Win32 would open for it, written into `scratch`.
///
/// Node's fs gets this resolution for free because libuv hands the string to
/// Win32, and so do bun's libuv-backed operations. The ones that open through
/// bun's own `NtCreateFile` paths relative to the cwd handle (readdir,
/// recursive rm, writeFile, the mkdir -p existence probe) do 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. Resolving here hands them the same absolute path Win32 would use.
///
/// `None` when Win32 cannot resolve the path; the caller then keeps its
/// existing handling of the path as given.
#[cfg(windows)]
fn resolve_drive_relative_windows<'a>(
path: &[u8],
scratch: &'a mut PathBuffer,
) -> Option<&'a [u8]> {
debug_assert!(is_drive_relative_windows(path));
if !strings::fits_in_wide_path_buffer(path) {
return None;
}
let mut wide_path = bun_paths::w_path_buffer_pool::get();
let mut wide_resolved = bun_paths::w_path_buffer_pool::get();
let resolved = bun_sys::windows::get_full_path_name_w(
strings::paths::to_w_path(&mut wide_path[..], path),
&mut wide_resolved[..],
)?;
// `MAX_PATH_BYTES` is sized so that any wide path re-encodes into a
// `PathBuffer`, so this cannot truncate.
Comment on lines +969 to +970

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

Some(strings::paths::from_w_path(&mut scratch[..], resolved.as_slice()).as_bytes())
}

impl PathLikeExt for PathLike {
// Const-generics can't change return mutability, so this always returns
// `&ZStr`. A future force=true caller that needs `&mut ZStr` will need a
Expand All @@ -930,6 +975,19 @@ impl PathLikeExt for PathLike {

#[cfg(windows)]
{
// A drive-relative path is rebound to the absolute path Win32
// resolves it to, so the drive-absolute 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.
let mut drive_relative_scratch;
let sliced = if is_drive_relative_windows(sliced) {
drive_relative_scratch = bun_paths::path_buffer_pool::get();
resolve_drive_relative_windows(sliced, &mut drive_relative_scratch)
.unwrap_or(sliced)
} else {
sliced
};
// Only take the fast path for paths that can exist on NT at
// all (≤ ~32757 UTF-16 units). That bounds the `\\?\`-prefixed
// copy below in bytes too (≤ 3×32757 + 5 < MAX_PATH_BYTES);
Expand Down Expand Up @@ -1104,6 +1162,24 @@ impl PathLikeExt for PathLike {
let buf_u16 = unsafe { bun_core::bytes_as_slice_mut::<u16>(&mut buf[..]) };
return Ok(strings::to_kernel32_path(buf_u16, normal));
}
if is_drive_relative_windows(s) {
// Same buffer roles as the rooted branch above. The resolved
// path is drive-absolute, so `to_kernel32_path` prefixes it
// with `\\?\` and mkdir -p's NT-level existence probe gets an
// absolute object name. If Win32 cannot resolve it, the path
// is passed on as given, below.
if let Some(resolved) = resolve_drive_relative_windows(s, buf) {
let normal = bun_paths::resolve_path::normalize_buf::<
bun_paths::platform::Windows,
>(resolved, &mut b[..]);
if !strings::fits_in_wide_path_buffer(normal) {
return Err(NameTooLong);
}
// SAFETY: same alignment note as above.
let buf_u16 = unsafe { bun_core::bytes_as_slice_mut::<u16>(&mut buf[..]) };
return Ok(strings::to_kernel32_path(buf_u16, normal));
}
}
// Handle "." specially since normalizeStringBuf strips it to an empty string
if s.len() == 1 && s[0] == b'.' {
// SAFETY: see alignment note above (PathBuffer reinterpreted as [u16]).
Expand Down
87 changes: 87 additions & 0 deletions src/sys/windows/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -983,6 +983,46 @@ pub(crate) fn GetFinalPathNameByHandle(
Ok(ret)
}

/// `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`.
Comment on lines +986 to +992

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

pub fn get_full_path_name_w<'a>(
path: &bun_core::WStr,
out: &'a mut [u16],
) -> Option<&'a bun_core::WStr> {
// SAFETY: `path` is NUL-terminated by `WStr`'s invariant; `out` is valid
// for `out.len()` units, which is the length passed to the API.
let n = unsafe {
externs::GetFullPathNameW(
path.as_ptr(),
out.len() as DWORD,
out.as_mut_ptr(),
ptr::null_mut(),
)
} as usize;
// 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]`.
Comment on lines +1007 to +1009

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

if n == 0 || n >= out.len() {
bun_sys::syslog!(
"GetFullPathNameW({}) = {}",
bun_core::fmt::utf16(path.as_slice()),
n
);
return None;
}
bun_sys::syslog!(
"GetFullPathNameW({}) = {}",
bun_core::fmt::utf16(path.as_slice()),
bun_core::fmt::utf16(&out[..n])
);
Some(bun_core::WStr::from_buf(out, n))
}

const GET_MODULE_HANDLE_EX_FLAG_UNCHANGED_REFCOUNT: DWORD = 0x00000002;
const GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS: DWORD = 0x00000004;

Expand Down Expand Up @@ -2187,4 +2227,51 @@ mod tests {
let prefix: Vec<u16> = "\\Device\\".encode_utf16().collect();
assert!(device.starts_with(&prefix));
}

fn full_path_name(path: &str) -> Option<String> {
let wide: Vec<u16> = path.encode_utf16().chain(core::iter::once(0)).collect();
let mut out = bun_paths::w_path_buffer_pool::get();
let resolved = super::get_full_path_name_w(
bun_core::WStr::from_slice_with_nul(&wide),
&mut out.0[..],
)?;
Some(String::from_utf16(resolved.as_slice()).unwrap())
}

/// A drive-relative name on the cwd's drive resolves against the cwd;
/// the test binary's cwd is always drive-letter rooted.
#[test]
fn full_path_name_resolves_drive_relative_against_cwd() {
let cwd = std::env::current_dir()
.unwrap()
.to_str()
.unwrap()
.to_owned();
let drive = &cwd[..2];
assert_eq!(drive.as_bytes()[1], b':', "{cwd}");
let expected = format!("{}\\x", cwd.trim_end_matches('\\'));
assert_eq!(
full_path_name(&format!("{drive}x")).as_deref(),
Some(&*expected)
);
assert_eq!(
full_path_name(&format!("{}x", drive.to_ascii_lowercase())).as_deref(),
Some(&*expected)
);
assert_eq!(
full_path_name(&format!("{drive}a\\..\\x")).as_deref(),
Some(&*expected)
);
assert_eq!(full_path_name(drive).as_deref(), Some(&*cwd));
}

#[test]
fn full_path_name_rejects_too_small_buffer() {
let wide: Vec<u16> = "C:x".encode_utf16().chain(core::iter::once(0)).collect();
let mut out = [0u16; 4];
assert!(
super::get_full_path_name_w(bun_core::WStr::from_slice_with_nul(&wide), &mut out)
.is_none()
);
}
}
11 changes: 9 additions & 2 deletions src/windows_sys/externs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -985,8 +985,8 @@ pub mod kernel32 {
}
// Re-export externs declared at the crate root so `kernel32::Foo` resolves.
pub use super::{
CreateFileW, GetCurrentDirectoryW, GetFileAttributesW, GetSystemDirectoryW, GetSystemInfo,
SYSTEM_INFO, SetCurrentDirectoryW, SetFilePointerEx,
CreateFileW, GetCurrentDirectoryW, GetFileAttributesW, GetFullPathNameW,
GetSystemDirectoryW, GetSystemInfo, SYSTEM_INFO, SetCurrentDirectoryW, SetFilePointerEx,
};
pub use super::{
GetConsoleCP, GetConsoleMode, GetConsoleOutputCP, SetConsoleCP, SetConsoleMode,
Expand Down Expand Up @@ -1403,6 +1403,13 @@ unsafe extern "system" {

pub fn GetCurrentDirectoryW(nBufferLength: DWORD, lpBuffer: LPWSTR) -> DWORD;

pub fn GetFullPathNameW(
lpFileName: LPCWSTR,
nBufferLength: DWORD,
lpBuffer: LPWSTR,
lpFilePart: *mut LPWSTR,
) -> DWORD;

pub fn GetSystemDirectoryW(lpBuffer: LPWSTR, uSize: DWORD) -> DWORD;

pub fn GetFileAttributesW(lpFileName: LPCWSTR) -> DWORD;
Expand Down
Loading