fix(sys/windows): return error for unmapped Win32 codes in symlink_w#32643
fix(sys/windows): return error for unmapped Win32 codes in symlink_w#32643robobun wants to merge 2 commits into
Conversation
When CreateSymbolicLinkW fails with a Win32 error code that has no entry in the SystemErrno mapping table, symlink_w previously returned Ok(()) and the caller believed a symlink had been created. symlink_or_junction then skipped its junction fallback, and the isolated-install linker left node_modules with missing links. This surfaces in the wild on network shares, ReFS volumes, and when security software hooks CreateSymbolicLinkW and returns driver-specific codes (Sentry BUN-2Q70 / BUN-2VC5, ~750 events on 1.3.x). The 1.3.x builds panicked in the Zig @TagName path before reaching this branch; the Rust port cannot panic there (Win32Error is a newtype) but carried over the fall-through-to-success behavior. symlink_w now uses Win32ErrorExt::to_e(), which falls back to E::UNKNOWN for unmapped codes, sets the has_failed_to_create_symlink sticky bit, and returns an error so symlink_or_junction tries a junction instead. Win32ErrorUnwrap::unwrap had the same pattern (non-SUCCESS unmapped code returned Ok(())); it now returns EUNKNOWN. That path is used by MoveFileExW in the resolver save-file routine. Windows-only unit tests added under cfg(all(windows, test)).
|
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 (2)
WalkthroughTwo Windows error-handling fixes: ChangesWin32 Error Handling Fix
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
|
Looked at the three issues the bot suggested:
Not adding |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward error-handling fix that stops unmapped Win32 codes from being treated as success, matching the existing get_last_errno() pattern.
Extended reasoning...
Overview
This PR touches two files in the Windows syscall layer: src/sys/lib.rs (the symlink_w wrapper) and src/sys/windows/mod.rs (Win32ErrorUnwrap::unwrap, plus new unit tests). In both places, the previous code called to_system_errno() and, when the Win32 error code had no entry in the ~90-entry errno mapping table, fell through to Ok(()) — reporting success for a Win32 call that had returned a failure status. The fix routes unmapped codes to E::UNKNOWN / SystemErrno::EUNKNOWN instead, so the caller sees an error and (in the symlink_or_junction case) falls back to creating a junction.
Security risks
None. This is pure error-code mapping in the filesystem syscall wrappers; no auth, crypto, input parsing, or trust-boundary changes. The change strictly tightens behavior (failure → reported as failure) rather than loosening it.
Level of scrutiny
Low-to-moderate. The diff is ~15 lines of logic plus tests, Windows-only, and brings two outliers in line with the established convention already used by windows::get_last_errno() (src/sys/windows/mod.rs:3308) and the other *_w wrappers. I verified that Win32ErrorExt::to_e() (src/errno/windows_errno.rs:867) does indeed fall back to E::UNKNOWN, and that the only external caller of Win32ErrorUnwrap::unwrap is the MoveFileExW path in src/resolver/lib.rs, where surfacing the error rather than swallowing it is clearly the correct behavior.
Other factors
The bug hunting system found no issues. The PR includes targeted #[cfg(test)] unit tests covering SUCCESS, a mapped error, and an unmapped error (0xFFFE), and the description gives a clear rationale for why an integration test is impractical (requires filter-driver/AV-injected error codes not reproducible in CI). No outstanding reviewer comments. The change is small, self-contained, and obviously correct given that rc == 0 from CreateSymbolicLinkW always means failure regardless of whether we can name the errno.
There was a problem hiding this comment.
LGTM — straightforward error-path fix that aligns symlink_w and Win32ErrorUnwrap::unwrap with the existing to_e() → E::UNKNOWN fallback pattern.
Extended reasoning...
Overview
This PR touches two spots in the Windows syscall layer:
src/sys/lib.rs(symlink_w): replaces anif let Some(sys_errno) = win_err.to_system_errno()that fell through toOk(())on unmapped Win32 codes with an unconditionalwin_err.to_e()→Err(...), whereto_e()already falls back toE::UNKNOWN.src/sys/windows/mod.rs(Win32ErrorUnwrap::unwrap): same shape — non-SUCCESSunmapped codes now yieldErr(EUNKNOWN)instead ofOk(()).- Adds 4 small
#[cfg(test)]unit tests covering success/mapped/unmapped cases.
I cross-checked Win32ErrorExt::to_e() at src/errno/windows_errno.rs:867-871 and confirmed it does .unwrap_or(E::UNKNOWN), and symlink_or_junction at src/sys/lib.rs:8762-8769 falls through to the libuv junction on any error other than EXIST/NOENT — so the new E::UNKNOWN return correctly triggers the junction fallback and sets the sticky bit, exactly as the PR description claims.
Security risks
None. This is purely an error-propagation change in the Windows filesystem wrapper layer. It makes behavior strictly more conservative: a Win32 API failure that previously returned Ok(()) now returns Err. There is no new input parsing, no auth/crypto/permission logic, and no externally-controlled data flow.
Level of scrutiny
Low-to-moderate. The diff is ~20 lines of logic, Windows-only (#[cfg(windows)]), and follows the exact pattern already used by unlink_w, mkdir_w, link_w, and translate_ntstatus_to_errno (all route through to_e() / get_last_errno() with the EUNKNOWN fallback). The change is mechanical: drop a fall-through-to-success branch that was clearly a porting artifact from the Zig version (where it panicked before reaching the fall-through).
Other factors
- Bug hunting system found no issues.
- Unit tests added that pin the new behavior (
Win32Error(0xFFFE).unwrap().is_err(),to_e() == E::UNKNOWN). - The PR description includes a thorough root-cause analysis tied to Sentry crash reports and a clear explanation of why an integration test is impractical (requires filter-driver/AV-injected error codes not present on CI).
- No outstanding reviewer comments; CodeRabbit had no actionable findings.
- Caller (
symlink_or_junction) already handles the new error gracefully via the junction fallback, so no downstream breakage.
1 participant
Crash signature
Sentry BUN-2Q70 / BUN-2VC5, ~750 events across bun 1.3.10 through 1.3.14, Windows x86_64 only, all during
bun install --linker=isolated.Status of the reported panic
The
@tagName(errno)panic atsys.zig:2795cannot occur onmain: the Zig source tree stopped compiling at 23427db (the Rust rewrite, 4 commits afterbun-v1.3.14), and the RustWin32Erroris#[repr(transparent)] struct Win32Error(pub u16)with#[derive(Debug)]. EveryGetLastError()value round-trips; there is no enum tag to be invalid. So the crash itself is fixed for any post-rewrite release with no code change.The carried-over behavior bug
Both the Zig
symlinkWand its Rust port shared the same fall-through after the log line:SystemErrno::init_win32_erroronly maps the ~90 libuv-relevant codes.CreateSymbolicLinkWcan fail with many others: filter-driver status codes, network-redirector errors, ReFS limitations, or security software that hooks the call. When it does,to_system_errno()yieldsNoneand the function returnsOk(())even though no symlink was created.On 1.3.x Zig builds the user never reached this branch because
@tagName(errno)panicked first. Onmainthe user would reach it: the isolated-installSymlinkersees success,symlink_or_junctionskips its junction fallback, andnode_modulesis left with missing package links. The install exits 0 and module resolution fails at runtime.Fix
symlink_wnow maps the Win32 error viaWin32ErrorExt::to_e(), which falls back toE::UNKNOWNfor unmapped codes. That sets thehas_failed_to_create_symlinksticky bit (sinceUNKNOWNis notNOENT/EXIST) and returns anErr, sosymlink_or_junctionfalls through to a junction and subsequent calls go straight to junctions. This matches every other*_wwrapper in the same module (unlink_w,mkdir_w,link_w), which all route throughwindows::get_last_errno()with theEUNKNOWNfallback.Win32ErrorUnwrap::unwrap(used afterMoveFileExWinbun_resolver) had the identical pattern: non-SUCCESSunmapped code returnedOk(()). It now returnsEUNKNOWN.Verification
cargo check -p bun_sys -p bun_resolver -p bun_install --target x86_64-pc-windows-msvc --testspasses.#[cfg(all(windows, test))]unit tests added insrc/sys/windows/mod.rs:Win32Error::SUCCESS.unwrap()isOkWin32Error::FILE_NOT_FOUND.unwrap()isErrWin32Error(0xFFFE).unwrap()isErr(wasOkbefore this change)Win32Error(0xFFFE).to_e()isE::UNKNOWNWhy no integration test
Reproducing requires
CreateSymbolicLinkWto return 0 withGetLastError()set to a code outside the ~90-entryinit_win32_errortable. The common failure modes on a clean Windows CI runner all map:ERROR_PRIVILEGE_NOT_HELD(1314) maps toEPERM,ERROR_NOT_SUPPORTED(50) on FAT32 maps toENOTSUP,ERROR_ALREADY_EXISTS(183) maps toEEXIST, etc. The unmapped codes observed in the field come from the user's filter-driver / AV / network-redirector stack, which CI does not have. On Linux the code path does not compile (#[cfg(windows)]), so the gate's stash-src fail-before check has nothing to run; the Rust unit tests above run only undercargo test --target x86_64-pc-windows-msvc.