Check ZStr/WStr constructor and errno discriminant preconditions in release builds - #38914
Check ZStr/WStr constructor and errno discriminant preconditions in release builds#38914robobun wants to merge 4 commits into
Conversation
…elease builds ZStr::from_buf / from_buf_mut / from_slice_with_nul / from_static, WStr::from_buf / from_slice_with_nul and Unaligned::slice_align_cast_mut are safe fns whose unsafe block was guarded by debug_assert! only, so release builds handed an over-long or unterminated buffer (or a misaligned slice) on to the OS when a caller got a length wrong. Make the checks assert!. SystemErrno::from_raw (and E::from_raw on Windows) transmuted any u16 into the enum; build the variant through strum's from_repr and panic on an undeclared discriminant instead. The two places that fed OS-reported codes into from_raw (impl_get_errno_libc! and Linux's raw-syscall GetErrno impl) now go through from_errno, so a code the table does not declare becomes EIO rather than an invalid enum value; node_os homedir keeps the raw errno via from_code_int and the Darwin futex code decodes through e_from_negated. Adds bun_errno unit tests for the rejected inputs and a source lint that checks the bodies of these functions for a release-mode check.
|
Warning Review limit reached
Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (13)
Comment |
|
Status: fix pushed (head 5c82683), waiting on CI. Reproduced on main with |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. It's a well-reasoned soundness fix, but it touches core cross-platform infrastructure (ZStr/WStr constructors with ~300 call sites, the errno layer under every syscall wrapper) and embeds a couple of design choices — release-mode assert!s in hot constructors, and collapsing unknown OS errnos to EIO rather than panicking — so a maintainer should sign off.
Checked: strum::FromRepr on 0.26 emits a const fn from_repr, so the const fn from_raw bodies compile; from_errno(0) still yields SUCCESS (init(0) → Some(SUCCESS)); bun_sys::e_from_negated and bun_sys::darwin::E resolve to the same bun_errno::E, so the Darwin futex match arms type-check; Error::from_code_int exists and is already the crate convention for raw errno ints. The new source lint follows the existing test/internal/source-lints/ pattern and scripts/glob-sources.ts is present.
Extended reasoning...
Overview
Promotes debug-only precondition checks to release-mode assert! in seven safe constructors in src/bun_core/util.rs (ZStr::from_buf/from_buf_mut/from_slice_with_nul/from_static, WStr::from_buf/from_slice_with_nul, Unaligned::slice_align_cast_mut), and replaces the unchecked transmute::<u16, SystemErrno> in src/errno/ with strum::FromRepr-backed from_raw that panics on undeclared discriminants. Callers that feed OS-supplied codes (impl_get_errno_libc!, Linux raw-syscall GetErrno for usize, node_os::homedir, Darwin Futex) are rerouted through checked paths (from_errno / from_code_int / e_from_negated) so an unknown errno collapses to EIO/SUCCESS instead of panicking. Adds Rust unit tests in bun_errno (round-trip, density, #[should_panic], Windows E/SystemErrno discriminant-set equality) and a new source-lint test that pins these nine functions to a release-mode check and bans transmute::<u16, E|SystemErrno>.
Security risks
None introduced. This closes soundness holes (undefined behaviour from invalid enum tags and unterminated buffers reaching syscalls) rather than opening any. No new attack surface.
Level of scrutiny
High. ZStr/WStr constructors sit under every path-taking syscall wrapper (~300 call sites per the description), and get_errno runs after every failed syscall. Adding a hard assert! here is correct per REVIEW.md ("a check that stops memory corruption is not debug-only"), but a maintainer should confirm they're comfortable with the release-build panic surface and the one-or-two-compare cost per constructor call. The errno rerouting also picks a policy — unknown OS errno → EIO — which matches the crate's existing Error::to_zig_err convention but is worth an explicit ack.
Other factors
- The PR description notes #30924 covers the errno half with a different shape; a maintainer may want to reconcile.
from_rawremainsconst fnand now callsfrom_repr; strum 0.26 (perCargo.lock) generatesconst fn from_reprfor#[repr(u16)]enums with literal discriminants, and the PR was cargo-checked on all four targets, so this holds.- Verified
bun_sys::e_from_negatedis re-exported (src/sys/lib.rs:953) and returns the sameEtype the Darwin futexmatcharms use (src/sys/lib.rs:5624), so no type mismatch there.from_errno(posix::errno())onerrno == 0still yieldsSUCCESSviainit(0). - Test coverage is solid: Rust
#[should_panic]tests, a density test pinning the0..MAXinvariant thatinit's range check relies on, and a source lint that fails on main for every changed function.
…ariant Iterating the declared variants of each enum through to_e and from_raw is both the production conversion and a few hundred match evaluations, instead of 2 x 65536 from_repr calls, which took minutes under Miri.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it hardens foundational safe-fn preconditions that sit under ~300 ZStr/WStr call sites and every errno decode path, and reroutes several OS-code callers (get_errno, Darwin futex, node_os::homedir) through different helpers with a new fallback semantics (undeclared errno → EIO), a human look at the design tradeoffs would be worthwhile.
What was reviewed:
- The remaining
SystemErrno::from_raw/E::from_rawcallers outsidebun_errno(Error::resolve_system_errno,SystemErrno::to_e) — each feeds a discriminant already validated as anE, andE/SystemErrnosharing discriminants is now pinned bye_and_system_errno_declare_the_same_discriminants. e_from_negatedon the Darwin futex path: an unknown code maps toSUCCESSand reaches the existing_ => panic!arm; the matched errnos (EINTR/EFAULT/ETIMEDOUT/ENOENT/EALREADY) are all under Darwin'sMAX = 107.from_errno(0)still yieldsSUCCESS(viainit(0)→from_raw(0)), preserving theerrno == 0 after -1behavior.bun_sys::Error::from_code_int,bun_sys::e_from_negated, andbun_core::ffi::errno_ptrall exist and are exported as used.
Extended reasoning...
Overview
This PR promotes debug-only precondition checks to release-mode assert!s in seven ZStr/WStr/Unaligned safe constructors in src/bun_core/util.rs, and replaces transmute::<u16, SystemErrno|E> in src/errno/ with strum::FromRepr-based checked construction that panics on undeclared discriminants. Callers that feed OS-reported codes (impl_get_errno_libc!, Linux raw-syscall GetErrno for usize, Darwin __ulock_wait/__ulock_wake, node_os::homedir's getpwuid_r result) are rerouted to checked helpers (from_errno → EIO fallback, e_from_negated, Error::from_code_int) so an unknown kernel errno cannot panic. A new source-lint test locates each function by signature tree-wide and enforces a release-mode check in its body plus a ban on transmute::<u16, E|SystemErrno>.
Security risks
None new. The change strictly tightens: paths that were release-only UB (reading past a slice, holding an invalid enum tag) now panic. No user-controlled input reaches a new unchecked cast. The new test file spawns git ls-tree on the repo, which is consistent with sibling lints (dead-code-escapes.test.ts).
Level of scrutiny
High. ZStr::from_buf and friends front every open/stat/unlink/CreateFileW in the runtime (~300 call sites per the description), and get_errno runs on every failed syscall. Turning silent UB into a panic is correct, but any latent caller bug (the description already names one in PackageInstall::install_from_link, fixed separately in #38571) will now crash release builds instead of misbehaving. The choice of "undeclared OS errno → EIO" versus panicking, and whether every from_raw caller was audited, are design calls a maintainer should sign off on — especially given #30924 took a different shape for the errno half.
Other factors
- I traced the remaining
from_rawcallers outside the errno crate:Error::resolve_system_errno(Windows) feeds eithertranslate_uv_error_to_e's output (always a validE, falls back toE::UNKNOWN) or atry_from_raw-validatedE, andSystemErrno::to_ecasts aSystemErrnodiscriminant — all safe under the now-tested discriminant-set equality. strum::FromReprgenerates aconst fnfor unit-variant enums, soconst fn from_rawstill compiles.bun_sys::e_from_negatedandbun_sys::Error::from_code_intboth exist as used;bun_core::ffi::errno_ptris public for the new unit test.- The comment-cop bot fired ~19 times on an earlier commit; commit
56afe0fb("Trim the comments…") followed. The remaining multi-line comments in the diff are SAFETY comments (required) or pre-existing doc comments with one word changed. - Verification in the PR description is thorough (Miri, cross-target
cargo check, Windows fs/os test suites, the new source lint failing 10/12 on main), but the Windows-only unit tests can only be type-checked, not run, on the author's setup.
|
One correction to the review summary above: the Windows-only unit tests were executed, not only type-checked. |
…as debug-only; state the lint's scope
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes foundational safety invariants (ZStr/WStr constructors used by ~300 syscall call sites, and the errno enum discriminant path used by every syscall wrapper) and makes a design choice between panic-on-invalid vs. the alternative shape in #30924, a human look would still be worthwhile.
Checked that the remaining from_raw callers in src/sys/Error.rs:299,305 and SystemErrno::init feed provably-declared discriminants (the new e_and_system_errno_declare_the_same_discriminants test pins the Windows E↔SystemErrno hop).
Verified bun_sys::e_from_negated and bun_sys::Error::from_code_int exist and are already re-exported/used, so the Darwin futex and node_os::homedir reroutes compile and preserve behavior for declared codes.
Confirmed assert! with a string literal is valid in const fn from_static, and that from_errno's EIO fallback matches the crate's existing convention.
Extended reasoning...
Overview
This PR promotes debug_assert! to assert! in seven safe fns in src/bun_core/util.rs (ZStr::from_static/from_buf/from_buf_mut/from_slice_with_nul, WStr::from_buf/from_slice_with_nul, Unaligned::slice_align_cast_mut) whose unsafe blocks were only sound under the asserted precondition, and replaces the transmute::<u16, SystemErrno|E> in src/errno/ with strum::FromRepr + panic on undeclared discriminants. Callers that feed OS-reported codes (impl_get_errno_libc!, Linux raw-syscall GetErrno for usize, Darwin futex, node_os::homedir) are rerouted through from_errno/e_from_negated/from_code_int so an unknown errno collapses to EIO (or preserves the raw int) rather than panicking. Six other files are comment-only updates reflecting that the checks are no longer debug-only. A new source-lint test pins the nine functions and bans the transmute pattern.
Security risks
None identified. The change strictly tightens release-build checks on paths that were previously UB (out-of-bounds NUL-terminated buffers handed to open/stat/CreateFileW; invalid #[repr(u16)] enum values). No new attack surface, no auth/crypto/permission code touched.
Level of scrutiny
High. ZStr/WStr are borrowed on the way into every path-taking syscall, and SystemErrno::from_raw/get_errno sit under every syscall error path. The PR changes what was silent UB into a release-mode panic for from_raw, which is correct for the audited internal callers but requires confidence that no production caller was missed — I traced the remaining from_raw uses in src/sys/Error.rs and windows_errno.rs::to_e and they all feed values proven valid by the new discriminant-equality test or by init's < MAX gate. The reroutes for OS-fed codes look correct and preserve behavior for declared errnos (checked e_from_negated on Darwin returns the same variant for EINTR/EFAULT/ETIMEDOUT/ENOENT/EALREADY and SUCCESS for anything else, which correctly reaches the existing _ => panic! arms).
Other factors
- The PR description notes #30924 took a different shape (panicking inside
get_errno); a maintainer should confirm this shape (checkedfrom_repr+ rerouting OS callers) is the preferred one. - Adding a compare-and-branch to every
ZStr::from_bufis negligible next to the syscall it precedes, but it is a hot-path change a maintainer should acknowledge. - Windows verification was via Miri (
--target x86_64-pc-windows-msvc) plus a Windows debug-build fs/os suite run; the errno unit-test binary doesn't link on a Windows host (pre-existing per the description). - The
comment-copbot flagged multi-line comments in an earlier revision; those were resolved in 56afe0f and the current diff's comments are one-liners. - The new test file follows the
test/internal/source-lints/sibling pattern (unsound-erased-box.test.ts,self-receiver-reclaim.test.ts) and importsglobAllSourceswhich exists inscripts/glob-sources.ts.
Given the breadth (13 files, foundational types, cross-platform cfg branches) and the design choice involved, this warrants a maintainer's sign-off rather than automated approval.
Problem
ZStr::from_buf/from_buf_mut/from_slice_with_nul/from_static,WStr::from_buf/from_slice_with_nul(src/bun_core/util.rs) andUnaligned::slice_align_cast_mutare safe fns whoseunsafeblock is sound only if the arguments satisfy a precondition (buf[len] == 0inside the slice, trailing NUL, alignment), and the only check was adebug_assert!. Release builds ([profile.release]has debug assertions off;scripts/build/rust.tsturns them on for the debug, asan and assertions profiles only) therefore trust all ~300 call sites: a caller that gets a length wrong hands a buffer that runs past its end, or has no terminator, toopen/stat/symlinkat/CreateFileW.SystemErrno::from_raw(src/errno/lib.rs) transmuted anyu16into the#[repr(u16)]enum behind adebug_assert!on POSIX and with no check at all on Windows, where the enum is sparse; WindowsE::from_rawhad the same shape. Two callers feed OS-reported codes straight in:impl_get_errno_libc!(lib.rs, libc errno after a-1return) and Linux's raw-syscallimpl GetErrno for usize(linux_errno.rs), which transmuted anything in1..4096into an enum with 134 variants. A FUSE file system or a seccomp filter can return any such code. Miri, running the new unit test against main (raw syscall result-134):PackageInstall::install_from_link(src/install/PackageInstall.rs:2215) fills a[u8; 512]with a 512 byte name and callsZStr::from_buf(&buf, 512). A debug build trips the assertion; a release build reads past the array and, because such a name exceedsNAME_MAXanyway, happens to getENAMETOOLONG. install: stop aborting on linked package names that do not fit the symlink buffers #38571 removes that copy. Landing order: with this PR and without install: stop aborting on linked package names that do not fit the symlink buffers #38571, a name of exactly 512 bytes aborts in release withZStr::from_buf: NUL must lie within bufinstead of failing that one package (longer names already abort on main with a slice-index panic); with install: stop aborting on linked package names that do not fit the symlink buffers #38571 first, nothing user-visible changes. Either order is sound; merging install: stop aborting on linked package names that do not fit the symlink buffers #38571 first avoids the window.Fix
assert!(same conditions and messages as the debug asserts had). They are safe fns, so the check is what makes theunsafeblock sound; REVIEW.md's rule that a check which stops memory corruption is not debug-only applies, andbytes_as_slice_mutat the top of the same file already follows it for the same reason. Cost is one or two compares per call, each immediately followed by a syscall.SystemErrno::from_rawand WindowsE::from_rawbuild the variant withfrom_repr(strum::FromRepr, now derived on the POSIX enums too) and panic on an undeclared discriminant.from_repris the exhaustive match over the declared variants, so it is the right test on Windows as well, wheren < MAXis not. The errno crate no longer contains a transmute.from_rawpanicking is right for its remaining callers (table indices ininit, and theE<->SystemErrnohop on Windows, whose discriminant sets are identical by construction and now pinned by a test). Codes that come from the OS must not panic, so those callers are moved off it:impl_get_errno_libc!and the Linuxusizeimpl go throughfrom_errno;node_oshomedir stores thegetpwuid_rresult withError::from_code_int, keeping the raw number; the Darwin futex code decodes-errnowithe_from_negated, so an unknown code reaches its existing_ => panic!("Unexpected ...")arm instead of being cast first.errno == 0after a-1return still yieldsSUCCESS, as before.get_errno(rc)asEIO.Eis an exhaustive enum, so the raw number cannot travel through it; the alternatives are a panic on OS input (ruled out by REVIEW.md) or a catch-all variant added to every POSIX enum and its tables.EIOis the fallback the crate already uses for this situation (from_errno,Error::to_zig_err). The scope is narrow: thecheck!-style wrappers that produce mostbun_sys::Errors never callget_errno(rc); they store the errno as an integer (Error::from_code_int), andError::name()/to_system_error()go on reporting an undeclared code asUNKNOWNwith the realerrnonumber, as before. The 51get_errno(rc)call sites (copy_file,Fd::close, and mostly runtime/install code matching on specific variants) are the ones that now seeEIOfor such a code; on main they received an invalid enum value. The ~230Error::get_errno()method calls were already on the checked path and are unchanged.ZStr::as_cstr(its debug check guardsCStr's no-interior-NUL documentation contract, not memory, and a hard check would scan every path before every syscall);bun_sys::windows::attr_only_open(private, one caller, NUL supplied byGetSystemDirectoryW). Three more with the same shape elsewhere are real and were handed off as their own fixes rather than folded in:DynamicBitSetList::at(bun_collections; an out-of-range index becomes an out-of-bounds write),copy_utf16_into_utf8_with_utf8_lenandstrings::eql_long(.., false)(both want theunsafe fnshape: the precondition is O(n), or the unchecked mode is the point). The lint's header states this boundary: its table pins what has been converted; it does not claim to find the class syntactically.test/internal/source-lints/safe-fn-release-checks.test.ts: locates each of the nine functions by signature, extracts the body, and requires a release-mode check (assert!/panic!, orunsafe fn) and nodebug_assert!; also banstransmute::<u16, E | SystemErrno>. On main: 10 of 12 cases fail (every function, plus the three transmute sites listed by file:line); with this change: 12 pass, withbun bd testas well.cargo test -p bun_errno --libandbun run rust:miri -p bun_errno(the CI miri job covers this crate): 9 pass, including the new#[should_panic]tests forfrom_raw(MAX)/from_raw(u16::MAX), theget_errnotests for an undeclared libc errno and an undeclared raw-syscall errno (the latter is the Miri failure quoted above, on main), and the density test (enum_map::Enum::LENGTH == MAX, no hole belowMAX) thatinit's range check relies on. The Windows-only tests (everySystemErrnovariant throughto_e, everyEvariant throughSystemErrno::from_raw, equal variant counts;E::from_raw(138)panics) were run withcargo miri test -p bun_errno --target x86_64-pc-windows-msvc: 8 pass; the one failure,errno_mapping, fails identically on main (unsuffixed literals pick thei32initoverload on Windows; tracked separately). The test binary does not link on a Windows host, on main as well (81 unresolvedbun_core/mimalloc/simdutf externals, already reported forbun_paths), so the Windows binary was exercised through the test suite instead.bun bdof this branch:test/js/node/fs/translate-uv-error-windows,readdir-windows-ntstatus,rm-windows-ntstatus(4 pass, 2 skip; these go throughto_eandError::resolve_system_errno, the two callers of the checkedfrom_raw),fs-path-length(22 pass),fs-mkdir+dir(45 pass),os.test.js(53 pass),fs.test.ts(503 pass; the one failure,readSync > works on large files, is the 5 s timeout on NTFS zero-filling the 4.9 GB file that test creates, 8-11 s on this machine).cargo check/clippyofbun_errnofor darwin, freebsd and windows targets, ofbun_threadingfor darwin (the futex change), ofbun_pathsfor windows (comment change instring_paths.rs);cargo fmt --check.bun bd, thenbun bd testontest/js/node/fs/fs.test.ts(511 pass),fs-mkdir,fs-path-length,dir,test/js/node/os/os.test.js(homedir passes;userInfofails here because$USERis unset in the container, same on the unmodified binary),test/js/node/zlib/zlib.test.js(387 pass; exercisesslice_align_cast_mutthrough the zlib bindings),test/cli/install/bun-remove,bun-install-hardlink-fallback,bun-link(its "without crashing" case fails on main with any debug build, install: make the debug-build stack dump on package install failure opt-in #37335).string_paths.rs,node_fs.rs,Watcher.rs,process.rs) that still described these checks as debug-only, and adds the scope paragraph to the lint.from_reprplus rerouting the OS-code callers, rather than panicking insideget_errno) and also covers the string constructors, so no code was taken from it.Background
ZStr/WStrare borrowed NUL-terminated byte / UTF-16 strings:len()excludes the NUL, and the type invariant is that the unit atptr[len]is 0 and belongs to the same allocation.from_buf(buf, len)is the safe way to borrow one out of a stackPathBufferthat was filled tolenand NUL-terminated; the slice bound is what provesbuf[..=len]is one allocation, which is whylen < buf.len()is part of the check.SystemErrnois a#[repr(u16)]enum of errno values. Holding a value that is not a declared discriminant is immediate undefined behaviour in Rust, whether or not it is ever matched on (amatchon it may be compiled to a jump table indexed by the value). On POSIX the variants are dense (0..MAX,MAX= 134 on Linux); on Windows there is a dense head (0..=137) plus a tail ofUV_E*discriminants around 3000-4095, andEis a second enum declared from the same lists.strum::FromReprgeneratesfn from_repr(u16) -> Option<Self>, a match over exactly the declared variants.get_errno(rc)turns a syscall's return value into anE: on libc targets it reads the thread-local errno whenrcis the failure sentinel; for raw Linux syscalls the kernel returns-errnoin the result itself, and any value in-4095..=-1denotes an error, a range wider than the enum.from_errnois the existing checked conversion (init, falling back toEIO);e_from_negatedis the same for values that arrive negated.bun_sys::Erroritself stores the errno as a plain integer and validates it when read, which is why the raw number survives on that path.debug_assert!compiles to nothing unlessdebug-assertionsis on. In this repo that is thedevprofile (bun bd) and the asan/assertions release variants; the shipped release profile has it off, so a debug-only check in a safe fn is a release-only soundness hole, and a runtime test cannot show the difference underbun bd, which is why the regression test is a source lint.[review] gate passed · iteration 0 · 13 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file