windows: recognize reserved DOS device names before NtCreateFile - #34770
windows: recognize reserved DOS device names before NtCreateFile#34770robobun wants to merge 21 commits into
Conversation
…spaces in NtCreateFile paths
|
Updated 5:01 AM PT - Jul 20th, 2026
✅ @robobun, your commit de663fc466304f6fb7effad8ee69605d53c4bf79 passed in 🧪 To try this PR locally: bunx bun-pr 34770That installs a local version of the PR into your bun-34770 --bun |
|
Verified on Windows Server 2019 x64 (debug build at 53a19fb): The Diff is now 3 files: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughAdds Windows reserved DOS device-name recognition, integrates it into Win32 path normalization and NT device resolution, removes the directory-opening fast path, and adds Rust and JavaScript coverage. ChangesWindows path handling
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
…eplace flaky pipe test with os.devNull coverage
…xt] does not disagree with the file written to disk
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/bundler/bundle_v2.rs`:
- Around line 4210-4216: Extend the trailing-character stripping in the
bundle_v2 path normalization loop at src/bundler/bundle_v2.rs:4210-4216 to
remove both dots and spaces, matching normalize_path_windows_opts. Apply the
same change to rel_path handling before POSIX conversion at
src/bundler/linker_context/generateChunksInParallel.rs:369-378, preserving
parity between written paths and recorded output/import paths.
🪄 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: a2c39e14-a785-4bf3-96af-a79a1c1cee08
📒 Files selected for processing (5)
src/bundler/bundle_v2.rssrc/bundler/linker_context/generateChunksInParallel.rssrc/paths/lib.rssrc/sys/lib.rstest/js/node/fs/fs-windows-dos-device-names.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/paths/lib.rs`:
- Around line 108-111: Condense the comments at src/paths/lib.rs lines 108-111,
src/bundler/bundle_v2.rs lines 4210-4215, and
src/bundler/linker_context/generateChunksInParallel.rs lines 372-376 so each is
no more than three lines, preserving the existing rationale and behavior without
changing code.
🪄 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: 9e294bdf-623e-4642-8d04-7f423f2328e5
📒 Files selected for processing (5)
src/bundler/bundle_v2.rssrc/bundler/linker_context/generateChunksInParallel.rssrc/paths/lib.rssrc/sys/lib.rstest/js/node/fs/fs-windows-dos-device-names.test.ts
…t consistency); handle bare \\?\<name> and /->\ in \\.\ output
There was a problem hiding this comment.
No new findings after de663fc — all earlier inline comments are addressed or explicitly deferred to #33034. Leaving the merge call to a human since this rewrites the normalize_path_windows_opts prefix classification that every Windows openat goes through, removes the \\.\ → CreateFileW fallback in open_dir_at_windows_nt_path, and intentionally diverges from Node's behaviour.
What was reviewed:
win32_normalizesgate traced for each path shape (Relative/Rooted/Drive*,\??\, UNC,\\.\,\\?\X:,\\?\UNC\,\\?\Volume{…}, bare\\?\<name>) against the unit-test matrix.- Callers of
open_dir_at_windows_nt_path— all reach it vianormalize_path_windowsorto_nt_path, neither of which emits\\.\now, so the removed CreateFileW arm is dead. windows_reserved_device_name_trecogniser: trailing-./handling, near-misses, non-ASCII rejection, u8/u16 parity.- Remaining known gaps (libuv-routed ops for drive-absolute device paths;
nul\.) are pre-existing and called out for #33034.
Extended reasoning...
Overview
Three files: src/paths/lib.rs adds windows_reserved_device_name_t (a comptime_string_map-backed recogniser generic over u8/u16) with cross-platform unit tests; src/sys/lib.rs rewrites the DOS-device / \\.\ / \\?\ handling at the top of normalize_path_windows_opts, deletes open_windows_device_path and the \\.\ → CreateFileW fallback in open_dir_at_windows_nt_path, and adds a Windows-only unit-test module; test/js/node/fs/fs-windows-dos-device-names.test.ts is new end-to-end coverage. Earlier iterations also touched the bundler and added trailing-dot/space stripping — both were reverted after review, so the diff is now device-names only.
Security risks
None identified. The change narrows the set of inputs that reach NtCreateFile as literal filenames (fewer ways to create Explorer-undeletable files), and the exemption for UNC/LocalDevice/NT-object paths means named pipes and SMB shares are not silently redirected. No user input flows into a new allocation or index computation without a bounds check (buf.len() <= total guards the device-prefix copy).
Level of scrutiny
High. normalize_path_windows_opts sits on every Windows file-open path, the new win32_normalizes classification has to correctly distinguish six Win32 path types, and the PR removes a fallback (CreateFileW for \\.\) whose absence would break os.devNull if the new \??\ emission were wrong. It also deliberately diverges from Node, which the description justifies but which is a maintainer-level call.
Other factors
This PR has been through five prior review rounds; every substantive finding (write/stat inconsistency from trailing-dot stripping, untested bundler output change, stale comments, over-length comment blocks) was either fixed or reverted, and the two acknowledged non-blocking gaps are documented in the description with a follow-up issue. The author reports the Windows integration tests and fs-mkdir pass on a debug build. Windows CI for the current head was still building at review time, and I cannot execute the Windows-gated unit tests locally, so I'm deferring rather than approving.
On Windows,
fs.writeFileSync("nul", data)created a literal file namednulin the current directory instead of writing to the null device. The same happened forNul,con,aux,prn,com1,lpt1, and relative paths likesub\nul. The resulting files cannot be opened or deleted from Explorer or cmd without a\\?\prefix.Cause
writeFile/appendFileopen throughbun_sys::openat->normalize_path_windows->NtCreateFile, which resolves names in the NT object namespace and knows nothing about DOS device names. The only existing special case matched an absolute path whose last four code units were\nulor\NUL, so a barenul, mixed-caseNul, forward-slashsub/nul, and every other reserved name reachedNtCreateFileverbatim.Fix
normalize_path_windows_opts(src/sys/lib.rs) now classifies the input the wayRtlGetFullPathName_Udoes and, for Relative / Rooted / DriveRelative / DriveAbsolute inputs (plus\\?\X:, sinceslice_z_with_force_copyhands every drive-absolute node:fs path through with that prefix), checks whether the final path component is a reserved DOS device name (NUL,CON,PRN,AUX,COM1-COM9,LPT1-LPT9; case-insensitive, trailing./ignored). If so the whole path resolves to\??\DEVICE. This replaces the old\nultail match.UNC (
\\server\...), LocalDevice (\\.\...,\\?\UNC\...,\\?\Volume{...}) and NT-object (\??\...) inputs are exempt, because Win32 never applies DOS translation there: a named pipe whose last component happens to becom1ornul, or a file literally namedauxon an SMB share, must keep opening the pipe or the share file. The old\nultail check did not make that distinction, so\\.\pipe\foo\nulused to be redirected to the NUL device; that case now reaches the pipe.\\.\...and bare\\?\<name>inputs now emit\??\...for NtCreateFile (both spell the DosDevices directory; NtCreateFile only accepts the NT form, with/normalised to\), sofs.writeFileSync(os.devNull, ...)keeps working even though the DOS-name check no longer special-cases it. The now-unreachable\\.\-> CreateFileW fallback inopen_dir_at_windows_nt_path(and itsopen_windows_device_pathhelper) has been removed.The recogniser lives in
bun_paths::windows_reserved_device_name_t(backed by acomptime_string_map), testable on every platform.nul.txtand similar names with an extension are not treated as devices.Trailing-dot/space stripping of ordinary filenames is intentionally left out of this PR: applying it only in
normalize_path_windows_optswould makewriteFileSync(path.join(tmpdir, "x."))andstatSync(...)disagree on the target file, which is a regression from current behaviour. That, along with applying the device check to libuv-routed ops for drive-absolute paths, belongs inPathLike::slice_z_with_force_copy(see #33034).Tests
bun_paths::windows_reserved_device_name_tests: unit coverage for case, trailing characters, numbered devices, and near-misses (run on every platform).bun_sys::normalize_path_windows_tests::dos_device_names_resolve_to_nt_device: Windows-only unit tests covering the path-type gate, including\\.\pipe\com1,\\.\pipe/name,\\?\nul,\\server\share\aux,\\?\UNC\...\nul,\??\C:\a\nul.test/js/node/fs/fs-windows-dos-device-names.test.ts: end-to-end on Windows forwriteFileSync,readFileSync,statSync,Bun.write, relativesub\nul, near-misses, andos.devNull. 4/4 pass with the fix; the device-name test fails on the unpatched build.Note on Node compatibility: Node v26 on Windows also creates literal
nulfiles fromfs.writeFileSync("nul", ...)because it passes every path throughpath.toNamespacedPath(). This change intentionally matches cmd/Explorer rather than Node there, since the alternative leaves users with files they cannot remove through normal tools.no test proof · iteration 3 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/fs/fs-windows-dos-device-names.test.ts