Skip to content

Build Windows openat object names from NT device paths - #33874

Merged
Jarred-Sumner merged 12 commits into
mainfrom
claude/openat-nt-object-names
Jul 10, 2026
Merged

Jarred-Sumner merged 12 commits into
mainfrom
claude/openat-nt-object-names

Conversation

@dylan-conway

@dylan-conway dylan-conway commented Jul 9, 2026 •

Copy link
Copy Markdown
Member

What

On Windows, opening a path relative to a directory handle (openat with a multi-component subpath — tarball extraction, dirfd-relative node:fs ops, shell file operations) builds one absolute name for NtCreateFile. That name used to be \??\C:\base\rel, with the base resolved via GetFinalPathNameByHandleW(VOLUME_NAME_DOS). This PR switches the base query to VOLUME_NAME_NT and emits the \Device\HarddiskVolumeN\base\rel object name directly.

The drive letter was only ever decoration for the kernel's benefit: the output feeds NtCreateFile exclusively (verified across every caller), and the kernel resolves \??\C: back to the device anyway.

Performance

Every route through this function is faster than or equal to main (release build, Windows 11, 200k iterations):

  • Common shapes (multi-component, dotted, within-tree ..): ~22.1 µs → 12.1–15.1 µs per call (one handle-local query instead of the mount-manager round trip).
  • Relatives that resolve above the directory handle (readdir("..")-class): ~22.1 µs → 17.3 µs — these pay one extra handle-local query, because the volume/mount boundary they need is not derivable from the NT name; the audit of every caller shows only upward directory listings, ..-leading globs, and file:../ monorepo installs take this path.
  • Bare names: syscall-free passthrough (~10 ns), unchanged.

Why

The DOS translation is the one step of this path with real costs:

  • A mount-manager IOCTL per open (IOCTL_MOUNTMGR_QUERY_DOS_VOLUME_PATH, a round-trip through a single device queue). The NT form is answered from the handle itself. Benchmarked function-for-function (release build, Windows 11, 200k iterations on the same directory handle with a lib\mod\file.js-shaped input; the old body reconstructed verbatim from the pre-change commit): ~22.1 µs/op before → 12.1–15.1 µs/op after, 32–45% faster. The rare .. path — a second handle query plus the containment walk — costs ~17.3 µs/op, still below the old fast path; the bare-name passthrough is syscall-free at ~10 ns. Traced during a 200-file tarball install: 262 mount-manager IOCTLs before, 6 after (the remainder come from fd→path string helpers, not this path).
  • It doesn't exist everywhere. Volumes mounted without the mount manager (ImDisk-style ramdisks) have no DOS name — uv_fs_realpath's libuv docs and std::fs::canonicalize uses GetFinalPathNameByHandleW, which can fail rust-lang/rust#79449 document the failure class — and sandboxed (AppContainer) tokens are denied the mount-manager query outright (see Fix WeaklyCanonicalPath ERROR_ACCESS_DENIED in Windows AppContainers microsoft/onnxruntime#28509 for the same diagnosis). Those cases previously failed with EBADF; they now open fine.
  • It re-resolved the drive letter through the caller's device map on every open. Per-session subst/net use shadowing a global letter — including via an impersonated token's device map — could silently retarget an fd-relative open to a different volume after the handle was opened. The NT form pins the volume the handle actually lives on, which is what openat semantics require.

Bugs fixed (relative to main)

  1. Dirfd-relative opens failed on volumes without DOS names. Ramdisk-style volumes mounted without the mount manager have no drive letter to resolve, so every multi-component relative open returned EBADF. They now open normally.
  2. Drive-letter remapping could silently retarget relative opens. The old \??\C: form re-resolved the letter through the caller's device map on every open, so a per-session subst/net use shadowing the letter after a directory handle was opened (including via an impersonated token's device map) pointed later fd-relative opens at a different volume. Opens are now pinned to the volume the handle actually lives on.

How

  • The second query (the handle's volume-relative name, FileNameInformation) runs only when the relative part actually resolves above the directory handle — a single-pass classifier reports whether the running component depth ever goes negative; within-tree .. like a\..\b provably never reaches any clamp floor and takes the single-query path with identical output.
  • When it does run, the volume prefix is the kernel's own two answers subtracted — full NT object name minus volume-relative name, the same technique YY-Thunks uses — so no device-name shapes are assumed (nested device names and session-qualified redirector components are handled by construction). For Mup/redirector devices the .. clamp extends past \server\share, matching how the old drive-letter form clamped at the share. The prefix is copied verbatim and only the remainder is normalized; the shared normalizer is untouched, since it serves user-facing path semantics where \Device must not be special.
  • open_file_at_windows_nt_path decided RootDirectory by testing for a literal \??\ prefix; it now uses the same leading-separator rule as its dir/stat siblings.

Behavior notes

  • The .. climb floor is now structural: the volume prefix (share root, for UNC) is copied outside the normalizer's reach, so excess .. can never eat into the device/server/share components. On main, the shared normalizer's floor was absent for UNC prefixes in this branch — not reachable from user-supplied paths (those are collapsed lexically upstream), but internal .. uses are now clamped by construction, and a boundary that can't be established fails loud instead of guessing.
  • On devices other than the allowlisted share-rooted redirectors (Mup, LanmanRedirector), a .. that would cross the clamp floor now fails with EINVAL instead of silently clamping: a single device can multiplex many mounts (WinFsp-style sshfs/rclone, WebDAV), so clamping at the device root could resolve into a sibling mount. The same applies to folder-mounted volumes — .. across the mount root previously followed the drive-letter parent, which cannot be derived from the handle without the mount manager, so it now fails loudly rather than guessing. Win32 consumers of the flag-selected output receive the \\?\GLOBALROOT spelling rather than a raw NT name.
  • The subtraction boundary is only accepted when it lands on a real \Device\<name> prefix, so a rename racing the two name queries can never shear the .. clamp above the device (deeper shears only tighten it). Path-vs-open TOCTOU beyond that is inherent to every path-based syscall and unchanged from main.
  • Failed opens on network-redirector bases can surface ERROR_BAD_NETPATH vs ERROR_BAD_NET_NAME shades differently, since explicit-redirector names skip MUP prefix resolution.
  • Trailing dots/spaces, case handling, reparse traversal, ACLs, and path-length limits are identical for both spellings (stripping is a Win32-layer behavior; neither spelling passes through Win32). A 377-char path opens successfully via the new form.
  • The file and directory open helpers previously used different absoluteness rules for RootDirectory (literal \??\ prefix vs leading separator); they now share the leading-separator rule. Not user-observable — no caller delivers a rooted path here un-absolutized — but the two paths no longer disagree.

Testing

  • Unit tests for the clamp boundary (local volume, volume root, nested device names, Mup share, session-qualified redirector shapes, and the guard fallbacks) and the normalize output (multi-component, .. into parent, excess-.. clamp, ., bare component passthrough, absolute branch unchanged), plus an integration test that opens and reads a file through open_file_at_windows_a with the new name shape.
  • Drove the built binary end to end: 200-file tarball install with content assertions, shell dotted-path reads, and NtTrace capture showing the extraction opens carrying \Device\… names.
  • test/js/node/fs and test/js/bun/shell suites: every failure reproduces byte-identically on an unmodified main build of the same machine (pre-existing local issues).

Dirfd-relative multi-component opens used to resolve the base directory
handle to its DOS path (GetFinalPathNameByHandleW + VOLUME_NAME_DOS) and
emit `\??\C:\base\rel` for NtCreateFile. The DOS translation costs a
mount-manager IOCTL per open, is denied to sandboxed (AppContainer)
tokens, and does not exist on volumes mounted without the mount manager.

Query VOLUME_NAME_NT instead — answered from the handle itself — and
emit the `\Device\...\base\rel` object name directly. The volume prefix
(two components, or server\share for Mup/redirector devices, skipping
session-qualified `;` components) is copied verbatim and only the
remainder is normalized, so `..` clamps at the volume root exactly as
the drive-letter form clamped at `C:\`. The RootDirectory discriminator
in open_file_at_windows_nt_path now matches its dir/stat siblings
(leading separator = absolute object name) instead of testing for a
literal `\??\` prefix.

Also pins fd-relative opens to the volume the handle actually lives on:
the old form re-resolved the drive letter through the caller's device
map on every open, so a per-session subst/net-use shadowing the letter
(including via impersonation) could silently retarget relative opens.

Covers the new prefix logic with unit tests, including bare-device,
volume-root, Mup share, and session-qualified redirector shapes.
@robobun

robobun commented Jul 9, 2026 •

Copy link
Copy Markdown
Collaborator

@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Found 4 issues this PR may fix:

  1. Error running on ramdisk in Windows system #26192 - ImDisk ramdisk volumes lack DOS drive-letter mappings; switching to NT device paths fixes the ENOENT on volumes without DOS names
  2. Bun broken with subst (Windows) #20568 - subst virtual drives re-resolve through the caller's device map with DOS-form paths, causing path mismatch assertions; NT-form pins the actual volume
  3. Bun can't find workspace dependencies when using DevDrive mounted as folder #10011 - DevDrive mounted as a folder exposes the underlying volume letter via DOS-form resolution, causing workspace dependency path mismatches
  4. Windows: bun link fails on mapped drives #19575 - Mapped network drives go through the device map for DOS name resolution; NT paths avoid the mount-manager IOCTL that can fail for network-mapped volumes

If this is helpful, copy the block below into the PR description to auto-close these issues on merge.

Fixes #26192
Fixes #20568
Fixes #10011
Fixes #19575

🤖 Generated with Claude Code

@coderabbitai

coderabbitai Bot commented Jul 9, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Changes

Windows dirfd-relative normalization now produces NT device paths while preserving device and share roots during . and .. resolution. NT path opening recognizes absolute Windows path forms, and Windows-only tests cover normalization, clamping, prefix boundaries, and NT object-name access.

Windows NT path handling

Layer / File(s) Summary
Relative path classification
src/paths/classify.rs, src/paths/lib.rs
A public single-pass classifier reports separators, dots, and whole .. components for Windows and POSIX path formats, with unit coverage for edge cases.
NT prefix rules
src/sys/lib.rs
Documentation distinguishes absolute-input prefixing from dirfd-relative resolution, and nt_clamp_prefix_len() identifies boundaries for local, UNC, and session-qualified NT prefixes.
Relative NT resolution
src/sys/lib.rs, src/sys/windows/mod.rs, src/windows_sys/externs.rs
A volume-relative handle-name mode supports traversal detection and resolution while preserving the NT device prefix during normalization.
NT opening and validation
src/sys/lib.rs
NT absolute paths determine RootDirectory handling through Windows absolute-path detection, with tests covering path shapes, root clamping, prefix boundaries, and NT object-name access.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
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.
Title check ✅ Passed The title clearly summarizes the main change: building Windows openat object names from NT device paths.
Description check ✅ Passed The description covers what the PR does and how it was verified, with substantial implementation and testing detail.

Comment @coderabbitai help to get the list of available commands.

@claude claude Bot left a comment

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.

I didn't find any bugs, but this reworks Windows dirfd-relative path resolution and .. clamping in the core syscall layer — security-sensitive enough that a human should sign off.

Extended reasoning...

Overview

This PR changes how normalize_path_windows builds NT object names for NtCreateFile when opening a path relative to a directory handle on Windows. Instead of resolving the base handle to a DOS path (\\??\\C:\\…) via GetFinalPathNameByHandle(VOLUME_NAME_DOS), it now requests VOLUME_NAME_NT and emits \\Device\\HarddiskVolumeN\\… directly. It adds a new nt_volume_prefix_len helper to identify the volume boundary (with special-casing for Mup/LanmanRedirector UNC devices and session-qualified ;… components), copies that prefix verbatim, and normalizes only the remainder so .. clamps at the volume/share root. It also swaps the literal \\??\\ prefix check in open_file_at_windows_nt_path for the general is_absolute_windows_wtf16 predicate, and adds ~200 lines of Windows-gated unit/integration tests.

Security risks

Path normalization for dirfd-relative opens is squarely in path-traversal territory. The correctness of .. clamping now depends on nt_volume_prefix_len correctly identifying the boundary for every device-name shape Windows can return (local volumes, MUP, LanmanRedirector, session-qualified redirectors, and anything else). This code path feeds tarball extraction during bun install, so a mis-computed prefix boundary on some volume type could plausibly change where extracted files land. The buffer-bounds arithmetic and the broadened absolute-path predicate for RootDirectory selection are also worth a second pair of eyes. The reasoning in the PR description is thorough and the unit tests cover the enumerated shapes, but this is exactly the kind of change where a subtle miss has outsized consequences.

Level of scrutiny

High. src/sys/lib.rs is the cross-platform syscall wrapper layer that essentially everything file-related on Windows funnels through, and the change is a semantic redesign (different name shape, different clamping mechanism, documented behavior differences for UNC excess-.. and network error codes) rather than a mechanical refactor.

Other factors

The PR is well-researched and well-tested — the new Rust unit tests exercise the prefix-boundary shapes, .. clamping, and an end-to-end NtCreateFile open, and the description documents NtTrace verification and full fs/shell suite runs. No bugs surfaced in automated review. That said, the change is ~250 net lines in a critical Windows-only path with acknowledged behavior deltas, so it warrants a human reviewer with Windows internals context rather than bot approval.

Replace the component-counting volume-prefix scan with the subtraction
technique: the handle's full NT object name minus its volume-relative
name (FileNameInformation) is the device prefix, with no assumptions
about device-name shapes — nested names like
\Device\HarddiskDmVolumes\<x>\Volume1 and session-qualified redirector
components are handled by construction, since the kernel itself places
them on the device side. Mup/LanmanRedirector prefixes still extend
past server\share so the `..` clamp keeps share-root parity.

The volume-relative query only runs when the relative part actually
contains a `..` component — without one, any split point yields the
same output, so the common case (tarball extraction, plain subpaths)
stays at a single handle query. When `..` is present and the two names
do not compose (racing rename), the open fails with EBADF instead of
guessing a boundary that could resolve `..` to the wrong file.
Comment thread src/sys/lib.rs Outdated
The bare-component routing check and the `..` detection both walked the
relative part; fuse them into one component-aware pass that answers
both, exiting early only once a `..` component is confirmed (which
settles both answers). The drive-letter strip moves ahead of the scan
so `C:..\x` is inspected post-strip; the bypass still copies the
original path verbatim.

@coderabbitai coderabbitai Bot left a comment

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.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/sys/lib.rs (1)

6709-6754: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Strip C: in the passthrough branch. C:foo is classified as a drive-relative bare component, but this branch still copies path, so the colon survives into NtCreateFile and changes the object name being opened. Copy rel here and add a regression for bare drive-relative input.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/sys/lib.rs` around lines 6709 - 6754, In the no-separator passthrough
branch, copy the drive-letter-stripped `rel` slice rather than the original
`path`, while preserving length checks and null termination. Add a regression
test covering bare drive-relative input such as `C:foo`, verifying that
`NtCreateFile` receives `foo` without the `C:` prefix.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@src/sys/lib.rs`:
- Around line 6709-6754: In the no-separator passthrough branch, copy the
drive-letter-stripped `rel` slice rather than the original `path`, while
preserving length checks and null termination. Add a regression test covering
bare drive-relative input such as `C:foo`, verifying that `NtCreateFile`
receives `foo` without the `C:` prefix.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 52ecc17e-67e5-43e4-95ef-b39e521df084

📥 Commits

Reviewing files that changed from the base of the PR and between b30b6e1 and 69199dd.

📒 Files selected for processing (1)
  • src/sys/lib.rs

Since the whole-base fast path, a relative part that normalizes to
nothing (".", "./") left `joined` as a bare separator, so the result
grew a trailing `\` the drive-letter form never produced. Drop the
lone-separator suffix when the prefix is the base directory's full
path; keep it when the prefix is a bare device name (volume-root
bases, excess-`..` clamps), where the separator selects the root
directory over the volume device.
@dylan-conway

Copy link
Copy Markdown
Member Author

Investigated all four before adding any auto-close lines — none meet the bar:

Leaving the description without Fixes lines; #26192 is referenced as related in the follow-up plan.

@coderabbitai coderabbitai Bot left a comment

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.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/sys/lib.rs (1)

6749-6757: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Copy rel in the bare-name fast path. Drive-relative names like C:foo still hit this branch, so copying path preserves the stripped drive prefix and passes C:foo through to NtCreateFile instead of foo. Switching this branch to rel keeps it aligned with the .. case; update the nearby comment to match.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/sys/lib.rs` around lines 6749 - 6757, In the bare-name fast path, copy
the drive-stripped `rel` slice instead of `path` so drive-relative inputs such
as `C:foo` pass only `foo` to NtCreateFile. Update the nearby comment to
describe copying `rel`, while preserving the existing length check, NUL
termination, and WStr construction.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@src/sys/lib.rs`:
- Around line 6749-6757: In the bare-name fast path, copy the drive-stripped
`rel` slice instead of `path` so drive-relative inputs such as `C:foo` pass only
`foo` to NtCreateFile. Update the nearby comment to describe copying `rel`,
while preserving the existing length check, NUL termination, and WStr
construction.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 4cc9805a-a51b-4e35-af25-925bc5e5bddf

📥 Commits

Reviewing files that changed from the base of the PR and between 69199dd and def741b.

📒 Files selected for processing (1)
  • src/sys/lib.rs

Move the fused routing/dotdot scan out of normalize_path_windows_opts
into bun_paths::classify_rel_t, returning per-fact booleans (any
separator, any dot, whole `..` component) from a single walk. The
early exit fires only once every field is final, so a bare trailing
`..` still reports has_sep honestly. Covered by a platform-independent
test matrix over both element widths, including the POSIX cases where
a backslash is part of the component.
@dylan-conway

Copy link
Copy Markdown
Member Author

Re: both suggestions to copy the drive-stripped rel in the bare-name passthrough (C:foo → foo) — investigated and deliberately not applying:

  1. The shape is unreachable from user code. The JS layer absolutizes drive-relative paths before they reach native (fs.readFileSync("C:file.txt") resolves to <cwd>\file.txt and works — verified on this build and on canary). The passthrough only ever sees such a string from a hypothetical internal caller, and none exists.
  2. The proposed behavior would be a third semantics, not a fix. Win32 treats C:foo as drive-relative (resolved against the per-drive current directory); the NT-relative passthrough on main treats the colon per NTFS stream syntax. Stripping C: and opening foo under the dirfd matches neither — it would silently reinterpret the drive qualifier as decoration.
  3. This PR intentionally preserves main's passthrough byte-for-byte (the scan changes never altered what gets copied). Changing it belongs in its own discussion if a real caller ever appears.

@coderabbitai coderabbitai Bot left a comment

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.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/sys/lib.rs (1)

6709-6736: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Drive-relative inputs still bypass the rel normalization here. C:foo is a drive-relative path on Windows, not an ADS name, and NtCreateFile won’t resolve it relative to RootDirectory for you. This fast path should copy rel here too, or reject drive-relative inputs so dir_fd semantics match the slow path.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/sys/lib.rs` around lines 6709 - 6736, Fix the no-separator fast path in
the drive-relative handling by ensuring inputs like C:foo do not bypass
normalization: copy the stripped rel slice rather than the original path, or
explicitly reject drive-relative paths so dir_fd behavior matches the slow path.
Update the length check, copy, and resulting WStr length consistently in this
branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@src/sys/lib.rs`:
- Around line 6709-6736: Fix the no-separator fast path in the drive-relative
handling by ensuring inputs like C:foo do not bypass normalization: copy the
stripped rel slice rather than the original path, or explicitly reject
drive-relative paths so dir_fd behavior matches the slow path. Update the length
check, copy, and resulting WStr length consistently in this branch.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 615c10f7-f69d-4ee9-963f-a0ae84250e2f

📥 Commits

Reviewing files that changed from the base of the PR and between def741b and b929ce7.

📒 Files selected for processing (3)
  • src/paths/classify.rs
  • src/paths/lib.rs
  • src/sys/lib.rs

- Honor add_nt_prefix=false in the dirfd-relative branch: Win32
  consumers get the \?\GLOBALROOT spelling of the same object instead
  of a debug_assert (release builds previously emitted \Device\ names
  that kernel32 silently drive-roots; reachable via the async fs.cp
  directory path with a relative destination).
- Fail loud when a `..` component would cross the clamp floor on a
  device that is not an allowlisted share-rooted redirector: one
  device can multiplex many mounts (WinFsp-style), so clamping at the
  device root could resolve into a sibling mount. Mup/LanmanRedirector
  keep the silent share-root clamp.
- Restrict the null-RootDirectory decision in the three NtCreateFile
  openers to the shapes our producers emit (\??\ and \Device\ prefixes,
  one shared predicate) so other rooted strings no longer resolve in
  the raw object namespace.
- Surface NameTooLong from the base queries as ENAMETOOLONG instead of
  EBADF, and make server-only UNC volume-relative names fail closed
  with or without a trailing separator.
- Test matrix for the uncommon shapes: volume-root bases, redirector
  and lookalike device names, buffer-boundary lengths, non-file dirfds,
  drive-qualified passthrough, and classifier component-edge cases.
cargo test for bun_sys and bun_paths never linked: the crates reference
mimalloc, simdutf, highway, libuv and a handful of runtime symbols that
only the full binary link provides. Add a dev-only bun_test_native_link
crate whose build script archives the already-built libuv objects from
build/debug and provides the remaining symbols as shims — functional
Rust implementations where tests actually execute them (allocator,
transcoding, scalar string classification), loud panic stubs for the
WTF/JSC-backed surfaces tests must never reach. Wired in as a dev
dependency of the two crates only; the product build is unchanged.

Also initializes cached stdio in the one test that relied on the
binary's startup having done it.
The subtraction boundary must be a real device prefix: accept only
\Device\<name…> in the non-allowlisted arm, so a rename landing between
the two name queries can no longer shear the boundary to bare \Device
and hand `..` a forged depth budget (deeper shears only tighten the
clamp and stay accepted). The floor-crossing refusal now surfaces as
EINVAL — it is a path-shape decision, not descriptor state; the two
name-query failures keep EBADF.

Leading-colon components flow through again instead of tripping a
ported sanity assert: the dirfd join can legitimately collapse to a
`\:\…` object name, which NtCreateFile rejects with a proper error, so
the output-side debug assert in the normalizer is removed (the
input-side caller-mistake assert stays) and stream-name shapes behave
as before throughout.

Tests derive the clamp floor behaviorally instead of counting device
components, and cover forward-slash traversal, exact-floor landings,
dot-component neutrality, and the colon flow-through matrix. All of
them execute now.
@dylan-conway

Copy link
Copy Markdown
Member Author

Re the repeated bare-name passthrough suggestion (third occurrence): see the investigation above (#33874 (comment)) — the shape is unreachable from user code and the proposed change would introduce a third semantics matching neither Win32 nor main. The asymmetry is now pinned by the drive_qualified_bare_name_passes_through test and documented on the function.

@claude claude Bot left a comment

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.

The trailing-separator fix in def741b looks right (and the whole_base gate correctly preserves the load-bearing \ on bare device names). No new issues found on this revision, but the core change — NT device-path construction for every dirfd-relative open, new ..-clamp/floor semantics, and the ~800-line bun_test_native_link shim crate — is substantial enough that someone familiar with the Windows syscall layer should sign off.

Extended reasoning...

Overview

This PR reworks how Windows openat builds object names for NtCreateFile: instead of resolving the base directory handle to a DOS drive-letter path (\\??\\C:\\…) via a mount-manager IOCTL, it queries the NT device path (\\Device\\HarddiskVolumeN\\…) directly and constructs the object name from that. The change spans src/sys/lib.rs (normalize_path_windows_opts, nt_clamp_prefix_len, is_nt_object_name, and the RootDirectory selection in three NtCreateFile wrappers), a new one-pass path classifier in src/paths/classify.rs, a VolumeName::None variant in src/windows_sys/externs.rs, a removed debug assertion in src/paths/resolve_path.rs, and a new src/test_native_link crate (~810 lines of dev-only native-symbol shims + a build script that archives prebuilt libuv objects) so cargo test -p bun_sys/bun_paths link standalone. ~600 lines of new Windows-gated unit tests cover the clamp boundary, normalization output shapes, and end-to-end NtCreateFile opens.

Security risks

Path handling for .. traversal is the security-relevant surface here. The PR tightens it in most respects: the device/share prefix is copied outside the normalizer's reach so excess .. structurally cannot eat into \\Device\\… or \\server\\share; on non-allowlisted devices a .. that would cross the floor now returns EINVAL instead of silently clamping; and the subtraction boundary is only accepted when it lands on a real \\Device\\<name> prefix (guarding against rename-race shears). It also removes the drive-letter re-resolution TOCTOU where a per-session subst could retarget an fd-relative open. That said, the nt_clamp_prefix_len allowlist (Mup, LanmanRedirector) and the depth-walk in normalize_path_windows_opts are new, non-trivial logic on a path that every dirfd-relative Windows open flows through — the reasoning is well-documented and unit-tested, but it warrants human eyes.

Level of scrutiny

High. This is core Windows syscall-path infrastructure: normalize_path_windows feeds every multi-component openat (tarball extraction, node:fs dirfd ops, shell file operations). The PR also changes user-observable behavior in edge cases (.. across a folder-mount root now fails EINVAL instead of following the drive-letter parent; network-redirector error shades may differ), removes a debug_assert! in the shared normalizer, and introduces a whole new dev-only crate with a build script and hundreds of lines of #[no_mangle] allocator/simdutf/highway shims. None of this is mechanical.

Other factors

I flagged one bug on the previous revision (trailing \\ when rel collapses to nothing); the author fixed it in def741b with a more precise gate than I suggested (whole_base distinguishes directory-path prefixes from bare device names, where the trailing separator selects root-directory vs. volume-device) and pinned both sides with tests. The author also gave detailed, well-reasoned responses to the auto-issue-finder and the C:foo passthrough suggestion. Test coverage for the new logic is thorough. The bug-hunting pass on the current revision found nothing. Still, the scope, the behavioral changes noted in the PR description, and the new test-link infrastructure put this well outside the auto-approval bar.

…ocally"

This reverts commit ba6415a900686e0212b52a12ffb62b56900e78a9 and removes
the test module registrations that referenced it. The local test-linking
infrastructure does not belong in this change.
Comment thread src/paths/resolve_path.rs Outdated
Comment thread src/sys/lib.rs
// device roots: copy the device prefix (the NT object name minus the
// volume-relative name) verbatim and normalize only the remainder.
let mut rel_buf = bun_paths::w_path_buffer_pool::get();
let vol_rel = match windows::GetFinalPathNameByHandle(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Two system calls here? Do we really need this?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Narrowed in 05b7c5f. The second call was already gated to ..-containing paths (never the hot path), and it's now confined to the minimal set: paths that resolve above the directory handle (readdir("..") and friends). Everything else — multi-component opens, dotted names, within-tree .. like a\..\b — is exactly one handle query; the within-tree case can't reach any clamp floor because the base's NT name is kernel-normalized, so no boundary is needed and the output is provably identical.

For the residue it can't be removed: resolving .. above the handle needs to know where the volume (or network mount) root sits, and that boundary isn't derivable from the NT name alone — device prefixes aren't a fixed shape (\Device\HarddiskDmVolumes\<x>\Volume1, and WinFsp-style redirectors multiplex many mounts under one device, where a guessed floor would let excess .. walk into a sibling mount). FileNameInformation is the one handle-local, mount-manager-free source of that fact. Verified at the fs surface: readdirSync("a/../b") issues zero volume-relative queries, readdirSync("..") exactly one.

@Jarred-Sumner Jarred-Sumner left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can we avoid the added syscall?

dylan-conway and others added 3 commits July 10, 2026 10:18
The second GetFinalPathNameByHandle call exists to place the clamp
floor, and the floor is only reachable when the relative part resolves
above the directory handle itself: the base suffix is kernel-normalized
so within-tree `..` (a\..\b) can never dip below its own start. Teach
classify_rel_t to report whether the running component depth ever goes
negative, computed in its existing single pass, and gate the second
query, the boundary subtraction, and the floor walk on that instead of
mere `..` presence. Multi-component opens, dotted names, and within-tree
`..` now cost exactly one handle query; only paths that resolve above
the directory (readdir("..") and friends) pay the second, where the
volume boundary is genuinely required and cannot be derived from the
NT name alone.

Also drops a leftover comment in the normalizer from the earlier assert
removal.
The gate switch left RelPathFacts.has_dotdot_component with no
production reader — remove the field (the depth scoring it shared with
climbs_above_start stays). Add normalize-level tests for the fast
path's collapsed-to-base shape and for a climbing relative with a
leading name component, which keeps the floor walk covered now that
within-tree dotdots skip it.
@robobun

robobun commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

Merged main (85aa006) to pick up ed4a810 (#33898).

The Windows hard failure on the previous build (71563, postgres-binary-array-bounds.test.ts with ERR_POSTGRES_CONNECTION_REFUSED) was the ephemeral-port exhaustion that #33898 fixed on main; this branch was based on fc865b3 which predates it. Every build lacking that commit shows the same cluster (postgres-invalid-message-length, trace-events, node-tls-connect, res.send) on shard 1/8, and every build including it is clean. The remaining flakes in that run (bun-install.test.ts EBADF, serve.test.ts EBADF, dev-and-prod, fetch-backpressure OOB, test-repl-close, napi gcUntil, bun-install-registry peer-*, nodemailer secrets-API) are all pre-existing and hit many unrelated PR branches; none are related to this diff.

@Jarred-Sumner
Jarred-Sumner merged commit c773f88 into main Jul 10, 2026
68 of 79 checks passed
@Jarred-Sumner
Jarred-Sumner deleted the claude/openat-nt-object-names branch July 10, 2026 22:43
robobun added a commit that referenced this pull request Jul 10, 2026
#33874 removed the debug_assert block that consumed `result`, leaving a
clippy::let_and_return violation that every open PR now trips. The code
is identical on main; this is merge hygiene.
Jarred-Sumner pushed a commit that referenced this pull request Jul 11, 2026
…frames) (#33951)

## Problem

`cargo clippy` fails on `main`, and therefore on **every open pull
request that touches a `.rs` file**. There are *two* breakages, stacked
— the second was hidden because `bun_runtime` depends on `bun_paths`, so
it was never checked while `bun_paths` failed to compile.

### 1. `bun_paths` — `clippy::let_and_return`

```
error: returning the result of a `let` binding from a block
    --> src/paths/resolve_path.rs:1148:5
```

#33874 removed the `debug_assert` that used to sit between the binding
and the return, leaving `let result = ...;` followed by `result`.

### 2. `bun_runtime` — `clippy::large_stack_frames`

```
error: this function may allocate 221049 bytes on the stack
  --> src/runtime/jsc_hooks.rs:301:11
```

Introduced by #31216. The frame is `bun_bundler::Transpiler::init`'s
return value: a `Transpiler` carries a `Resolver` whose
`BSSMapInner<DirInfo, 2048>` is ~229 KB, and it is returned by move then
`ptr::write`n straight into `(*vm).transpiler`. Optimized builds elide
the copy; the function runs once per VM init. Only Linux crosses the 128
KB `stack-size-threshold` from `clippy.toml`, which is why local macOS
clippy never flagged it.

## Why neither was caught

`.github/workflows/clippy.yml` triggers on `workflow_call`,
`workflow_dispatch`, `pull_request` and `merge_group` — there is **no
`push:` trigger**, so **main never lints itself**. Because
`pull_request` lints the PR merged into main, both breaks surface on
contributors' PRs instead, attributed to their commits.

## Fix

1. Return the slice directly. The binding had no other use.
2. Suppress `large_stack_frames` on `init_runtime_state` with the reason
recorded, exactly as `find_context_identifiers::walk_expr` already does.
`#[allow]` rather than `#[expect]` because the lint does not fire on
macOS, where an expectation would go unfulfilled. Removing the temporary
for real needs an in-place `Transpiler::init_in_place(dst, ..)` — a
larger change owned by the bundler, and worth doing separately now that
every Worker thread runs this init.

## Verification

- Reproduced **both** failures on a clean `main` tip before fixing;
`cargo clippy -p bun_paths --no-deps` is clean after (1), and `cargo
check -p bun_runtime` + `cargo fmt --check` are clean after (2). CI's
ubuntu clippy job is the oracle for (2), since macOS never fires it.
- Built `bun-debug` and drove the surface `normalize_string_generic_tz`
actually backs, against a built node v26.3.0 — identical on every case:
  ```
  a/b/../c            => "a/c"
  /a/./b//c/          => "/a/b/c/"
  ../../x             => "../../x"
  resolve             => "/foo/bar/baz"
  resolve-up          => "/foo/baz"
  win32 normalize     => "C:\a\c"
  win32 UNC resolve   => "\\server\share\b"
  normalize("")       => "."
  normalize(".")      => "."
  normalize("/")      => "/"
  ```
- `bun bd test test/js/node/path/` → 122 pass, 2 skip, 0 fail. All 16
vendored `test-path*.js` pass.

## Notes

A workspace-wide `cargo clippy --workspace --no-deps --keep-going` also
reports `derivable_impls` for `Sendfile` in
`src/runtime/server/FileResponseStream.rs`, but only on macOS:
`socket_fd: Fd::INVALID` is a Linux/Android-gated field that is not
`Default`, so the impl is genuinely not derivable on the Ubuntu runner.
Left alone.

Adding a `push:` trigger to `clippy.yml` would stop this class of
breakage from being invisible on main. Happy to do that here or in a
follow-up.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants