Conversation
…n-string assignments
process.title = "name" was a JS-only shadow: the setter stored the value in
a Rust static so the getter read it back, but never touched the kernel, so
/proc/self/{comm,cmdline} (and thus ps/top/pgrep/pkill) kept showing "bun".
Node writes both via uv_set_process_title.
Separately, the setter used dynamicDowncast<JSString> and silently dropped
non-string assignments where Node coerces via ToString (process.title = 42
should read back as "42", Symbol should throw TypeError).
Fix:
- bun_core: add os_argv_title_span() which computes (once) the contiguous
kernel argv byte span, after ensuring argv has been copied to owned
storage, and set_process_title() which on Linux does prctl(PR_SET_NAME)
plus an in-place NUL-padded overwrite of that span.
- node_process.rs / Arguments.rs: call set_process_title() from the
process.title setter and the --title flag.
- BunProcess.cpp: coerce the setter RHS via JSValue::toString() instead of
dynamicDowncast, with RETURN_IF_EXCEPTION for the Symbol case.
|
Status: diff is green; ready for maintainer review. CI build #75447 (re-roll): 282/286 jobs passed.
The remaining annotations are retry-passed flakes (terminal, install, repl, webview, etc.). The darwin x64 bake timeouts seen on the previous run (#75338) were a single-box job timeout on Reproduce locally: |
|
Updated 9:34 AM PT - Jul 18th, 2026
❌ @robobun, your commit 98e645c has 2 failures in
🧪 To try this PR locally: bunx bun-pr 34618That installs a local version of the PR into your bun-34618 --bun |
|
Warning Review limit reached
Next review available in: 4 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 (6)
Comment |
There was a problem hiding this comment.
Beyond the inline nit, I checked: (1) the --title CLI path passes a slice into the owned argv_storage() ZBox, not the kernel block, so copy_nonoverlapping src/dest don't overlap; (2) os_argv_title_span() forces argv_storage() before returning the span, so bun_core::argv() / process.argv never observe the overwritten bytes; (3) the argv-span write is bounded (n ≤ cap-1, remainder NUL-padded, cap ≥ 1 since end ≥ first + strlen(first)+1); (4) the new toString(globalObject) is followed by RETURN_IF_EXCEPTION for the Symbol/throwing-toString case. Deferring to a human because the change adds unsafe in-place mutation of the kernel argv block.
Extended reasoning...
This PR adds unsafe raw-pointer writes into the process-lifetime kernel argv region and changes C++ JSC exception-path behavior. The bug-hunting pass surfaced only a test-style nit; the concrete memory-safety concerns I traced (source/dest overlap on the CLI path, argv() aliasing, span bounds, ToString exception check) all hold up. But per the approval bar, new unsafe blocks that mutate process-global kernel memory are not the kind of change to auto-approve — a human should sign off on the provenance/aliasing reasoning in os_argv_title_span and set_process_title.
Per review: fold stderr/stdout/exitCode into a single toEqual so a failing run surfaces all three in the diff instead of short-circuiting on stderr.
There was a problem hiding this comment.
No bugs found, but this adds new unsafe code that overwrites the kernel argv block in-place — worth a human look before merging.
What was reviewed:
os_argv_title_span()pointer arithmetic andOncecaching;argv_storage()is forced first so the overwrite cannot aliasbun_core::argv()'s ownedZBoxes or the CLI-parsed--titleslice.set_process_titlebounds:n = min(len, cap-1)+ NUL-padcap-n; no underflow whencap >= 1(always true since span includes argv[0]'s NUL).- C++ setter:
toString()+RETURN_IF_EXCEPTIONbefore use; downstreamBun::toStringRefpath unchanged. - Mutex held across the OS write in
set_title; CLI--titlepath is single-threaded startup.
Extended reasoning...
Overview
This PR makes process.title = ... write through to the OS on Linux (via prctl(PR_SET_NAME) + in-place rewrite of the kernel argv block, matching libuv's uv_set_process_title) and changes the C++ setter to coerce the RHS via ToString instead of silently dropping non-string assignments. It touches bun_core (two new functions with unsafe blocks), BunProcess.cpp (JSC exception path), the CLI --title handler, and node_process.rs. Four new tests cover coercion (all platforms) and /proc/self/{comm,cmdline} write-through (Linux-only).
Security risks
None identified. The argv-block rewrite is bounded by the cached span length computed from the original kernel argv layout; the title bytes come from an owned Box<[u8]> (JS path) or a ZBox-backed argv slice (CLI path), never from the kernel block itself, so copy_nonoverlapping is sound. PR_SET_NAME is truncated to 15 bytes into a zeroed 16-byte stack buffer.
Level of scrutiny
Moderate-to-high. The change is small and well-motivated (Node compat), but it introduces raw-pointer writes into a process-global memory region, caches a *mut u8 as usize in a Once, and relies on argv_storage() having already copied the original strings before the first overwrite. These invariants all check out on my read, and the technique mirrors libuv exactly, but per REVIEW.md's memory-safety guidance ("Never let a pointer or slice outlive the memory it points into", "exactly one named owner") this is the kind of unsafe code a maintainer should sign off on rather than a bot.
Other factors
The prior inline nit (combined-object assertion for the --title subprocess test) was addressed in 104da5d. Tests pass locally per the PR description with fail-before/pass-after verification and rust:check-all across 10 targets. macOS remains a no-op (existing TODO preserved); Windows keeps its uv_set_process_title path. The one thing I'd flag for the human reviewer to sanity-check is whether any other code path (e.g. crash-handler command-line dumping, bun_crash_handler) reads the raw kernel argv block directly rather than via bun_core::argv() — I found none, but the reviewer will know the codebase better.
|
Re the crash-handler question in the review: verified that nothing reads the raw kernel argv block after startup. |
There was a problem hiding this comment.
No issues found, but this adds new unsafe code that overwrites the kernel argv block in place — worth a human maintainer confirming the aliasing/lifetime story and the choice to leave macOS as JS-readback-only.
What was reviewed:
os_argv_title_spancaches the span viaOncebefore any overwrite and forcesargv_storage()first, sobun_core::argv()/crash-handler dumps read owned copies (robobun traced the onlyOS_ARGVreaders).set_process_titlebounds:n ≤ cap-1sowrite_bytes(.., cap-n)always writes ≥1 NUL and never overruns;prctlbuffer is 16-byte zeroed.- C++ setter's
toString+RETURN_IF_EXCEPTIONis correct for Symbol;toStringRefon the resultingJSString*is the same call path as before. - The prior stderr-assertion nit was applied in 104da5d for the
--titletest.
Extended reasoning...
Overview
This PR makes process.title = ... write through to the OS on Linux (via prctl(PR_SET_NAME) + in-place rewrite of the kernel argv block, matching libuv's uv_set_process_title), and changes the C++ setter to coerce non-string RHS via ToString instead of silently dropping them. Touches: bun_core/Global.rs (new set_process_title), bun_core/util.rs (new os_argv_title_span), BunProcess.cpp (setter coercion), node_process.rs (hold lock across OS write), Arguments.rs (--title flag wires through), plus tests.
Security risks
None user-facing. The new unsafe blocks write to process-owned memory (the CRT/kernel argv block) that the process already has full write access to; no external input controls the write bounds beyond min(title.len(), cap-1). No auth/crypto/permissions.
Level of scrutiny
Medium-high. This is not mechanical: it introduces two new unsafe blocks doing raw copy_nonoverlapping/write_bytes into a kernel-managed byte region, plus a Once-cached raw pointer stored as usize. The approach is standard (it's what libuv/Node do), the SAFETY comments are accurate, and the span computation correctly caches before any rewrite so later strlen calls can't observe NUL-padded results. But REVIEW.md flags memory safety as the most-blocked category, and a human should confirm they're comfortable with (a) the in-place argv rewrite on Linux and (b) leaving macOS as a no-op (Node writes through there too via libuv's darwin-proctitle path; the existing TODO is preserved but not addressed).
Other factors
- CI green (281/286; the one red job is a pre-existing unrelated JSC assertion on main).
- Tests cover ToString coercion (all platforms), /proc write-through for both the setter and
--title, and the long-title truncation behavior. rust:check-allpassed 10/10 targets per the PR description.- The one nit I raised previously (combined-object subprocess assertion) was applied in 104da5d for the
--titletest; the author left the JSON-parsed test as-is with a reasonable justification. - I checked that
prctl(PR_SET_NAME)from a Worker thread would set the worker thread's comm rather than the main process's — this matches Node/libuv behavior (libuv'suv_set_process_titlealso callsprctlon the calling thread), so not a divergence.
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-18, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
What
process.title = "name"was a JS-only shadow: the setter stored the value in a Rust static so the getter read it back, but never touched the kernel./proc/self/{comm,cmdline}(and thereforeps,top,pgrep,pkill) kept showingbun. Node writes both viauv_set_process_title.Separately, the setter used
dynamicDowncast<JSString>and silently dropped non-string assignments. Node coerces viaToString:process.title = 42reads back as"42", andSymbolthrowsTypeError.Repro
Cause
Bun__Process__setTitleinsrc/runtime/node/node_process.rsonly stored the bytes into theBun__Node__ProcessTitlestatic; no syscall was ever made on POSIX. (On Windows the C++ setter already calleduv_set_process_title.)setProcessTitleinsrc/jsc/bindings/BunProcess.cppuseddynamicDowncast<JSC::JSString>and returnedfalsefor any non-string RHS, so the assignment was a no-op instead of coercing.Fix
bun_core::os_argv_title_span()computes (and caches viaOnce) the contiguous kernel argv byte span starting atargv[0], after forcingargv_storage()so the original strings are already copied into ownedZBoxes and overwriting the kernel block cannot be observed viabun_core::argv()/process.argv.bun_core::set_process_title(title)on Linux performsprctl(PR_SET_NAME)(updates/proc/self/comm, kernel-truncated to 15 bytes) and an in-place NUL-padded overwrite of the argv span (updates/proc/self/cmdline). No-op on other platforms; Windows keeps usinguv_set_process_title, and macOS stays JS-readback-only (the TODO referencing libuv'sdarwin-proctitle.cremains).Bun__Process__setTitleand the--titleCLI flag now callset_process_titlein addition to updating the static. The setter holds the title mutex across the OS write so concurrent setters serialise.JSValue::toString(globalObject)withRETURN_IF_EXCEPTION, matching Node (42->"42", object withtoStringinvoked,null->"null",Symbolthrows).Verification
test/js/node/process/process.test.jscover ToString coercion (all platforms),/proc/self/{comm,cmdline}write-through forprocess.title =and--title(Linux only viait.skipIf(!isLinux)), and the long-title case (JS readback is full-length, comm is 15-byte, cmdline is capped to the argv span).USE_SYSTEM_BUN=1 bun test test/js/node/process/process.test.js -t title: new tests fail.bun bd test test/js/node/process/process.test.js -t title: 4 pass.process.argv/process.argv0/process.execPathverified unchanged after a title write (they read from owned copies).BUN_JSC_validateExceptionChecks=1clean for the Symbol throw path.bun run rust:check-all: 10/10 targets.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/process/process.test.js