Repository navigation
cli: remove unsafe from bun run / bun test / --filter and the other CLI commands - #40242
Jarred-Sumner wants to merge 33 commits into
Conversation
|
Updated 1:33 AM PT - Aug 31st, 2026
❌ @Jarred-Sumner, your commit e0ca0d6 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 40242That installs a local version of the PR into your bun-40242 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Confirmed the two 22:37 findings are addressed in 03839e4: repl_command.rs now uses run_with_api_lock_mut(|vm| ReplRunner::start(&mut runner, vm)), and the unused reader = arm / BufferedReaderOwner machinery is gone from src/io. The new build_command.rs nit is the same Stacked-Borrows class at one more site.
Extended reasoning...
Commit 03839e4 landed after my previous review round and addresses both open findings from that round. Verified at HEAD: repl_command.rs:147 calls run_with_api_lock_mut with the VM threaded through, and grep for reader =/BufferedReaderOwner/__impl_buffered_reader_owner in src/io returns nothing. The only new item this run is the build_command.rs:590 env_mut()-held-across-generate_from_cli nit posted inline — same Miri-theoretical tag-pop class as the three sites already fixed in this PR. Not approving given the size and scope (72 files across CLI dispatch, filter/multi-run process orchestration, resolver Entry mutability, lockfile load-result reshaping); a human look is warranted regardless.
There was a problem hiding this comment.
I reviewed this pass and found no new issues; the last open note on create_command.rs (progress.maybe_refresh() ordering) is addressed in bd0fa6f. Given the size of the change — 74 files across the CLI dispatch, filter_run/multi_run state-machine rewrites, resolver Entry::kind/abs_path receiver changes, new OwnedThis/GlobalMiniEventLoop abstractions, and the ContextData.log API surface change — a maintainer sign-off would still be worthwhile.
Checked this round: the init_global publish ordering now writes fields before store(Release); Scanner no longer holds a &'static FileSystem across the whole-FileSystem Unique retag; filter_run/multi_run ScriptConfig fields consumed at handle-construction time are dropped from the struct; the --analyze install now runs after BuildCommand::exec returns (behaviour note in the description matches).
Extended reasoning...
Overview
This PR eliminates ~300 unsafe blocks from src/runtime/cli/ (run_command 79→0, test_command 26→0, filter_run 27→0, multi_run 25→0, create_command 33→0, and the long tail), stacked on #40235. The unsafe migrates to a small set of new safe wrappers each carrying a one-line SAFETY: GlobalMiniEventLoop (Copy handle over the thread singleton), OwnedThis<T> (Box-like owner that lends ThisPtr), get_many_sync (condvar fan-in), bun_sys::ctrl_c, VirtualMachine/JSGlobalObject helpers, and resolver Entry::kind/abs_path moving to &self + Cell. ContextData.log becomes private Option<NonNull<Log>> with set_log/log_mut/log_ref/log_ptr, and init_global replaces set_global. filter_run/multi_run are rewritten from raw-pointer backref graphs to Cell/RefCell/OnceCell + index-based dependents. LoadResult grows a Copy LoadMeta so callers can release the &mut Lockfile before saving.
Security risks
None identified. No new external-input parsing, no auth/crypto/TLS changes; the ctrl_c helper replaces two hand-rolled sigaction/SetConsoleCtrlHandler blocks with a shared wrapper. set_argv is now RwLock-guarded rather than an unsynchronized static write.
Level of scrutiny
High. This is a broad memory-model refactor of the CLI process-orchestration paths (--filter, --parallel, bun test, bunx, bun create) plus resolver/event-loop plumbing they sit on. Every prior review round surfaced Stacked-Borrows nits (singleton-getter under a protected &mut, retag-pops-raw-tag), each addressed in a follow-up commit — ~12 fix commits over the review. The PR description's testing section is extensive (debug+ASAN across the affected suites, byte-identical old-vs-new comparisons), and this pass's bug hunt found nothing new. But the surface area — new OwnedThis/ThisPtr ownership model in bun_ptr, the impl_buffered_reader_parent! borrow = this variant, GlobalMiniEventLoop, the ContextData accessor redesign, and the --analyze control-flow change (install now runs after the bundler returns rather than inside on_analyze) — is well past what an automated pass should sign off alone.
Other factors
All 12+ prior inline findings are resolved with named fix commits; the last (create_command maybe_refresh ordering) is fixed in bd0fa6f at HEAD. The PR is stacked on #40235 and will retarget to main. No CODEOWNERS check performed here, but src/runtime/cli/, src/resolver/, src/jsc/VirtualMachine.rs are maintainer-owned areas.
3366239 to
2f9324b
Compare
bd0fa6f to
1b6cb06
Compare
There was a problem hiding this comment.
I reviewed this PR across several passes and the earlier inline findings have all been addressed; this run found no further issues. Given the scope — 74 files reshaping the aliasing model for CLI dispatch, the mini event loop, filter_run/multi_run, the resolver's Entry accessors, and the lockfile load-result plumbing — a human look is still warranted before merge.
What was reviewed:
- Singleton-getter-under-live-
&mutsites (loop_ptr, get_many_sync, repl/build/test_command, install/pm log paths, Scanner readdir, create_command progress node) — the flagged instances were fixed and no new ones surfaced. init_globalpublish ordering,ScriptConfigdead-field carry in filter_run/multi_run, and thePmWhyCommanddead_ctxparam — all addressed.--analyzereshaping in install_command / pm_update_package_json — install now runs afterBuildCommand::execreturns, so a non-install failure reports throughhandle_root_error(noted in the PR description); behavior otherwise preserved.
Extended reasoning...
Overview
This PR removes unsafe from src/runtime/cli/ (run/test/filter/multi_run/create/bunx/repl/pm_* and the long tail) by pushing the unsafe operations behind typed wrappers in lower crates: GlobalMiniEventLoop (Copy handle over the thread singleton), bun_ptr::OwnedThis, ContextData::{set_log,log_mut,log_ref,log_ptr} + init_global, bun_http::get_many_sync, bun_sys::ctrl_c + Windows FFI wrappers, Entry::{kind,abs_path} going Cell-based with EntryKindResolver::resolve_kind(&self), LoadMeta split from LoadResult, and the impl_buffered_reader_parent! borrow = this arm. filter_run/multi_run are rebuilt around OwnedThis<ProcessHandle> + Cell/RefCell state and index-based dependents; --analyze moves the install out of the bundler callback.
Security risks
None identified. The changes are internal aliasing/ownership reshaping of CLI-only paths; no auth, no untrusted-input parsing surface, no crypto/TLS changes. The Windows FFI is wrapped rather than expanded.
Level of scrutiny
High. 74 files, several new cross-crate abstractions (GlobalMiniEventLoop, OwnedThis, LoadMeta, the borrow = this macro arm), a behavior-visible reshaping of --analyze, and a wholesale rebuild of the filter_run/multi_run process-handle graph. This is the class of change REVIEW.md's Architecture & layering and Treat every refactor as guilty until proven behavior-preserving rules exist for. Prior review passes surfaced ~10 Stacked-Borrows-lineage / dead-state findings, all of which the author fixed — but that history itself argues for a maintainer sign-off on the final shape rather than an automated approval.
Other factors
The PR is stacked on #40235 (base branch claude/buntest-zero-unsafe). The author reports extensive debug+ASAN test coverage plus byte-identical old-vs-new comparisons, and every prior review thread on this PR is resolved. No outstanding human review comments.
2f9324b to
4a2425a
Compare
1b6cb06 to
bc8219f
Compare
There was a problem hiding this comment.
I reviewed this PR and this pass found no new issues; all earlier rounds' findings (Stacked-Borrows singleton-under-protected-&mut sites, dead ScriptConfig fields, init_global publish ordering, PmWhyCommand dead param, create_command progress-node retag) have been addressed. Given the scope — ~74 files reshaping CLI dispatch, filter_run/multi_run state to Cell/OwnedThis/ThisPtr, ContextData.log ownership, LoadResult→LoadMeta, new GlobalMiniEventLoop/get_many_sync/ctrl_c helpers, and the --analyze control-flow change — a human pass is still warranted.
What was reviewed:
- Behaviour preservation across the
filter_run/multi_runrewrite (dependency graph, pre/main/post chaining, abort/finish accounting, envp/PATH build) andbunx/create/openbuffer-lifetime changes. - The
--analyzereshape ininstall_command/pm_update_package_json/build_command: install now runs afterBuildCommand::execreturns; the earlyreturn Ok(())for the scanner path is guarded byfetcher.is_some(). - New helpers (
OwnedThis,GlobalMiniEventLoop,read_cell,ctrl_c::set_flag_once,get_many_sync,LoadMeta) for correct drop/ownership and no aliasing regressions vs. the code they replace.
Extended reasoning...
Overview
This PR is the CLI tranche of the ongoing unsafe-removal programme (stacked on #40235). It takes src/runtime/cli/ from ~300 unsafe blocks to effectively zero across run_command, test_command, filter_run, multi_run, create_command, bunx_command, repl_command, pm_*, open, Scanner, and the long tail. To do so it introduces or extends several primitives in lower crates: bun_ptr::OwnedThis, bun_event_loop::GlobalMiniEventLoop, bun_io::impl_buffered_reader_parent!{ borrow = this }/BufferedReader::read_cell, bun_http::async_http::get_many_sync, bun_sys::ctrl_c, bun_sys::windows::{query_file_basic_information, copy_file_w, create_directory_ex_w}, bun_install::lockfile::LoadMeta, PackageManager::{load_lockfile_from_bytes, migrate_other_lockfile, activate_scripts_node, root_package_json_path}, and reshapes ContextData.log from a public *mut Log to a private Option<NonNull<Log>> behind log_mut/log_ref/log_ptr/set_log with init_global allocating and publishing the context.
The two largest structural rewrites are filter_run and multi_run: ProcessHandle/State move from raw-pointer backrefs and self-referential Box<[ProcessHandle]> to OwnedThis<ProcessHandle> + BackRef<State> with Cell/RefCell/OnceCell interior mutability, dependents become index vectors, envp is built once per script instead of PATH-swapping the shared loader at spawn time, and the AbortHandler is replaced by bun_sys::ctrl_c. The --analyze path in install_command/pm_update_package_json no longer runs the install inside the bundler's on_analyze callback; instead the callback records positionals and the install runs after BuildCommand::exec returns (which now early-returns after the dependency scan when a fetcher is present).
Security risks
None specific to this PR. It does not add new user-input parsing, network endpoints, or trust boundaries. The ctrl_c handler and Windows FFI wrappers replace existing per-file copies with shared helpers of the same shape. No auth/crypto/TLS logic is touched beyond passing an existing reject_unauthorized flag unchanged.
Level of scrutiny
High. This is a very large refactor (74 files, several thousand lines of net churn) of core CLI dispatch, process spawning/pipe accounting, and package-manager entry points — code that every bun run/bun test/bun install/bun x invocation goes through. The changes are largely mechanical unsafe-removal, but several are genuine control-flow reshapes (--analyze, filter_run/multi_run lifecycle, create_command progress/scopeguard removal, open.rs argv construction, pm_trusted_command load/save split via LoadMeta) whose behaviour-preservation depends on invariants that are easy to get subtly wrong. The PR description reports a broad debug+ASAN test run and byte-identical hand comparisons, and six earlier bug-hunting rounds surfaced ~15 findings that were all fixed in follow-up commits.
Other factors
Every prior review thread on this PR is resolved. The remaining structural risk is the sheer breadth: e.g. the filter_run/multi_run rewrite changes when envp is materialised, how dependents are keyed, and how finish/abort interleave with pipe callbacks; create_command replaces several scopeguard::defer! node-completion paths with straight-line complete_one() calls; command::get() now returns &'static ContextData instead of &'static mut. None of these tripped the bug-hunting system on this pass, but they are the kind of change a maintainer should skim before merge. This is well outside the "simple, mechanical, obvious" bar for auto-approval.
4a2425a to
d236c14
Compare
bc8219f to
5fe2401
Compare
There was a problem hiding this comment.
All fifteen inline threads from the earlier passes are addressed in the eaf6b03…5fe24013 push — I re-checked each fix site and the one new rebase commit and found nothing further. Given the size (74 files, ~7k line churn) and that this rewrites aliasing/provenance contracts across OwnedThis, GlobalMiniEventLoop, ContextData::log, ProcessHandle, and the resolver/fs &self-mutation surface, a human pass is still worthwhile before merge.
What was reviewed on this push:
get_many_syncnow owns requests asOwnedThisand derives the callback ctx fromthis_ptr();on_print_error_zig_exceptionis written explicitly inVirtualMachine::init.init_globalpublishesGLOBAL_CLI_CTXafterset_log/start_time;filter_runbuilds argv[2] viafrom_bytes_until_nulto matchmulti_run;create_commandrefreshes progress before lendingprogress.root.repl_commandthreads&mut VirtualMachinethroughrun_with_api_lock_mut; the unusedreader =arm ofimpl_buffered_reader_parent!and its supporting types were dropped;to_executable/exe_pathnow take&Loadersobuild_commandholds a shared borrow acrossgenerate_from_cli.- Dead
_ctxparams removed frompm_pkg/pm_version/pm_why; the stale SAFETY/RacyCell/&mut-borrow comments indedupe/prune/update_transitive/resolver/router/interpreter/open/scan_command/upgrade_commandare gone;Scannerholds leaf copies instead of&'static FileSystem;spawn::ProcessHandleaccessors read throughRefPtr::Derefafter the #40478 rebase.
Extended reasoning...
Overview
This branch removes unsafe from the CLI command layer (src/runtime/cli/*), the test runner, and their supporting infrastructure — 74 files, ~3000 insertions / ~4100 deletions. It introduces bun_ptr::OwnedThis<T>, converts RacyCell statics to locked equivalents, replaces raw fn+ctx callback pairs with boxed closures, adds safe wrapper methods on AsyncHTTP/ProcessHandle/ConsoleObject/JSGlobalObject/bun_sys, and reworks aliasing contracts in ContextData, MiniEventLoop, the resolver Entry/DirEntry surface, and filter_run/multi_run process handles. Fifteen inline findings from earlier passes (a mix of Stacked-Borrows provenance regressions, stale SAFETY comments, dead parameters, an alloc_zeroed niche-validity gap, and a debug-panic on user input) were each addressed by a dedicated commit in the eaf6b03…5fe24013 push.
Security risks
No new attack surface: this is an internal safety refactor with no protocol, auth, or input-validation changes. The one user-input-reachable behavioral concern raised earlier (debug-build debug_assert! panic on interior-NUL script values in filter_run) is fixed via from_bytes_until_nul. The get_many_sync helper is used only by first-party CLI paths (upgrade/completions) against Bun-controlled endpoints.
Level of scrutiny
High. This PR rewrites the aliasing and provenance contracts that gate most of the CLI's interaction with process-global singletons (VirtualMachine, FileSystem, ContextData, MiniEventLoop), introduces a new smart-pointer type whose Drop semantics matter, and adds &self-mutating accessors (Entry::set_abs_path via Cell) whose correctness depends on every caller respecting the new interior-mutability shape. The earlier passes surfaced a dozen subtle SB-provenance regressions that were each real under Miri; the fix commits look correct on re-read, but the class of bug is easy to reintroduce and hard to test for outside Miri.
Other factors
No new tests are added (the PR claims byte-identical behavior verified by hand and the existing suite under debug+ASAN). The rebase commit 5fe2401 is a mechanical RefPtr::data() → Deref swap on top of #40478 and reads correctly. All fifteen fix commits do what their subject lines say; the dead reader = macro arm and its ~70 lines of supporting types were removed rather than left speculative. Given the scope and that this is stacked on another open branch, human sign-off remains appropriate.
…uplex.rs Squash of the branch (previously 15 commits) for the rebase over #40516: - socket: hold ext-slot/owner refs in typed RefPtr slots instead of bare ref/deref - socket: hand the closed connection's io_ref to the teardown guard before running close/connectError handlers - socket: connect-error teardown owns the failed connection's io_ref on the synchronous path too; named-pipe owner takes its ref out before dispatching close/connectError - review: typed SSL ex-data slot, ContextFunction handle clears its slot on drop, ext slot is one owner pointer, HTTPContext owns its SSL_CTX - socket: take the closing socket's io_ref before dispatching the upgradeTLS twin's close handler - host_fn: ContextFunction::new takes a BackRef to the context - cron: scoped with_cron_jobs callback instead of a &'static mut accessor - BackRef<_, Root> for the h2 native-socket and duplex owner back-references - typed ref slots release on drop; ContextFunction takes EncodedSlice
…f the offered list
3f90307 to
3af4294
Compare
TimeoutObject/ImmediateObject/WTFTimer/DateHeaderTimer/EventLoopDelayMonitor and timer::All no longer contain any unsafe code. The intrusive timer heap is reached through TimerRef (a handle to a TimerOwner's EventLoopTimer slot) and TimerHeap in bun_event_loop, which own the links and heap membership; bun_io::heap::Intrusive keeps its nodes behind Cell links and never forms a &mut. timer::All is &self-only with interior mutability, so timer callbacks that re-enter it no longer alias a &mut. The heap's ref on a scheduled JS timer is a typed RefPtr slot instead of bare ref/deref counting, and the tag->owner recovery for firing/cancelling timers lives in dispatch.rs.
…o-op for unlinked slots - drain_due_wtf_timers fires the WTFTimer directly (dispatch::fire_wtf_timer) instead of going through the tag dispatch, so nothing reads a WTF slot outside the wtf_timers lock; the tag asserts in wtf_arm/wtf_disarm move under the lock. - All::remove is the single 'disarm if armed' entry point: an unlinked slot is left alone (and marked CANCELLED) rather than debug-asserting, and the DevServerMemoryVisualizerTick arm marks its slot FIRED like every other owner, so DevServer's state == ACTIVE guards stay consistent with the heap. - get_timeout no longer needs the VM. - drop the unused JsCell::from_mut.
…Timer's write-only repeat flag
…get_many_sync owns requests as OwnedThis so the callback ctx keeps its provenance
…drop the unused reader= arm of impl_buffered_reader_parent
…ared env borrow across generate_from_cli
…install/pm log print, Scanner dir iterator); drop PmWhyCommand's dead ctx param
…andles instead of keeping them on ScriptConfig; init_global publishes after initialising; Scanner holds leaf copies instead of &FileSystem
…ble/target_builtins take &Loader, test coverage collection goes through ByteRangeMapping::with_map
80c80b6 to
e0ca0d6
Compare
|
A note on the red Linux aarch64 lanes of this branch, in case it helps.
Cause: #40258 fixes the race:
|
What
Same programme as #40055 … #40240. Stacked on #40235 (base branch
claude/buntest-zero-unsafe; retarget to main once that lands). Takessrc/runtime/cli/to zero:run_command.rs79 → 0,test_command.rs26 → 0,filter_run.rs27 → 0,multi_run.rs25 → 0,create_command.rs33 → 0,pm_trusted_command.rs26 → 0,bunx_command.rs12 → 0,repl_command.rs12 → 0,test/Scanner.rs10 → 0,open.rs7 → 0, and the long tail (package_manager_command,install_command,update_interactive_command,exec_command,outdated_command,build_command,filter_arg,scan_command,install_completions_command,pm_update_package_json,init_command,pm_pkg_command,pm_version_command,add_completions,audit_command,dedupe_command,why_command) → 0. Residuals are attribute-only:mod.rs1 (unsafe extern "C" { safe fn Bun__WebView__hostMain }),Arguments.rs10 /upgrade_command.rs1 (#[unsafe(no_mangle)]statics read by C++,#[unsafe(link_section)]). Not in scope:pack_command.rs,publish_command.rs,repl.rs,test/ChangedFilesFilter.rs,test/parallel/*.Where the
unsafewent (each with a one-line SAFETY):bun_event_loop::GlobalMiniEventLoop(Copy handle on the never-freed thread singleton; call-scoped borrows likeAnyEventLoop::mini_mut);bun_jsc::VirtualMachine::{enable_hot_module_reloading, run_with_api_lock_mut, tick_immediate_tasks, watcher_add_file_by_path_slow},on_print_error_zig_exception: Option<Box<dyn Fn(&ZigException)>>(no ctx pointer),JSGlobalObject::{expose_node_module_globals, add_gc, repl_setup_global_require},ConsoleObject::print_values(&[JSValue]);bun_http::async_http::get_many_sync(condvar fan-in);bun_sys::ctrl_c::{set_flag_once, restore_default},bun_sys::windows::{get_temp_path_w, get_current_directory_w, query_file_basic_information, copy_file_w, create_directory_ex_w};bun_resolver::{EntryKindResolver::resolve_kind(&self), Entry::kind safe, Entry.abs_path: Cell + set_abs_path(&self), DirEntry::entries, Resolver::rfs};bun_options_types::context::{ContextData.log private root with set_log/log_mut/log_ref, init_global}(replacesunsafe set_global);bun_core::set_argvsafe (RwLock);bun_sourcemap_jsc::ByteRangeMapping::with_map;bun_install::PackageManager::{load_lockfile_from_bytes, migrate_other_lockfile, activate_scripts_node, root_package_json_path},LoadResult::{meta, packages_need_update};impl_buffered_reader_parent!borrow = thisandbun_ptr::OwnedThis(same hunks as #40204/#40139).Behaviour note:
BuildCommand::execnow returns after its dependency scan and the--analyzeinstall runs after the bundler returns, so a non-install failure on that path is reported throughhandle_root_error;multi_runkeeps the old truncate-at-NUL semantics viaCStr::from_bytes_until_nul.Testing
Debug+ASAN, run directly: run-shell, run_command, preload, if-present, run-eval, run-process-env, run-cjs, filter-workspace (79), multi-run (121), cpu-prof, as-node, markdown-entrypoint, no-orphans, bunx (34), bun-run (295), bun-run-bunfig, bun-test (98), coverage, parallel (34), path-ignore-patterns, lifecycle-snapshot, test-changed, isolation, retry, rerun-each, randomize, shard, timeout-behavior, concurrent-glob, pass-with-no-tests, bunshell (424), hot (12), watch, init (15), bun-pm/pkg/version/why/scan, lifecycle-scripts (122), bun-create (21), bun-upgrade (8), update_interactive_install, bun-add (70), bake ecosystem, repl (177) — pass; environment-only failures (root cwd permission case, registry/dev-server-dependent cases) identical with system bun. Hand-driven old-vs-new comparisons (pre/post scripts, env expansion,
--filter '*' buildwith one failing package,run ./file.ts,run -,xlocal bin, missing script,--bun runnode shebang,test --only-failures,-e,--print,--workspaces) byte-identical. clippy clean on 16 crates;rust-check-allwindows-msvc + apple-darwin pass.