Add ForeignRef<T> and make FetchHeaders the owned handle - #33820
Add ForeignRef<T> and make FetchHeaders the owned handle#33820Jarred-Sumner wants to merge 2 commits into
Conversation
|
Updated 8:37 PM PT - Jul 9th, 2026
@Jarred-Sumner, your commit 7fd7042 is building: |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches⚔️ Resolve merge conflicts
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No bugs found by automated review, but this is a very large PR (239 files, ~1M diff chars) that reworks ownership/aliasing invariants across the runtime, introduces a new cross-cutting ForeignRef abstraction, and includes a user-visible behavior fix — definitely one for human eyes.
Extended reasoning...
Overview
This PR introduces ForeignRef<T> / ForeignOwned / foreign_owned! in bun_opaque and converts FetchHeaders into an owned RAII handle over the C++ WebCore::FetchHeaders refcount. It then sweeps ~434 open-coded unsafe { &mut *ptr } sites across 239 files — bundler, JSC bindings, event loop, HTTP/H3 client, WebSocket upgrade, package manager, isolated/hoisted install, shell interpreter, socket layer, SQL connectors, webcore (Request/Response/Blob/streams/fetch), test runner, debugger, and more — replacing raw-pointer fields with BackRef/ParentRef/JsCell/Cell/Box receivers, tightening &mut self receivers to &self where exclusivity was never real, and reflowing several intrusive-task lifecycles to Box<Self> receivers. It also ships a behavior fix (server.fetch(url, { headers }) now clones the caller's Headers instead of aliasing it) with a regression test.
Security risks
No new attack surface, but the changes are pervasive in memory-safety-critical code: FFI ownership handoff, intrusive refcounts, GC rooting, cross-thread task queues, and Stacked/Tree Borrows provenance reasoning. A subtle mistake in any of the receiver-mutability flips, Drop-vs-manual-deinit conversions, or ManuallyDrop<FetchHeaders> borrow paths would manifest as UAF/double-free rather than a logic bug. The diff is defensive and well-commented, and the author reports clean ASan + Miri (tree-borrows) runs, but the blast radius is the entire runtime.
Level of scrutiny
High. This is exactly the class of change CLAUDE.md flags as most-blocked: paired acquisition/release rewrites, refcount balance across all terminal paths, &mut → & receiver changes on FFI opaque types, Box<Self> destructor conversions, and a new cross-cutting abstraction that will be the template for ~27 more C++ types. It also touches server.fetch semantics user-visibly. A maintainer should sanity-check the ForeignRef design (naming, ManuallyDrop for borrowed casts, leak/adopt pairing), spot-check a handful of the trickier lifecycle rewrites (e.g. WebSocketUpgradeClient, ConcurrentPromiseTask, isolated_install::Installer::manager_mut now being unsafe), and confirm the clone_this fix is the intended semantics for server.fetch.
Other factors
The bug-hunting system found nothing, the PR description documents verification thoroughly (release + debug + ASan + Miri), and the diff quality is high with per-site SAFETY comments. But at 239 files and >1M chars, with the diff truncated in my context, this is far beyond what I can verify mechanically — deferring to human review.
b40054a to
7fd7042
Compare
There was a problem hiding this comment.
No bugs found, but this is a 239-file memory-safety refactor that introduces new ownership abstractions (ForeignRef<T>, foreign_owned!) and rewrites raw-pointer handling across the bundler, event loops, JSC core, package manager, and server — plus a behavioral fix to server.fetch header ownership — so it needs a human sign-off.
Extended reasoning...
Overview
This PR adds a generic ForeignRef<T> / ForeignOwned / foreign_owned! abstraction to bun_opaque for RAII ownership of C++-allocated objects, converts FetchHeaders to be the first user (moving the raw ZST to a sys submodule and making the owned handle the public name), and then applies a sweeping cleanup that replaces ~430 unsafe { &mut *ptr } sites with in-tree owners (JsCell, Cell, ParentRef, BackRef, Box<Self> receivers). It touches 239 files across essentially every subsystem: allocators, bundler/linker, package manager, IO/event loops, JSC core (VirtualMachine, Debugger, GC controller, IPC), HTTP/H3/WebSocket clients, resolver, server request handling, shell, SQL, test runner, and CLI. It also includes one intentional behavioral change: server.fetch(url, { headers }) now clones the caller's Headers instead of aliasing it, with a regression test in test/js/bun/http/bun-server.test.ts.
Security risks
No new attack surface is introduced — this is an internal ownership/aliasing refactor. However, the changes are pervasive in memory-safety-critical code: FFI ownership of C++ refcounted objects (FetchHeaders Drop now calls WebCore__FetchHeaders__deref), intrusive-refcount destructors moved from unsafe fn deinit(*mut Self) to impl Drop, Box<Self> consuming receivers replacing heap::take, and many &mut self → &self receiver changes on opaque handles. Any subtle mismatch between the new Drop timing and the old explicit deref() call sites, or a ManuallyDrop<FetchHeaders> that escapes and is accidentally dropped, would be a use-after-free or double-free. The diff also touches concurrent paths (bundler thread pool, HTTP thread, isolated installer) where &mut self accessors were narrowed or made unsafe — the aliasing contracts are now documented in SAFETY comments rather than enforced by borrowck.
Level of scrutiny
This warrants high scrutiny from a maintainer. Per the repo's own review guidance, memory safety is the most-blocked category, and this PR simultaneously (a) introduces a new cross-cutting abstraction (ForeignRef<T>) that ~27 more types are slated to adopt, (b) changes destructor semantics on refcounted types across the runtime, (c) rewrites borrow patterns in the bundler linker's parallel chunk generation and the installer's concurrent task callbacks, and (d) ships a user-visible behavior fix. The verification section is thorough (Miri tree-borrows, ASan, ~1.9k tests), but the design decisions — whether ForeignRef is the right shape, whether ManuallyDrop<FetchHeaders> for the cast borrow case is the right API, whether the &self-everywhere receiver policy on opaque types should be codified — are architectural calls a human should make.
Other factors
The bug-hunting system found no issues. The diff is truncated at ~1M characters, so I have not reviewed roughly half of it. There are no prior human reviews or outstanding comments to address. Given the scale, the new abstraction being introduced, and the breadth of critical subsystems touched, deferring is the only reasonable call.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/jsc/VirtualMachine.rs (1)
3596-3604: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse the shared event-loop accessor here
enqueue_task_concurrentonly needs shared access, but this wrapper still goes throughevent_loop_mut()and creates an unnecessary&mut EventLoopon the foreign-thread path. Callevent_loop_shared()instead.🤖 Prompt for 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. In `@src/jsc/VirtualMachine.rs` around lines 3596 - 3604, Update VirtualMachine::enqueue_task_concurrent to call event_loop_shared() instead of event_loop_mut(), preserving shared access for the lock-free foreign-thread enqueue operation.
🤖 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/install/patch_install.rs`:
- Around line 145-147: Implement cleanup for the raw network task pointer in
`destroy` before the boxed installer is dropped, safely reclaiming it exactly
once and handling null pointers as needed. Remove the TODO and ensure
ownership/deallocation semantics match how `Callback::CalcHash::network_task` is
allocated and used.
In `@src/opaque/lib.rs`:
- Around line 488-492: Replace mem::forget in ForeignRef::leak with
ManuallyDrop-based handling, preserving the pointer extraction while preventing
Drop from running. Follow the existing StoreRef::into_raw pattern in
webcore_types.rs and remove the mem::forget call so cargo clippy passes.
---
Outside diff comments:
In `@src/jsc/VirtualMachine.rs`:
- Around line 3596-3604: Update VirtualMachine::enqueue_task_concurrent to call
event_loop_shared() instead of event_loop_mut(), preserving shared access for
the lock-free foreign-thread enqueue operation.
🪄 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: 42e3cbe7-5ebf-4f1c-ae2d-44f7b632a237
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (238)
src/ast/e.rssrc/bun_alloc/BufferFallbackAllocator.rssrc/bun_alloc/lib.rssrc/bundler/BundleThread.rssrc/bundler/LinkerContext.rssrc/bundler/ParseTask.rssrc/bundler/ServerComponentParseTask.rssrc/bundler/ThreadPool.rssrc/bundler/bundle_v2.rssrc/bundler/linker_context/computeChunks.rssrc/bundler/linker_context/convertStmtsForChunk.rssrc/bundler/linker_context/findImportedFilesInCSSOrder.rssrc/bundler/linker_context/generateChunksInParallel.rssrc/bundler/linker_context/generateCodeForFileInChunkJS.rssrc/bundler/linker_context/generateCodeForLazyExport.rssrc/bundler/linker_context/generateCompileResultForJSChunk.rssrc/bundler/linker_context/postProcessCSSChunk.rssrc/bundler/linker_context/postProcessHTMLChunk.rssrc/bundler/linker_context/postProcessJSChunk.rssrc/bundler/linker_context/prepareCssAstsForChunk.rssrc/bundler/linker_context/writeOutputFilesToDisk.rssrc/bundler/transpiler.rssrc/bunfig/Cargo.tomlsrc/bunfig/arguments.rssrc/collections/pool.rssrc/crash_handler/lib.rssrc/event_loop/AnyEventLoop.rssrc/event_loop/MiniEventLoop.rssrc/http/AsyncHTTP.rssrc/http/HTTPThread.rssrc/http/h3_client/ClientSession.rssrc/http/h3_client/PendingConnect.rssrc/http/h3_client/Stream.rssrc/http/h3_client/callbacks.rssrc/http_jsc/headers_jsc.rssrc/http_jsc/websocket_client.rssrc/http_jsc/websocket_client/WebSocketUpgradeClient.rssrc/ini/lib.rssrc/install/PackageInstaller.rssrc/install/PackageManager.rssrc/install/PackageManager/PackageManagerDirectories.rssrc/install/PackageManager/PackageManagerEnqueue.rssrc/install/PackageManager/PopulateManifestCache.rssrc/install/PackageManager/ProgressStrings.rssrc/install/PackageManager/runTasks.rssrc/install/PackageManager/updatePackageJSONAndInstall.rssrc/install/auto_installer.rssrc/install/hoisted_install.rssrc/install/isolated_install.rssrc/install/isolated_install/Installer.rssrc/install/lifecycle_script_runner.rssrc/install/patch_install.rssrc/install/resolvers/folder_resolver.rssrc/install_jsc/ini_jsc.rssrc/io/ParentDeathWatchdog.rssrc/io/PipeReader.rssrc/io/lib.rssrc/io/posix_event_loop.rssrc/io/source.rssrc/io/windows_event_loop.rssrc/js_parser/p.rssrc/js_parser/scan/scan_imports.rssrc/js_parser/visit/mod.rssrc/js_parser_jsc/Macro.rssrc/js_printer/renamer.rssrc/jsc/ConcurrentPromiseTask.rssrc/jsc/Debugger.rssrc/jsc/FetchHeaders.rssrc/jsc/GarbageCollectionController.rssrc/jsc/JSMap.rssrc/jsc/JSPromise.rssrc/jsc/MarkedArgumentBuffer.rssrc/jsc/PosixSignalHandle.rssrc/jsc/RuntimeTranspilerStore.rssrc/jsc/VirtualMachine.rssrc/jsc/WorkTask.rssrc/jsc/any_task_job.rssrc/jsc/btjs.rssrc/jsc/event_loop.rssrc/jsc/hot_reloader.rssrc/jsc/ipc.rssrc/jsc/lib.rssrc/jsc/rare_data.rssrc/jsc/webcore_types.rssrc/libuv_sys/libuv.rssrc/opaque/lib.rssrc/parsers/toml.rssrc/ptr/weak_ptr.rssrc/resolver/fs.rssrc/resolver/lib.rssrc/resolver/package_json.rssrc/resolver/resolver.rssrc/router/lib.rssrc/runtime/allocators/LinuxMemFdAllocator.rssrc/runtime/api/Archive.rssrc/runtime/api/BunObject.rssrc/runtime/api/JSBundler.rssrc/runtime/api/JSTranspiler.rssrc/runtime/api/YAMLObject.rssrc/runtime/api/bun/SSLContextCache.rssrc/runtime/api/bun/SecureContext.rssrc/runtime/api/bun/h2_frame_parser.rssrc/runtime/api/bun/js_bun_spawn_bindings.rssrc/runtime/api/bun/subprocess.rssrc/runtime/api/bun/subprocess/Writable.rssrc/runtime/api/cron.rssrc/runtime/api/filesystem_router.rssrc/runtime/api/html_rewriter.rssrc/runtime/api/js_bundle_completion_task.rssrc/runtime/api/output_file_jsc.rssrc/runtime/api/standalone_graph_jsc.rssrc/runtime/bake/DevServer.rssrc/runtime/bake/FrameworkRouter.rssrc/runtime/bake/dev_server/mod.rssrc/runtime/bake/dev_server/source_map_store.rssrc/runtime/bake/production.rssrc/runtime/cli/build_command.rssrc/runtime/cli/bunx_command.rssrc/runtime/cli/create_command.rssrc/runtime/cli/exec_command.rssrc/runtime/cli/filter_run.rssrc/runtime/cli/init_command.rssrc/runtime/cli/mod.rssrc/runtime/cli/multi_run.rssrc/runtime/cli/open.rssrc/runtime/cli/outdated_command.rssrc/runtime/cli/pack_command.rssrc/runtime/cli/package_manager_command.rssrc/runtime/cli/pm_trusted_command.rssrc/runtime/cli/pm_update_package_json.rssrc/runtime/cli/pm_version_command.rssrc/runtime/cli/publish_command.rssrc/runtime/cli/run_command.rssrc/runtime/cli/scan_command.rssrc/runtime/cli/test/Scanner.rssrc/runtime/cli/test/parallel/Channel.rssrc/runtime/cli/test_command.rssrc/runtime/cli/update_interactive_command.rssrc/runtime/cli/upgrade_command.rssrc/runtime/crypto/PBKDF2.rssrc/runtime/crypto/PasswordObject.rssrc/runtime/dispatch.rssrc/runtime/dns_jsc/dns.rssrc/runtime/ffi/ffi_body.rssrc/runtime/hw_exports.rssrc/runtime/image/Image.rssrc/runtime/ipc_host.rssrc/runtime/jsc_hooks.rssrc/runtime/napi/napi_body.rssrc/runtime/node/node_cluster_binding.rssrc/runtime/node/node_crypto_binding.rssrc/runtime/node/node_fs.rssrc/runtime/node/node_fs_watcher.rssrc/runtime/node/types.rssrc/runtime/node/win_watcher.rssrc/runtime/node/zlib/NativeZlib.rssrc/runtime/server/AnyRequestContext.rssrc/runtime/server/FileRoute.rssrc/runtime/server/HTMLBundle.rssrc/runtime/server/NodeHTTPResponse.rssrc/runtime/server/RequestContext.rssrc/runtime/server/ServerConfig.rssrc/runtime/server/StaticRoute.rssrc/runtime/server/mod.rssrc/runtime/server/server_body.rssrc/runtime/shell/Builtin.rssrc/runtime/shell/IOReader.rssrc/runtime/shell/IOWriter.rssrc/runtime/shell/builtin/cp.rssrc/runtime/shell/dispatch_tasks.rssrc/runtime/shell/interpreter.rssrc/runtime/shell/shell_body.rssrc/runtime/shell/states/Cmd.rssrc/runtime/shell/states/CondExpr.rssrc/runtime/shell/states/Expansion.rssrc/runtime/shell/subproc.rssrc/runtime/socket/Listener.rssrc/runtime/socket/WindowsNamedPipeContext.rssrc/runtime/socket/socket_body.rssrc/runtime/socket/udp_socket.rssrc/runtime/test_runner/Collection.rssrc/runtime/test_runner/Execution.rssrc/runtime/test_runner/Order.rssrc/runtime/test_runner/ScopeFunctions.rssrc/runtime/test_runner/bun_test.rssrc/runtime/test_runner/debug.rssrc/runtime/test_runner/expect.rssrc/runtime/test_runner/pretty_format.rssrc/runtime/test_runner/snapshot.rssrc/runtime/timer/EventLoopDelayMonitor.rssrc/runtime/timer/mod.rssrc/runtime/valkey_jsc/js_valkey.rssrc/runtime/webcore/ArrayBufferSink.rssrc/runtime/webcore/BakeResponse.rssrc/runtime/webcore/Blob.rssrc/runtime/webcore/Body.rssrc/runtime/webcore/ByteBlobLoader.rssrc/runtime/webcore/FileReader.rssrc/runtime/webcore/FileSink.rssrc/runtime/webcore/ObjectURLRegistry.rssrc/runtime/webcore/ReadableStream.rssrc/runtime/webcore/Request.rssrc/runtime/webcore/Response.rssrc/runtime/webcore/S3Client.rssrc/runtime/webcore/S3File.rssrc/runtime/webcore/Sink.rssrc/runtime/webcore/blob/Store.rssrc/runtime/webcore/blob/copy_file.rssrc/runtime/webcore/blob/read_file.rssrc/runtime/webcore/blob/write_file.rssrc/runtime/webcore/fetch.rssrc/runtime/webcore/fetch/FetchTasklet.rssrc/runtime/webcore/prompt.rssrc/runtime/webcore/s3/client.rssrc/runtime/webcore/s3/simple_request.rssrc/runtime/webcore/streams.rssrc/runtime/webcore/wasm_streaming.rssrc/sourcemap_jsc/CodeCoverage.rssrc/spawn/process.rssrc/spawn/static_pipe_writer.rssrc/sql_jsc/jsc.rssrc/sql_jsc/mysql/JSMySQLConnection.rssrc/sql_jsc/mysql/JSMySQLQuery.rssrc/sql_jsc/mysql/MySQLConnection.rssrc/sql_jsc/mysql/MySQLQuery.rssrc/sql_jsc/mysql/MySQLStatement.rssrc/sql_jsc/postgres/PostgresSQLConnection.rssrc/sql_jsc/postgres/PostgresSQLQuery.rssrc/sys/lib.rssrc/threading/ThreadPool.rssrc/uws_sys/Cargo.tomlsrc/uws_sys/ListenSocket.rssrc/uws_sys/Loop.rssrc/uws_sys/WebSocket.rssrc/uws_sys/quic/Stream.rssrc/watcher/Watcher.rssrc/watcher/lib.rstest/js/bun/http/bun-server.test.ts
💤 Files with no reviewable changes (1)
- src/runtime/server/NodeHTTPResponse.rs
| pub fn destroy(self: Box<Self>) { | ||
| // TODO: how to deinit `self.callback.calc_hash.network_task` | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial
Open TODO for network_task cleanup.
destroy now relies solely on Box drop, but a raw *mut NetworkTask held inside Callback::CalcHash won't be reclaimed by field Drop, so it can leak. This appears to mirror the prior behavior rather than being newly introduced, but the TODO acknowledges the gap.
Want me to open a tracking issue for deinitializing self.callback.calc_hash.network_task on destroy?
🤖 Prompt for 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.
In `@src/install/patch_install.rs` around lines 145 - 147, Implement cleanup for
the raw network task pointer in `destroy` before the boxed installer is dropped,
safely reclaiming it exactly once and handling null pointers as needed. Remove
the TODO and ensure ownership/deallocation semantics match how
`Callback::CalcHash::network_task` is allocated and used.
7fd7042 to
c6d453b
Compare
| /// must keep the raw deref + SAFETY note locally. | ||
| #[inline] | ||
| pub fn bv2_ptr(&self) -> *mut BundleV2<'static> { | ||
| self.bv2 | ||
| self.bv2.expect("bv2").as_mut_ptr() | ||
| } |
There was a problem hiding this comment.
🟡 The doc comment on Load::bv2_ptr() still cites on_load_from_js_loop as an example caller, but this PR removed that function in favor of the generic run_task_from_js_loop::<T> trampoline. Since the PR touches bv2_ptr()'s body directly, the adjacent stale reference should be updated (e.g. to run_task_from_js_loop) or dropped.
Extended reasoning...
What the issue is
The doc comment on Load::bv2_ptr() at src/bundler/bundle_v2.rs:1264-1265 reads:
Callers that need
&mut BundleV2alongside&mut Load(e.g.on_load_from_js_loop,on_load_async) must keep the raw deref + SAFETY note locally.
This PR removed pub(crate) fn on_load_from_js_loop (along with on_load_from_js_loop_raw, on_resolve_from_js_loop, and on_resolve_from_js_loop_raw) and replaced them with the generic run_task_from_js_loop::<T: BundlerTask> trampoline. The other cited caller, on_load_async, still exists — so the comment's intent is intact, but one of its two named examples now points at a deleted function.
Why this falls in scope
The PR modified bv2_ptr()'s body directly (line 1270: self.bv2.expect("bv2").as_mut_ptr()), so the doc comment sits immediately adjacent to changed lines. Per CLAUDE.md's "Delete dead code in the same PR that makes it dead" guidance, references to a removed function should be updated in the same change that removes it. grep confirms on_load_from_js_loop now appears nowhere in src/ except in this comment.
Step-by-step proof
- Before this PR,
bundle_v2.rsdefinedpub(crate) fn on_load_from_js_loop(load: &mut jsc_api::JSBundler::Load)which dereferencedload.bv2and calledBundleV2::on_load. - The diff shows this function (and its
_rawwrapper, plus theon_resolvetwins) removed and replaced bytrait BundlerTask+fn run_task_from_js_loop<T: BundlerTask>(ctx: *mut T). - The doc comment on
bv2_ptr()was left unchanged in the diff (only the method body was updated), so it still nameson_load_from_js_loop. - A reader following the comment to understand the
bv2_ptr()contract will search foron_load_from_js_loopand find nothing.
Impact
Documentation only — zero runtime effect. The comment's guidance ("keep the raw deref + SAFETY note locally") remains correct, and the surviving on_load_async reference is still valid, so a reader can still understand the intent. This is purely a dangling identifier in prose.
Suggested fix
One-word substitution: replace on_load_from_js_loop with run_task_from_js_loop in the doc comment, or drop the dead example and keep only on_load_async.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/resolver/fs.rs (1)
270-285: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard
abs_pathwith the same synchronization as the other mutableEntryfields.Entry: Syncstill relies on a lock-freeCell<Interned>here, andresolver.rsreads/writesabs_pathwithout the per-entry mutex. Extend the mutex/once-only path to coverabs_pathtoo, or theunsafe impl Sync for Entrystays unsound.🤖 Prompt for 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. In `@src/resolver/fs.rs` around lines 270 - 285, Guard Entry::abs_path with the per-entry mutex everywhere it is read or written, matching the synchronization used for the other mutable Entry fields. Update the resolver access paths and any mutex/initialization handling so abs_path is only accessed while the entry lock is held, preserving the safety assumptions behind unsafe impl Sync for Entry.src/http/HTTPThread.rs (1)
547-558: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle the fallible cache insert
src/http/HTTPThread.rs:548—custom_ssl_context_map().put(...)can fail, and this entry already owns the adoptedRefPtr. If the insert returnsAllocError, dropping the temporary leaks the SSL context ref. Reserve before adopting, or propagate the insert failure here.🤖 Prompt for 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. In `@src/http/HTTPThread.rs` around lines 547 - 558, Handle the fallible custom_ssl_context_map().put call in the surrounding SSL context caching logic: reserve capacity before constructing and adopting the RefPtr, or propagate/handle the returned AllocError so the already-owned SSL context reference is released on failure. Ensure no temporary SslContextCacheEntry drop can leak the adopted ref.
🤖 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/linker_context/postProcessJSChunk.rs`:
- Around line 52-54: Remove the unsound mutable linker access in postprocessing:
replace unsafe `ctx.c()` usage in the postprocess task with a shared,
synchronized linker handle, or serialize this path so no parallel
`generate_chunk` task can access it concurrently. Apply the same fix to the CSS
and HTML sibling implementations, and eliminate the `&mut LinkerContext`
aliasing assumption and related safety comment.
In `@src/runtime/node/win_watcher.rs`:
- Around line 232-246: In the error path of the watcher callback, update the
sequence around `Self::maybe_deinit(this)` so `me.emit_in_progress.set(false)`
executes before deinitialization. Match the ordering used by the other branches,
ensuring `maybe_deinit` can deinitialize when an error callback removes the
final handler.
---
Outside diff comments:
In `@src/http/HTTPThread.rs`:
- Around line 547-558: Handle the fallible custom_ssl_context_map().put call in
the surrounding SSL context caching logic: reserve capacity before constructing
and adopting the RefPtr, or propagate/handle the returned AllocError so the
already-owned SSL context reference is released on failure. Ensure no temporary
SslContextCacheEntry drop can leak the adopted ref.
In `@src/resolver/fs.rs`:
- Around line 270-285: Guard Entry::abs_path with the per-entry mutex everywhere
it is read or written, matching the synchronization used for the other mutable
Entry fields. Update the resolver access paths and any mutex/initialization
handling so abs_path is only accessed while the entry lock is held, preserving
the safety assumptions behind unsafe impl Sync for Entry.
🪄 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: 4f8f81ac-becc-4ce6-83ca-94e4de1f5c1c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (237)
src/ast/e.rssrc/bun_alloc/BufferFallbackAllocator.rssrc/bun_alloc/lib.rssrc/bundler/BundleThread.rssrc/bundler/LinkerContext.rssrc/bundler/ParseTask.rssrc/bundler/ServerComponentParseTask.rssrc/bundler/ThreadPool.rssrc/bundler/bundle_v2.rssrc/bundler/linker_context/computeChunks.rssrc/bundler/linker_context/convertStmtsForChunk.rssrc/bundler/linker_context/findImportedFilesInCSSOrder.rssrc/bundler/linker_context/generateChunksInParallel.rssrc/bundler/linker_context/generateCodeForFileInChunkJS.rssrc/bundler/linker_context/generateCodeForLazyExport.rssrc/bundler/linker_context/generateCompileResultForJSChunk.rssrc/bundler/linker_context/postProcessCSSChunk.rssrc/bundler/linker_context/postProcessHTMLChunk.rssrc/bundler/linker_context/postProcessJSChunk.rssrc/bundler/linker_context/prepareCssAstsForChunk.rssrc/bundler/linker_context/writeOutputFilesToDisk.rssrc/bundler/transpiler.rssrc/bunfig/Cargo.tomlsrc/bunfig/arguments.rssrc/collections/pool.rssrc/crash_handler/lib.rssrc/event_loop/AnyEventLoop.rssrc/event_loop/MiniEventLoop.rssrc/http/AsyncHTTP.rssrc/http/HTTPThread.rssrc/http/h3_client/ClientSession.rssrc/http/h3_client/PendingConnect.rssrc/http/h3_client/Stream.rssrc/http/h3_client/callbacks.rssrc/http_jsc/headers_jsc.rssrc/http_jsc/websocket_client.rssrc/http_jsc/websocket_client/WebSocketUpgradeClient.rssrc/ini/lib.rssrc/install/PackageInstaller.rssrc/install/PackageManager.rssrc/install/PackageManager/PackageManagerDirectories.rssrc/install/PackageManager/PackageManagerEnqueue.rssrc/install/PackageManager/PopulateManifestCache.rssrc/install/PackageManager/ProgressStrings.rssrc/install/PackageManager/runTasks.rssrc/install/PackageManager/updatePackageJSONAndInstall.rssrc/install/auto_installer.rssrc/install/hoisted_install.rssrc/install/isolated_install.rssrc/install/isolated_install/Installer.rssrc/install/lifecycle_script_runner.rssrc/install/patch_install.rssrc/install/resolvers/folder_resolver.rssrc/install_jsc/ini_jsc.rssrc/io/ParentDeathWatchdog.rssrc/io/PipeReader.rssrc/io/lib.rssrc/io/posix_event_loop.rssrc/io/source.rssrc/io/windows_event_loop.rssrc/js_parser/p.rssrc/js_parser/scan/scan_imports.rssrc/js_parser/visit/mod.rssrc/js_parser_jsc/Macro.rssrc/js_printer/renamer.rssrc/jsc/ConcurrentPromiseTask.rssrc/jsc/Debugger.rssrc/jsc/FetchHeaders.rssrc/jsc/JSMap.rssrc/jsc/JSPromise.rssrc/jsc/MarkedArgumentBuffer.rssrc/jsc/PosixSignalHandle.rssrc/jsc/RuntimeTranspilerStore.rssrc/jsc/VirtualMachine.rssrc/jsc/WorkTask.rssrc/jsc/any_task_job.rssrc/jsc/btjs.rssrc/jsc/event_loop.rssrc/jsc/hot_reloader.rssrc/jsc/ipc.rssrc/jsc/lib.rssrc/jsc/rare_data.rssrc/jsc/webcore_types.rssrc/libuv_sys/libuv.rssrc/opaque/lib.rssrc/parsers/toml.rssrc/ptr/weak_ptr.rssrc/resolver/fs.rssrc/resolver/lib.rssrc/resolver/package_json.rssrc/resolver/resolver.rssrc/router/lib.rssrc/runtime/allocators/LinuxMemFdAllocator.rssrc/runtime/api/Archive.rssrc/runtime/api/BunObject.rssrc/runtime/api/JSBundler.rssrc/runtime/api/JSTranspiler.rssrc/runtime/api/YAMLObject.rssrc/runtime/api/bun/SSLContextCache.rssrc/runtime/api/bun/SecureContext.rssrc/runtime/api/bun/h2_frame_parser.rssrc/runtime/api/bun/js_bun_spawn_bindings.rssrc/runtime/api/bun/subprocess.rssrc/runtime/api/bun/subprocess/Writable.rssrc/runtime/api/cron.rssrc/runtime/api/filesystem_router.rssrc/runtime/api/html_rewriter.rssrc/runtime/api/js_bundle_completion_task.rssrc/runtime/api/output_file_jsc.rssrc/runtime/api/standalone_graph_jsc.rssrc/runtime/bake/DevServer.rssrc/runtime/bake/FrameworkRouter.rssrc/runtime/bake/dev_server/mod.rssrc/runtime/bake/dev_server/source_map_store.rssrc/runtime/bake/production.rssrc/runtime/cli/build_command.rssrc/runtime/cli/bunx_command.rssrc/runtime/cli/create_command.rssrc/runtime/cli/exec_command.rssrc/runtime/cli/filter_run.rssrc/runtime/cli/init_command.rssrc/runtime/cli/mod.rssrc/runtime/cli/multi_run.rssrc/runtime/cli/open.rssrc/runtime/cli/outdated_command.rssrc/runtime/cli/pack_command.rssrc/runtime/cli/package_manager_command.rssrc/runtime/cli/pm_trusted_command.rssrc/runtime/cli/pm_update_package_json.rssrc/runtime/cli/pm_version_command.rssrc/runtime/cli/publish_command.rssrc/runtime/cli/run_command.rssrc/runtime/cli/scan_command.rssrc/runtime/cli/test/Scanner.rssrc/runtime/cli/test/parallel/Channel.rssrc/runtime/cli/test_command.rssrc/runtime/cli/update_interactive_command.rssrc/runtime/cli/upgrade_command.rssrc/runtime/crypto/PBKDF2.rssrc/runtime/crypto/PasswordObject.rssrc/runtime/dispatch.rssrc/runtime/dns_jsc/dns.rssrc/runtime/ffi/ffi_body.rssrc/runtime/hw_exports.rssrc/runtime/image/Image.rssrc/runtime/ipc_host.rssrc/runtime/jsc_hooks.rssrc/runtime/napi/napi_body.rssrc/runtime/node/node_cluster_binding.rssrc/runtime/node/node_crypto_binding.rssrc/runtime/node/node_fs.rssrc/runtime/node/node_fs_watcher.rssrc/runtime/node/types.rssrc/runtime/node/win_watcher.rssrc/runtime/node/zlib/NativeZlib.rssrc/runtime/server/AnyRequestContext.rssrc/runtime/server/FileRoute.rssrc/runtime/server/HTMLBundle.rssrc/runtime/server/NodeHTTPResponse.rssrc/runtime/server/RequestContext.rssrc/runtime/server/ServerConfig.rssrc/runtime/server/StaticRoute.rssrc/runtime/server/mod.rssrc/runtime/server/server_body.rssrc/runtime/shell/Builtin.rssrc/runtime/shell/IOReader.rssrc/runtime/shell/IOWriter.rssrc/runtime/shell/builtin/cp.rssrc/runtime/shell/dispatch_tasks.rssrc/runtime/shell/interpreter.rssrc/runtime/shell/shell_body.rssrc/runtime/shell/states/Cmd.rssrc/runtime/shell/states/CondExpr.rssrc/runtime/shell/states/Expansion.rssrc/runtime/shell/subproc.rssrc/runtime/socket/Listener.rssrc/runtime/socket/WindowsNamedPipeContext.rssrc/runtime/socket/socket_body.rssrc/runtime/socket/udp_socket.rssrc/runtime/test_runner/Collection.rssrc/runtime/test_runner/Execution.rssrc/runtime/test_runner/Order.rssrc/runtime/test_runner/ScopeFunctions.rssrc/runtime/test_runner/bun_test.rssrc/runtime/test_runner/debug.rssrc/runtime/test_runner/expect.rssrc/runtime/test_runner/pretty_format.rssrc/runtime/test_runner/snapshot.rssrc/runtime/timer/EventLoopDelayMonitor.rssrc/runtime/timer/mod.rssrc/runtime/valkey_jsc/js_valkey.rssrc/runtime/webcore/ArrayBufferSink.rssrc/runtime/webcore/BakeResponse.rssrc/runtime/webcore/Blob.rssrc/runtime/webcore/Body.rssrc/runtime/webcore/ByteBlobLoader.rssrc/runtime/webcore/FileReader.rssrc/runtime/webcore/FileSink.rssrc/runtime/webcore/ObjectURLRegistry.rssrc/runtime/webcore/ReadableStream.rssrc/runtime/webcore/Request.rssrc/runtime/webcore/Response.rssrc/runtime/webcore/S3Client.rssrc/runtime/webcore/S3File.rssrc/runtime/webcore/Sink.rssrc/runtime/webcore/blob/Store.rssrc/runtime/webcore/blob/copy_file.rssrc/runtime/webcore/blob/read_file.rssrc/runtime/webcore/blob/write_file.rssrc/runtime/webcore/fetch.rssrc/runtime/webcore/fetch/FetchTasklet.rssrc/runtime/webcore/prompt.rssrc/runtime/webcore/s3/client.rssrc/runtime/webcore/s3/simple_request.rssrc/runtime/webcore/streams.rssrc/runtime/webcore/wasm_streaming.rssrc/sourcemap_jsc/CodeCoverage.rssrc/spawn/process.rssrc/spawn/static_pipe_writer.rssrc/sql_jsc/jsc.rssrc/sql_jsc/mysql/JSMySQLConnection.rssrc/sql_jsc/mysql/JSMySQLQuery.rssrc/sql_jsc/mysql/MySQLConnection.rssrc/sql_jsc/mysql/MySQLQuery.rssrc/sql_jsc/mysql/MySQLStatement.rssrc/sql_jsc/postgres/PostgresSQLConnection.rssrc/sql_jsc/postgres/PostgresSQLQuery.rssrc/sys/lib.rssrc/threading/ThreadPool.rssrc/uws_sys/Cargo.tomlsrc/uws_sys/ListenSocket.rssrc/uws_sys/Loop.rssrc/uws_sys/WebSocket.rssrc/uws_sys/quic/Stream.rssrc/watcher/Watcher.rssrc/watcher/lib.rstest/js/bun/http/bun-server.test.ts
💤 Files with no reviewable changes (6)
- src/runtime/server/NodeHTTPResponse.rs
- src/runtime/server/ServerConfig.rs
- src/runtime/server/HTMLBundle.rs
- src/runtime/server/StaticRoute.rs
- src/runtime/server/RequestContext.rs
- src/runtime/server/mod.rs
| // SAFETY: caller must ensure no peer `generate_chunk` task holds an | ||
| // overlapping borrow of the linker; see `GenerateChunkCtx::c`. | ||
| let c: &mut LinkerContext = unsafe { ctx.c() }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect GenerateChunkCtx::c signature/body and how generate_chunk tasks are scheduled.
fd -e rs -a . src/bundler | xargs rg -nP -C4 '\bfn\s+c\s*\(' 2>/dev/null
rg -nP -C3 'GenerateChunkCtx' src/bundler
rg -nP -C4 'generate_chunk\b|generate_chunks_in_parallel|post_process_(js|css|html)_chunk' src/bundler/linker_context/generateChunksInParallel.rs 2>/dev/null || \
fd -e rs . src/bundler --exec rg -nP -C4 'generate_chunk\b'Repository: oven-sh/bun
Length of output: 28804
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the chunk scheduling loop and the worker-pool dispatch semantics.
sed -n '180,330p' src/bundler/linker_context/generateChunksInParallel.rs
printf '\n==== each_ptr definition ====\n'
rg -n -C4 'fn each_ptr|each_ptr\(' src -g '*.rs'Repository: oven-sh/bun
Length of output: 10938
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '488,620p' src/threading/ThreadPool.rsRepository: oven-sh/bun
Length of output: 5400
Stop taking &mut LinkerContext in parallel postprocess tasks each_ptr runs generate_chunk on worker threads, so unsafe { ctx.c() } can alias the same linker concurrently. Move the shared linker access behind a shared/locked handle or make this path single-threaded; the CSS and HTML siblings use the same pattern.
🤖 Prompt for 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.
In `@src/bundler/linker_context/postProcessJSChunk.rs` around lines 52 - 54,
Remove the unsound mutable linker access in postprocessing: replace unsafe
`ctx.c()` usage in the postprocess task with a shared, synchronized linker
handle, or serialize this path so no parallel `generate_chunk` task can access
it concurrently. Apply the same fix to the CSS and HTML sibling implementations,
and eliminate the `&mut LinkerContext` aliasing assumption and related safety
comment.
| if let Some(err) = status.to_error(sys::Tag::watch) { | ||
| this.emit_in_progress = true; | ||
| me.emit_in_progress.set(true); | ||
|
|
||
| for &ctx in this.handlers.keys() { | ||
| // Re-read the key each turn: the JS callback may mutate `handlers`. | ||
| for i in 0..me.handlers.get().len() { | ||
| let ctx = me.handlers.get().keys()[i]; | ||
| on_path_update_fn(Some(ctx), Event::Error(err.clone()), false); | ||
| on_update_end_fn(Some(ctx)); | ||
| } | ||
|
|
||
| // The guard is still `true` when `maybe_deinit` checks it (always a no-op there). | ||
| this.maybe_deinit(); | ||
| this.emit_in_progress = false; | ||
| // SAFETY: `this` is live; the guard makes this call a no-op. | ||
| unsafe { Self::maybe_deinit(this) }; | ||
| me.emit_in_progress.set(false); | ||
| return; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/runtime/node/win_watcher.rs"
# Show relevant sections with line numbers
sed -n '1,260p' "$file" | nl -ba | sed -n '1,260p'
echo
echo "---- maybe_deinit / emit_in_progress references ----"
rg -n "maybe_deinit|emit_in_progress" "$file"Repository: oven-sh/bun
Length of output: 189
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/runtime/node/win_watcher.rs"
echo "---- lines 1-260 ----"
awk 'NR>=1 && NR<=260 { printf "%5d %s\n", NR, $0 }' "$file"
echo
echo "---- maybe_deinit / emit_in_progress references ----"
rg -n "maybe_deinit|emit_in_progress" "$file"Repository: oven-sh/bun
Length of output: 13664
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/runtime/node/win_watcher.rs"
echo "---- lines 255-520 ----"
awk 'NR>=255 && NR<=520 { printf "%5d %s\n", NR, $0 }' "$file"Repository: oven-sh/bun
Length of output: 14213
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/runtime/node/node_fs_watcher.rs"
echo "---- matches for detach / on_update_end / ON_PATH_UPDATE ----"
rg -n "detach|on_update_end|ON_PATH_UPDATE|remove|swap_remove|handlers" "$file"
echo
echo "---- outline / selected lines ----"
awk 'NR>=1 && NR<=520 { printf "%5d %s\n", NR, $0 }' "$file" | sed -n '1,260p'Repository: oven-sh/bun
Length of output: 13241
Clear emit_in_progress before maybe_deinit in the error path.
maybe_deinit(this) is a no-op while the flag is still set, so if an error callback detaches the last handler here the watcher never deinitializes and the libuv handle stays open. Match the other branches by clearing the flag first, then calling maybe_deinit.
🤖 Prompt for 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.
In `@src/runtime/node/win_watcher.rs` around lines 232 - 246, In the error path of
the watcher callback, update the sequence around `Self::maybe_deinit(this)` so
`me.emit_in_progress.set(false)` executes before deinitialization. Match the
ordering used by the other branches, ensuring `maybe_deinit` can deinitialize
when an error callback removes the final handler.
c6d453b to
4f14bfa
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/runtime/node/win_watcher.rs (1)
232-246: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClear
emit_in_progressbeforemaybe_deinitin the error path.
maybe_deinit(this)is a no-op whileemit_in_progressis stilltrue. If an error callback detaches the last handler here, the flag is only cleared after the (no-op)maybe_deinitand then the function returns — so no deinit is ever performed and the libuv handle leaks. The no-filename branch (Lines 259-268) andemit(Lines 337-339) already clear the flag first. Match that ordering (clearing beforemaybe_deinit, which may freethis, so nothing may readmeafterward).🔧 Proposed fix
- // The guard is still `true` when `maybe_deinit` checks it (always a no-op there). - // SAFETY: `this` is live; the guard makes this call a no-op. - unsafe { Self::maybe_deinit(this) }; - me.emit_in_progress.set(false); + me.emit_in_progress.set(false); + // SAFETY: `this` is live; this may free it, and nothing reads it afterwards. + unsafe { Self::maybe_deinit(this) }; return;🤖 Prompt for 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. In `@src/runtime/node/win_watcher.rs` around lines 232 - 246, In the error path of the watcher callback, clear emit_in_progress before calling Self::maybe_deinit(this), matching the ordering used by the no-filename branch and emit. Because maybe_deinit may free this, ensure no accesses to me occur after that call; move the flag reset before it and return immediately afterward.
🤖 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/install/resolvers/folder_resolver.rs`:
- Around line 60-69: In the relative-path branch of the resolver, copy the bytes
from rel into joined[2..2 + rel.len()] before returning the widened slice; use
the existing joined buffer and length calculation so the result contains "./"
followed by the relative path rather than stale absolute-path data.
In `@src/io/PipeReader.rs`:
- Around line 1690-1732: The uv_fs_read scheduling logic is duplicated between
the post-match block and the on_file_read epilogue. Extract the shared setup and
scheduling sequence into a named helper returning sys::Result<()>, including
buffer creation, inflight-read flag management, offset calculation, file lookup,
uv_fs_read invocation, and failure cleanup; then call it from both sites while
preserving each caller’s distinct error handling.
In `@src/runtime/cli/test_command.rs`:
- Around line 2005-2008: Replace Box::leak in the DotEnv::Map singleton
initialization with bun_core::heap::release(Box::new(DotEnv::Map::init())),
preserving the &'static mut DotEnv::Map type and existing DotEnv::Loader
initialization. Follow the canonical process-lifetime allocation pattern used
elsewhere in the repository and ensure the required heap import is available.
---
Duplicate comments:
In `@src/runtime/node/win_watcher.rs`:
- Around line 232-246: In the error path of the watcher callback, clear
emit_in_progress before calling Self::maybe_deinit(this), matching the ordering
used by the no-filename branch and emit. Because maybe_deinit may free this,
ensure no accesses to me occur after that call; move the flag reset before it
and return immediately afterward.
🪄 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: 299e82f2-da0f-4037-b74d-3e62ae9e9e2e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (237)
src/ast/e.rssrc/bun_alloc/BufferFallbackAllocator.rssrc/bun_alloc/lib.rssrc/bundler/BundleThread.rssrc/bundler/LinkerContext.rssrc/bundler/ParseTask.rssrc/bundler/ServerComponentParseTask.rssrc/bundler/ThreadPool.rssrc/bundler/bundle_v2.rssrc/bundler/linker_context/computeChunks.rssrc/bundler/linker_context/convertStmtsForChunk.rssrc/bundler/linker_context/findImportedFilesInCSSOrder.rssrc/bundler/linker_context/generateChunksInParallel.rssrc/bundler/linker_context/generateCodeForFileInChunkJS.rssrc/bundler/linker_context/generateCodeForLazyExport.rssrc/bundler/linker_context/generateCompileResultForJSChunk.rssrc/bundler/linker_context/postProcessCSSChunk.rssrc/bundler/linker_context/postProcessHTMLChunk.rssrc/bundler/linker_context/postProcessJSChunk.rssrc/bundler/linker_context/prepareCssAstsForChunk.rssrc/bundler/linker_context/writeOutputFilesToDisk.rssrc/bundler/transpiler.rssrc/bunfig/Cargo.tomlsrc/bunfig/arguments.rssrc/collections/pool.rssrc/crash_handler/lib.rssrc/event_loop/AnyEventLoop.rssrc/event_loop/MiniEventLoop.rssrc/http/AsyncHTTP.rssrc/http/HTTPThread.rssrc/http/h3_client/ClientSession.rssrc/http/h3_client/PendingConnect.rssrc/http/h3_client/Stream.rssrc/http/h3_client/callbacks.rssrc/http_jsc/headers_jsc.rssrc/http_jsc/websocket_client.rssrc/http_jsc/websocket_client/WebSocketUpgradeClient.rssrc/ini/lib.rssrc/install/PackageInstaller.rssrc/install/PackageManager.rssrc/install/PackageManager/PackageManagerDirectories.rssrc/install/PackageManager/PackageManagerEnqueue.rssrc/install/PackageManager/PopulateManifestCache.rssrc/install/PackageManager/ProgressStrings.rssrc/install/PackageManager/runTasks.rssrc/install/PackageManager/updatePackageJSONAndInstall.rssrc/install/auto_installer.rssrc/install/hoisted_install.rssrc/install/isolated_install.rssrc/install/isolated_install/Installer.rssrc/install/lifecycle_script_runner.rssrc/install/patch_install.rssrc/install/resolvers/folder_resolver.rssrc/install_jsc/ini_jsc.rssrc/io/ParentDeathWatchdog.rssrc/io/PipeReader.rssrc/io/lib.rssrc/io/posix_event_loop.rssrc/io/source.rssrc/io/windows_event_loop.rssrc/js_parser/p.rssrc/js_parser/scan/scan_imports.rssrc/js_parser/visit/mod.rssrc/js_parser_jsc/Macro.rssrc/js_printer/renamer.rssrc/jsc/ConcurrentPromiseTask.rssrc/jsc/Debugger.rssrc/jsc/FetchHeaders.rssrc/jsc/JSMap.rssrc/jsc/JSPromise.rssrc/jsc/MarkedArgumentBuffer.rssrc/jsc/PosixSignalHandle.rssrc/jsc/RuntimeTranspilerStore.rssrc/jsc/VirtualMachine.rssrc/jsc/WorkTask.rssrc/jsc/any_task_job.rssrc/jsc/btjs.rssrc/jsc/event_loop.rssrc/jsc/hot_reloader.rssrc/jsc/ipc.rssrc/jsc/lib.rssrc/jsc/rare_data.rssrc/jsc/webcore_types.rssrc/libuv_sys/libuv.rssrc/opaque/lib.rssrc/parsers/toml.rssrc/ptr/weak_ptr.rssrc/resolver/fs.rssrc/resolver/lib.rssrc/resolver/package_json.rssrc/resolver/resolver.rssrc/router/lib.rssrc/runtime/allocators/LinuxMemFdAllocator.rssrc/runtime/api/Archive.rssrc/runtime/api/BunObject.rssrc/runtime/api/JSBundler.rssrc/runtime/api/JSTranspiler.rssrc/runtime/api/YAMLObject.rssrc/runtime/api/bun/SSLContextCache.rssrc/runtime/api/bun/SecureContext.rssrc/runtime/api/bun/h2_frame_parser.rssrc/runtime/api/bun/js_bun_spawn_bindings.rssrc/runtime/api/bun/subprocess.rssrc/runtime/api/bun/subprocess/Writable.rssrc/runtime/api/cron.rssrc/runtime/api/filesystem_router.rssrc/runtime/api/html_rewriter.rssrc/runtime/api/js_bundle_completion_task.rssrc/runtime/api/output_file_jsc.rssrc/runtime/api/standalone_graph_jsc.rssrc/runtime/bake/DevServer.rssrc/runtime/bake/FrameworkRouter.rssrc/runtime/bake/dev_server/mod.rssrc/runtime/bake/dev_server/source_map_store.rssrc/runtime/bake/production.rssrc/runtime/cli/build_command.rssrc/runtime/cli/bunx_command.rssrc/runtime/cli/create_command.rssrc/runtime/cli/exec_command.rssrc/runtime/cli/filter_run.rssrc/runtime/cli/init_command.rssrc/runtime/cli/mod.rssrc/runtime/cli/multi_run.rssrc/runtime/cli/open.rssrc/runtime/cli/outdated_command.rssrc/runtime/cli/pack_command.rssrc/runtime/cli/package_manager_command.rssrc/runtime/cli/pm_trusted_command.rssrc/runtime/cli/pm_update_package_json.rssrc/runtime/cli/pm_version_command.rssrc/runtime/cli/publish_command.rssrc/runtime/cli/run_command.rssrc/runtime/cli/scan_command.rssrc/runtime/cli/test/Scanner.rssrc/runtime/cli/test/parallel/Channel.rssrc/runtime/cli/test_command.rssrc/runtime/cli/update_interactive_command.rssrc/runtime/cli/upgrade_command.rssrc/runtime/crypto/PBKDF2.rssrc/runtime/crypto/PasswordObject.rssrc/runtime/dispatch.rssrc/runtime/dns_jsc/dns.rssrc/runtime/ffi/ffi_body.rssrc/runtime/hw_exports.rssrc/runtime/image/Image.rssrc/runtime/ipc_host.rssrc/runtime/jsc_hooks.rssrc/runtime/napi/napi_body.rssrc/runtime/node/node_cluster_binding.rssrc/runtime/node/node_crypto_binding.rssrc/runtime/node/node_fs.rssrc/runtime/node/node_fs_watcher.rssrc/runtime/node/types.rssrc/runtime/node/win_watcher.rssrc/runtime/node/zlib/NativeZlib.rssrc/runtime/server/AnyRequestContext.rssrc/runtime/server/FileRoute.rssrc/runtime/server/HTMLBundle.rssrc/runtime/server/NodeHTTPResponse.rssrc/runtime/server/RequestContext.rssrc/runtime/server/ServerConfig.rssrc/runtime/server/StaticRoute.rssrc/runtime/server/mod.rssrc/runtime/server/server_body.rssrc/runtime/shell/Builtin.rssrc/runtime/shell/IOReader.rssrc/runtime/shell/IOWriter.rssrc/runtime/shell/builtin/cp.rssrc/runtime/shell/dispatch_tasks.rssrc/runtime/shell/interpreter.rssrc/runtime/shell/shell_body.rssrc/runtime/shell/states/Cmd.rssrc/runtime/shell/states/CondExpr.rssrc/runtime/shell/states/Expansion.rssrc/runtime/shell/subproc.rssrc/runtime/socket/Listener.rssrc/runtime/socket/WindowsNamedPipeContext.rssrc/runtime/socket/socket_body.rssrc/runtime/socket/udp_socket.rssrc/runtime/test_runner/Collection.rssrc/runtime/test_runner/Execution.rssrc/runtime/test_runner/Order.rssrc/runtime/test_runner/ScopeFunctions.rssrc/runtime/test_runner/bun_test.rssrc/runtime/test_runner/debug.rssrc/runtime/test_runner/expect.rssrc/runtime/test_runner/pretty_format.rssrc/runtime/test_runner/snapshot.rssrc/runtime/timer/EventLoopDelayMonitor.rssrc/runtime/timer/mod.rssrc/runtime/valkey_jsc/js_valkey.rssrc/runtime/webcore/ArrayBufferSink.rssrc/runtime/webcore/BakeResponse.rssrc/runtime/webcore/Blob.rssrc/runtime/webcore/Body.rssrc/runtime/webcore/ByteBlobLoader.rssrc/runtime/webcore/FileReader.rssrc/runtime/webcore/FileSink.rssrc/runtime/webcore/ObjectURLRegistry.rssrc/runtime/webcore/ReadableStream.rssrc/runtime/webcore/Request.rssrc/runtime/webcore/Response.rssrc/runtime/webcore/S3Client.rssrc/runtime/webcore/S3File.rssrc/runtime/webcore/Sink.rssrc/runtime/webcore/blob/Store.rssrc/runtime/webcore/blob/copy_file.rssrc/runtime/webcore/blob/read_file.rssrc/runtime/webcore/blob/write_file.rssrc/runtime/webcore/fetch.rssrc/runtime/webcore/fetch/FetchTasklet.rssrc/runtime/webcore/prompt.rssrc/runtime/webcore/s3/client.rssrc/runtime/webcore/s3/simple_request.rssrc/runtime/webcore/streams.rssrc/runtime/webcore/wasm_streaming.rssrc/sourcemap_jsc/CodeCoverage.rssrc/spawn/process.rssrc/spawn/static_pipe_writer.rssrc/sql_jsc/jsc.rssrc/sql_jsc/mysql/JSMySQLConnection.rssrc/sql_jsc/mysql/JSMySQLQuery.rssrc/sql_jsc/mysql/MySQLConnection.rssrc/sql_jsc/mysql/MySQLQuery.rssrc/sql_jsc/mysql/MySQLStatement.rssrc/sql_jsc/postgres/PostgresSQLConnection.rssrc/sql_jsc/postgres/PostgresSQLQuery.rssrc/sys/lib.rssrc/threading/ThreadPool.rssrc/uws_sys/Cargo.tomlsrc/uws_sys/ListenSocket.rssrc/uws_sys/Loop.rssrc/uws_sys/WebSocket.rssrc/uws_sys/quic/Stream.rssrc/watcher/Watcher.rssrc/watcher/lib.rstest/js/bun/http/bun-server.test.ts
💤 Files with no reviewable changes (1)
- src/runtime/server/NodeHTTPResponse.rs
| return sys::Result::Ok(()); | ||
| } | ||
| } | ||
|
|
||
| // The `&mut File` borrow ends with the match, so the `&mut self` | ||
| // methods below need no raw-pointer break; nothing above replaces | ||
| // `self.source`. | ||
| let iov = uv::uv_buf_t::init(self.get_read_buffer_with_stable_memory_address(64 * 1024)); | ||
| self.flags.insert(WindowsFlags::HAS_INFLIGHT_READ); | ||
|
|
||
| let offset = if self.flags.contains(WindowsFlags::USE_PREAD) { | ||
| i64::try_from(self._offset).expect("int cast") | ||
| } else { | ||
| -1 | ||
| }; | ||
| let loop_ = self.vtable.loop_(); | ||
| let Some(Source::File(file)) = self.source.as_mut() else { | ||
| unreachable!() | ||
| }; | ||
| file.iov = iov; | ||
| // SAFETY: file is fully initialized; libuv stores cb and fires | ||
| // it on the event loop. | ||
| if let Some(err) = unsafe { | ||
| uv::uv_fs_read( | ||
| loop_.cast(), | ||
| &mut file.fs, | ||
| file.file, | ||
| &file.iov, | ||
| 1, | ||
| offset, | ||
| Some(Self::on_file_read), | ||
| ) | ||
| } | ||
| // Tagged `.write` even though the syscall is `uv_fs_read`, so | ||
| // user-visible `error.syscall` stays bit-identical with | ||
| // previous releases. | ||
| .to_error(sys::Tag::write) | ||
| { | ||
| file.complete(false); | ||
| self.flags.remove(WindowsFlags::HAS_INFLIGHT_READ); | ||
| return sys::Result::Err(err); | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
Consider extracting the duplicated uv_fs_read scheduling.
This post-match block (iov build → HAS_INFLIGHT_READ → offset → re-fetch Source::File → file.iov = iov → uv_fs_read → on-failure file.complete(false) + clear HAS_INFLIGHT_READ) is byte-for-byte the same as the on_file_read epilogue (Lines 1598-1640). The only divergence is the error tail: here you return Err, there you insert(IS_PAUSED) + on_read(Err, …).
A helper returning sys::Result<()> for the shared portion would let each caller keep its own error tail without maintaining two copies of the scheduling logic. The correctness is fine either way; this is purely to prevent the two sites drifting.
As per coding guidelines: "The second time a multi-line block appears in your diff, extract a named helper and use it at EVERY parallel site."
🤖 Prompt for 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.
In `@src/io/PipeReader.rs` around lines 1690 - 1732, The uv_fs_read scheduling
logic is duplicated between the post-match block and the on_file_read epilogue.
Extract the shared setup and scheduling sequence into a named helper returning
sys::Result<()>, including buffer creation, inflight-read flag management,
offset calculation, file lookup, uv_fs_read invocation, and failure cleanup;
then call it from both sites while preserving each caller’s distinct error
handling.
Source: Coding guidelines
| // `Loader::init` borrows the map; `Box::leak` gives it a `'static` borrow | ||
| // (the map is never freed — process-lifetime singleton). | ||
| let env_map: *mut DotEnv::Map = bun_core::heap::into_raw(Box::new(DotEnv::Map::init())); | ||
| // SAFETY: `env_map` is heap-allocated and never freed; valid for process lifetime. | ||
| let mut env_loader: Box<DotEnv::Loader> = | ||
| Box::new(DotEnv::Loader::init(unsafe { &mut *env_map })); | ||
| let env_map: &'static mut DotEnv::Map = Box::leak(Box::new(DotEnv::Map::init())); | ||
| let mut env_loader: Box<DotEnv::Loader> = Box::new(DotEnv::Loader::init(env_map)); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Prefer bun_core::heap::release over Box::leak for the process-lifetime Map singleton.
This site replaces heap::into_raw(Box::new(...)) with Box::leak. Repo comments in transpiler.rs (init_in_place), init_command.rs, and mod.rs cite PORTING.md §Forbidden as barring Box::leak even for process-lifetime singletons. The canonical pattern for this exact DotEnv::Map allocation is bun_core::heap::release(Box::new(DotEnv::Map::init())), which also yields a &'static mut Map.
♻️ Align with the canonical singleton pattern
- let env_map: &'static mut DotEnv::Map = Box::leak(Box::new(DotEnv::Map::init()));
+ let env_map: &'static mut DotEnv::Map =
+ bun_core::heap::release(Box::new(DotEnv::Map::init()));
let mut env_loader: Box<DotEnv::Loader> = Box::new(DotEnv::Loader::init(env_map));As per coding guidelines: prefer in-tree helpers over bespoke primitives and match the file's local conventions. Please confirm heap::release is the intended helper and that PORTING.md still bars Box::leak.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // `Loader::init` borrows the map; `Box::leak` gives it a `'static` borrow | |
| // (the map is never freed — process-lifetime singleton). | |
| let env_map: *mut DotEnv::Map = bun_core::heap::into_raw(Box::new(DotEnv::Map::init())); | |
| // SAFETY: `env_map` is heap-allocated and never freed; valid for process lifetime. | |
| let mut env_loader: Box<DotEnv::Loader> = | |
| Box::new(DotEnv::Loader::init(unsafe { &mut *env_map })); | |
| let env_map: &'static mut DotEnv::Map = Box::leak(Box::new(DotEnv::Map::init())); | |
| let mut env_loader: Box<DotEnv::Loader> = Box::new(DotEnv::Loader::init(env_map)); | |
| let env_map: &'static mut DotEnv::Map = | |
| bun_core::heap::release(Box::new(DotEnv::Map::init())); | |
| let mut env_loader: Box<DotEnv::Loader> = Box::new(DotEnv::Loader::init(env_map)); |
🤖 Prompt for 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.
In `@src/runtime/cli/test_command.rs` around lines 2005 - 2008, Replace Box::leak
in the DotEnv::Map singleton initialization with
bun_core::heap::release(Box::new(DotEnv::Map::init())), preserving the &'static
mut DotEnv::Map type and existing DotEnv::Loader initialization. Follow the
canonical process-lifetime allocation pattern used elsewhere in the repository
and ensure the required heap import is available.
Source: Coding guidelines
A C++-allocated object that Rust releases through an FFI destructor needs two
types: the borrowed opaque (the `opaque_ffi!` ZST) and an owner whose Drop
gives the ref back. Four such owners were hand-rolled and 27 more C++ types
release by hand at every call site. Add the generic pair to `bun_opaque`:
- `ForeignOwned`: this opaque type has a release extern
- `ForeignRef<T>`: `Deref` + `Drop`, `repr(transparent)` over `NonNull<T>`
- `foreign_owned!(T, release_fn)` emits the impl
`FetchHeaders` is the first user, and the owned handle takes the public name;
the raw ZST moves into an extern-only `sys` module.
- Every receiver is `&self`. The type is `UnsafeCell`-backed precisely so
that `&T` carries no `noalias`, and C++ mutates the header storage through
the same pointer, so `&mut self` asserted an exclusivity that was never
true and never needed.
- Constructors return `Self`; C++ always hands back a fresh +1.
- `cast()` returns `ManuallyDrop<FetchHeaders>`: it borrows the ref that the
JS `Headers` wrapper owns, so releasing it must not be expressible.
- The four `void*`-taking externs become `unsafe fn`. C++ dereferences the
pointer, and safe Rust can forge a `*mut c_void`, so `safe fn` was
unsound on all four. This matches the rule already written above the
extern block.
- `create()` and `copy_to()` take slices instead of `*mut StringPointer`
plus a separate length.
- `FetchHeaders::from` and `create_value` had no callers; deleted. The
`_`-suffixed raw variants had no external callers; now private.
Making `cast()` a `ManuallyDrop` stopped `server.fetch(url, { headers })`
from compiling, which is how it turned out to be a use-after-free. That path
adopted the ref owned by the JS `Headers` wrapper: `cast_` never bumps the
refcount, and the wrapper's `Ref<>` derefs on finalize, so the internal
`Request` and the wrapper each released the same single ref. ASan confirms
the heap-use-after-free, and the aliasing half is deterministic without it: a
header the handler sets on `req.headers` shows up on the caller's `Headers`
object. Copy with `clone_this` instead, which is the path
`new Response(_, { headers })` already takes. All six `cast`/`cast_` callers
were audited; this was the only one that adopted the borrow. Regression test
added.
The rest of the diff converts the `unsafe { &mut *ptr }` pattern this grew
out of: raw-pointer fields become the in-tree owners (`JsCell`, `Cell`,
`ParentRef`, `BackRef`, `Box<Self>` receivers) across the runtime, and the
receivers that only ever needed `&self` say so. No allocation, lock, `Rc`,
`Arc`, or refcount is added anywhere in the diff.
4f14bfa to
ee92c7f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/install/patch_install.rs (1)
148-150: 🩺 Stability & Availability | 🔵 Trivial
network_taskinCallback::CalcHashstill not reclaimed ondestroy.The
Box<Self>receiver drops owned fields, but a raw*mut NetworkTaskheld insideCalcPatchHashwon't be freed by fieldDrop, so it can leak. TheTODOacknowledges the gap. This mirrors prior behavior rather than being newly introduced by this refactor.🤖 Prompt for 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. In `@src/install/patch_install.rs` around lines 148 - 150, Implement cleanup in Callback::destroy for the raw network_task pointer stored in self.callback.calc_hash, reclaiming it exactly once with the appropriate ownership conversion before self is dropped; remove the TODO and ensure null or already-cleared pointers are handled safely.
🤖 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/event_loop/AnyEventLoop.rs`:
- Around line 650-663: Add a SAFETY comment in the EventLoopHandle::Mini branch
of env(), immediately before the .cast(), documenting that env_ptr() references
the VM-owned, thread-lifetime singleton and that widening DotEnvLoader<'_> to
DotEnvLoader<'static> is valid; mirror the rationale used by top_level_dir().
In `@src/runtime/api/html_rewriter.rs`:
- Around line 704-707: The header-cloning error path leaks the newly allocated
raw Response because response_value is not yet initialized. In the Response
construction logic around headers.clone_this, ensure the raw response is freed
before propagating the clone error, while preserving the existing ownership
transfer when set_init_headers succeeds.
---
Duplicate comments:
In `@src/install/patch_install.rs`:
- Around line 148-150: Implement cleanup in Callback::destroy for the raw
network_task pointer stored in self.callback.calc_hash, reclaiming it exactly
once with the appropriate ownership conversion before self is dropped; remove
the TODO and ensure null or already-cleared pointers are handled safely.
🪄 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: 0fceede3-14e0-4c66-80f2-df8cdc0b7a17
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (237)
src/ast/e.rssrc/bun_alloc/BufferFallbackAllocator.rssrc/bun_alloc/lib.rssrc/bundler/BundleThread.rssrc/bundler/LinkerContext.rssrc/bundler/ParseTask.rssrc/bundler/ServerComponentParseTask.rssrc/bundler/ThreadPool.rssrc/bundler/bundle_v2.rssrc/bundler/linker_context/computeChunks.rssrc/bundler/linker_context/convertStmtsForChunk.rssrc/bundler/linker_context/findImportedFilesInCSSOrder.rssrc/bundler/linker_context/generateChunksInParallel.rssrc/bundler/linker_context/generateCodeForFileInChunkJS.rssrc/bundler/linker_context/generateCodeForLazyExport.rssrc/bundler/linker_context/generateCompileResultForJSChunk.rssrc/bundler/linker_context/postProcessCSSChunk.rssrc/bundler/linker_context/postProcessHTMLChunk.rssrc/bundler/linker_context/postProcessJSChunk.rssrc/bundler/linker_context/prepareCssAstsForChunk.rssrc/bundler/linker_context/writeOutputFilesToDisk.rssrc/bundler/transpiler.rssrc/bunfig/Cargo.tomlsrc/bunfig/arguments.rssrc/collections/pool.rssrc/crash_handler/lib.rssrc/event_loop/AnyEventLoop.rssrc/event_loop/MiniEventLoop.rssrc/http/AsyncHTTP.rssrc/http/HTTPThread.rssrc/http/h3_client/ClientSession.rssrc/http/h3_client/PendingConnect.rssrc/http/h3_client/Stream.rssrc/http/h3_client/callbacks.rssrc/http_jsc/headers_jsc.rssrc/http_jsc/websocket_client.rssrc/http_jsc/websocket_client/WebSocketUpgradeClient.rssrc/ini/lib.rssrc/install/PackageInstaller.rssrc/install/PackageManager.rssrc/install/PackageManager/PackageManagerDirectories.rssrc/install/PackageManager/PackageManagerEnqueue.rssrc/install/PackageManager/PopulateManifestCache.rssrc/install/PackageManager/ProgressStrings.rssrc/install/PackageManager/runTasks.rssrc/install/PackageManager/updatePackageJSONAndInstall.rssrc/install/auto_installer.rssrc/install/hoisted_install.rssrc/install/isolated_install.rssrc/install/isolated_install/Installer.rssrc/install/lifecycle_script_runner.rssrc/install/patch_install.rssrc/install/resolvers/folder_resolver.rssrc/install_jsc/ini_jsc.rssrc/io/ParentDeathWatchdog.rssrc/io/PipeReader.rssrc/io/lib.rssrc/io/posix_event_loop.rssrc/io/source.rssrc/io/windows_event_loop.rssrc/js_parser/p.rssrc/js_parser/scan/scan_imports.rssrc/js_parser/visit/mod.rssrc/js_parser_jsc/Macro.rssrc/js_printer/renamer.rssrc/jsc/ConcurrentPromiseTask.rssrc/jsc/Debugger.rssrc/jsc/FetchHeaders.rssrc/jsc/JSMap.rssrc/jsc/JSPromise.rssrc/jsc/MarkedArgumentBuffer.rssrc/jsc/PosixSignalHandle.rssrc/jsc/RuntimeTranspilerStore.rssrc/jsc/VirtualMachine.rssrc/jsc/WorkTask.rssrc/jsc/any_task_job.rssrc/jsc/btjs.rssrc/jsc/event_loop.rssrc/jsc/hot_reloader.rssrc/jsc/ipc.rssrc/jsc/lib.rssrc/jsc/rare_data.rssrc/jsc/webcore_types.rssrc/libuv_sys/libuv.rssrc/opaque/lib.rssrc/parsers/toml.rssrc/ptr/weak_ptr.rssrc/resolver/fs.rssrc/resolver/lib.rssrc/resolver/package_json.rssrc/resolver/resolver.rssrc/router/lib.rssrc/runtime/allocators/LinuxMemFdAllocator.rssrc/runtime/api/Archive.rssrc/runtime/api/BunObject.rssrc/runtime/api/JSBundler.rssrc/runtime/api/JSTranspiler.rssrc/runtime/api/YAMLObject.rssrc/runtime/api/bun/SSLContextCache.rssrc/runtime/api/bun/SecureContext.rssrc/runtime/api/bun/h2_frame_parser.rssrc/runtime/api/bun/js_bun_spawn_bindings.rssrc/runtime/api/bun/subprocess.rssrc/runtime/api/bun/subprocess/Writable.rssrc/runtime/api/cron.rssrc/runtime/api/filesystem_router.rssrc/runtime/api/html_rewriter.rssrc/runtime/api/js_bundle_completion_task.rssrc/runtime/api/output_file_jsc.rssrc/runtime/api/standalone_graph_jsc.rssrc/runtime/bake/DevServer.rssrc/runtime/bake/FrameworkRouter.rssrc/runtime/bake/dev_server/mod.rssrc/runtime/bake/dev_server/source_map_store.rssrc/runtime/bake/production.rssrc/runtime/cli/build_command.rssrc/runtime/cli/bunx_command.rssrc/runtime/cli/create_command.rssrc/runtime/cli/exec_command.rssrc/runtime/cli/filter_run.rssrc/runtime/cli/init_command.rssrc/runtime/cli/mod.rssrc/runtime/cli/multi_run.rssrc/runtime/cli/open.rssrc/runtime/cli/outdated_command.rssrc/runtime/cli/pack_command.rssrc/runtime/cli/package_manager_command.rssrc/runtime/cli/pm_trusted_command.rssrc/runtime/cli/pm_update_package_json.rssrc/runtime/cli/pm_version_command.rssrc/runtime/cli/publish_command.rssrc/runtime/cli/run_command.rssrc/runtime/cli/scan_command.rssrc/runtime/cli/test/Scanner.rssrc/runtime/cli/test/parallel/Channel.rssrc/runtime/cli/test_command.rssrc/runtime/cli/update_interactive_command.rssrc/runtime/cli/upgrade_command.rssrc/runtime/crypto/PBKDF2.rssrc/runtime/crypto/PasswordObject.rssrc/runtime/dispatch.rssrc/runtime/dns_jsc/dns.rssrc/runtime/ffi/ffi_body.rssrc/runtime/hw_exports.rssrc/runtime/image/Image.rssrc/runtime/ipc_host.rssrc/runtime/jsc_hooks.rssrc/runtime/napi/napi_body.rssrc/runtime/node/node_cluster_binding.rssrc/runtime/node/node_crypto_binding.rssrc/runtime/node/node_fs.rssrc/runtime/node/node_fs_watcher.rssrc/runtime/node/types.rssrc/runtime/node/win_watcher.rssrc/runtime/node/zlib/NativeZlib.rssrc/runtime/server/AnyRequestContext.rssrc/runtime/server/FileRoute.rssrc/runtime/server/HTMLBundle.rssrc/runtime/server/NodeHTTPResponse.rssrc/runtime/server/RequestContext.rssrc/runtime/server/ServerConfig.rssrc/runtime/server/StaticRoute.rssrc/runtime/server/mod.rssrc/runtime/server/server_body.rssrc/runtime/shell/Builtin.rssrc/runtime/shell/IOReader.rssrc/runtime/shell/IOWriter.rssrc/runtime/shell/builtin/cp.rssrc/runtime/shell/dispatch_tasks.rssrc/runtime/shell/interpreter.rssrc/runtime/shell/shell_body.rssrc/runtime/shell/states/Cmd.rssrc/runtime/shell/states/CondExpr.rssrc/runtime/shell/states/Expansion.rssrc/runtime/shell/subproc.rssrc/runtime/socket/Listener.rssrc/runtime/socket/WindowsNamedPipeContext.rssrc/runtime/socket/socket_body.rssrc/runtime/socket/udp_socket.rssrc/runtime/test_runner/Collection.rssrc/runtime/test_runner/Execution.rssrc/runtime/test_runner/Order.rssrc/runtime/test_runner/ScopeFunctions.rssrc/runtime/test_runner/bun_test.rssrc/runtime/test_runner/debug.rssrc/runtime/test_runner/expect.rssrc/runtime/test_runner/pretty_format.rssrc/runtime/test_runner/snapshot.rssrc/runtime/timer/EventLoopDelayMonitor.rssrc/runtime/timer/mod.rssrc/runtime/valkey_jsc/js_valkey.rssrc/runtime/webcore/ArrayBufferSink.rssrc/runtime/webcore/BakeResponse.rssrc/runtime/webcore/Blob.rssrc/runtime/webcore/Body.rssrc/runtime/webcore/ByteBlobLoader.rssrc/runtime/webcore/FileReader.rssrc/runtime/webcore/FileSink.rssrc/runtime/webcore/ObjectURLRegistry.rssrc/runtime/webcore/ReadableStream.rssrc/runtime/webcore/Request.rssrc/runtime/webcore/Response.rssrc/runtime/webcore/S3Client.rssrc/runtime/webcore/S3File.rssrc/runtime/webcore/Sink.rssrc/runtime/webcore/blob/Store.rssrc/runtime/webcore/blob/copy_file.rssrc/runtime/webcore/blob/read_file.rssrc/runtime/webcore/blob/write_file.rssrc/runtime/webcore/fetch.rssrc/runtime/webcore/fetch/FetchTasklet.rssrc/runtime/webcore/prompt.rssrc/runtime/webcore/s3/client.rssrc/runtime/webcore/s3/simple_request.rssrc/runtime/webcore/streams.rssrc/runtime/webcore/wasm_streaming.rssrc/sourcemap_jsc/CodeCoverage.rssrc/spawn/process.rssrc/spawn/static_pipe_writer.rssrc/sql_jsc/jsc.rssrc/sql_jsc/mysql/JSMySQLConnection.rssrc/sql_jsc/mysql/JSMySQLQuery.rssrc/sql_jsc/mysql/MySQLConnection.rssrc/sql_jsc/mysql/MySQLQuery.rssrc/sql_jsc/mysql/MySQLStatement.rssrc/sql_jsc/postgres/PostgresSQLConnection.rssrc/sql_jsc/postgres/PostgresSQLQuery.rssrc/sys/lib.rssrc/threading/ThreadPool.rssrc/uws_sys/Cargo.tomlsrc/uws_sys/ListenSocket.rssrc/uws_sys/Loop.rssrc/uws_sys/WebSocket.rssrc/uws_sys/quic/Stream.rssrc/watcher/Watcher.rssrc/watcher/lib.rstest/js/bun/http/bun-server.test.ts
💤 Files with no reviewable changes (1)
- src/runtime/server/NodeHTTPResponse.rs
| pub fn env(self) -> BackRef<DotEnvLoader<'static>> { | ||
| match self { | ||
| EventLoopHandle::Js { owner } => owner.env(), | ||
| // `env` must be set — caller invariant. `env_ptr()` takes | ||
| // `&self` and returns `Option<NonNull<DotEnvLoader>>` (mutable | ||
| // provenance). Safe via `BackRef: Deref`. | ||
| // SAFETY: the VM-owned `DotEnv::Loader` is a thread-lifetime | ||
| // singleton; it outlives every handle to the loop. | ||
| EventLoopHandle::Js { owner } => unsafe { BackRef::from_raw(owner.env()) }, | ||
| // `env` must be set — caller invariant. `env_ptr()` takes `&self` | ||
| // and returns `Option<NonNull<DotEnvLoader>>`. | ||
| EventLoopHandle::Mini(mini) => mini | ||
| .env_ptr() | ||
| .expect("MiniEventLoop.env unset") | ||
| .as_ptr() | ||
| .cast(), | ||
| .cast() | ||
| .into(), | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Document the 'static lifetime widening in the Mini branch.
The Js branch has an explicit SAFETY comment justifying the pointer's provenance/lifetime; the Mini branch's .cast() (widening DotEnvLoader<'_> → DotEnvLoader<'static>) has no equivalent justification, even though the sibling top_level_dir() method documents the identical widening a few lines below. Since BackRef::from<NonNull<T>> is a safe constructor with no compiler-enforced invariant, a comment here is the only place this reasoning can live.
♻️ Suggested comment
EventLoopHandle::Mini(mini) => mini
.env_ptr()
.expect("MiniEventLoop.env unset")
+ // SAFETY: the loader is a thread-/process-lifetime singleton
+ // (see `MiniEventLoop::env_ptr` invariant), so widening to
+ // `'static` here matches the `Js` arm and `top_level_dir()`.
.cast()
.into(),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn env(self) -> BackRef<DotEnvLoader<'static>> { | |
| match self { | |
| EventLoopHandle::Js { owner } => owner.env(), | |
| // `env` must be set — caller invariant. `env_ptr()` takes | |
| // `&self` and returns `Option<NonNull<DotEnvLoader>>` (mutable | |
| // provenance). Safe via `BackRef: Deref`. | |
| // SAFETY: the VM-owned `DotEnv::Loader` is a thread-lifetime | |
| // singleton; it outlives every handle to the loop. | |
| EventLoopHandle::Js { owner } => unsafe { BackRef::from_raw(owner.env()) }, | |
| // `env` must be set — caller invariant. `env_ptr()` takes `&self` | |
| // and returns `Option<NonNull<DotEnvLoader>>`. | |
| EventLoopHandle::Mini(mini) => mini | |
| .env_ptr() | |
| .expect("MiniEventLoop.env unset") | |
| .as_ptr() | |
| .cast(), | |
| .cast() | |
| .into(), | |
| } | |
| } | |
| pub fn env(self) -> BackRef<DotEnvLoader<'static>> { | |
| match self { | |
| // SAFETY: the VM-owned `DotEnv::Loader` is a thread-lifetime | |
| // singleton; it outlives every handle to the loop. | |
| EventLoopHandle::Js { owner } => unsafe { BackRef::from_raw(owner.env()) }, | |
| // `env` must be set — caller invariant. `env_ptr()` takes `&self` | |
| // and returns `Option<NonNull<DotEnvLoader>>`. | |
| EventLoopHandle::Mini(mini) => mini | |
| .env_ptr() | |
| .expect("MiniEventLoop.env unset") | |
| // SAFETY: the loader is a thread-/process-lifetime singleton | |
| // (see `MiniEventLoop::env_ptr` invariant), so widening to | |
| // `'static` here matches the `Js` arm and `top_level_dir()`. | |
| .cast() | |
| .into(), | |
| } | |
| } |
🤖 Prompt for 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.
In `@src/event_loop/AnyEventLoop.rs` around lines 650 - 663, Add a SAFETY comment
in the EventLoopHandle::Mini branch of env(), immediately before the .cast(),
documenting that env_ptr() references the VM-owned, thread-lifetime singleton
and that widening DotEnvLoader<'_> to DotEnvLoader<'static> is valid; mirror the
rationale used by top_level_dir().
| if let Some(headers) = (*original).headers() { | ||
| let cloned = headers.clone_this(global)?; | ||
| (*result).set_init_headers(cloned.map(|p| HeadersRef::adopt(p))); | ||
| (*result).set_init_headers(cloned); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect BufferOutputSink teardown: does drop/destroy free `response` and `rewriter`?
fd -a 'html_rewriter.rs' src | head -1
rg -nP -C4 'impl\s+Drop\s+for\s+BufferOutputSink|CellRefCounted|destroy\s*=\s*.*BufferOutputSink' src/runtime/api/html_rewriter.rs
# Where are (*sink).response / (*sink).rewriter freed?
rg -nP -C3 '\(\*sink\)\.(response|rewriter)|heap::take\(rewriter\)|Response::finalize' src/runtime/api/html_rewriter.rsRepository: oven-sh/bun
Length of output: 11251
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=src/runtime/api/html_rewriter.rs
echo "== BufferOutputSink fields / init / drop =="
sed -n '541,740p' "$file" | cat -n
echo
echo "== Response finalizer / rewriter ownership path =="
sed -n '430,520p' "$file" | cat -n
echo
echo "== BufferOutputSink drop details =="
sed -n '976,1015p' "$file" | cat -nRepository: oven-sh/bun
Length of output: 18366
Free the raw Response on the header-clone error path. BufferOutputSink::drop already destroys rewriter, but response is still just an unowned raw pointer here until response_value is set. If headers.clone_this(global)? returns early, the newly allocated Response leaks.
🤖 Prompt for 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.
In `@src/runtime/api/html_rewriter.rs` around lines 704 - 707, The header-cloning
error path leaks the newly allocated raw Response because response_value is not
yet initialized. In the Response construction logic around headers.clone_this,
ensure the raw response is freed before propagating the clone error, while
preserving the existing ownership transfer when set_init_headers succeeds.
Stacked on #33820, which introduces `ForeignRef<T>` and converts `FetchHeaders`. ## What this does Extends the owned-handle pattern to every remaining opaque FFI type that Rust holds an ownership unit of, and generates the boilerplate instead of copying it. **1. Owned handles for 16 more types** — each replaces a hand-rolled owner, a `scopeguard`, or a bare raw pointer with a `#[repr(transparent)]` newtype whose `Drop` calls the C release function. Also flips 314 `&mut self` receivers to `&self` on `opaque_ffi!` ZSTs: those types are `UnsafeCell`-backed, so `&T` carries no `noalias` and C mutates through it — the `&mut` asserted an exclusivity that was never true and never needed. **2. `ForeignRef<T, R = DefaultRelease>`** — a release-marker parameter. `ForeignOwned` admits one release per type, so an object with two ownership disciplines could not use it twice: libarchive's `struct archive` is freed by `archive_read_free` when opened for reading and `archive_write_free` when opened for writing, and the write side had degraded into a bespoke owner with a hand-written `Drop`. It is now `ForeignRef<sys::Archive, WriteFree>`. The parameter defaults, so no existing `ForeignRef<T>` changed. **3. `foreign_handle!`** emits the newtype plus `adopt` / `adopt_ptr` / `as_ptr` / `leak` / `raw`. That block had been hand-copied onto 17 types; a missing `mem::forget` in one copy is a double-free the other sixteen would not reveal. Net -437 lines. `adopt` and `adopt_ptr` are now `unsafe`. Most hand-written copies were safe private fns, but adopting a pointer whose ownership unit you were not given is UB, so the obligation belongs at the call site — all 29 now carry a `SAFETY:` comment naming the producer. **4. The remaining C handles**, converted after reading the C and C++ rather than the names: `ENGINE`, `X509`, `X509_STORE`, `X509_STORE_CTX`, `SSL_SESSION`, `spng_ctx`, `WebPDemuxer`, `WebPMux`, and `CookieMapRef` folded onto `ForeignRef`. The certificate handles are careful about where the ref comes from. `SSL_get_peer_certificate`, `X509_up_ref`, `X509_STORE_CTX_get1_issuer` and `d2i_X509` hand over a `+1` and are adopted; `SSL_get_certificate`, `sk_X509_value` and `SSL_CTX_get_cert_store` return borrows and stay raw pointers. `SSL_set0_verify_cert_store` takes ownership, so that path leaks the handle rather than dropping it. ## Two bugs this surfaced - `cppbind` mapped `JSC::SourceProvider` to the owning handle, so the generated **safe** wrapper passed the address of a Rust stack slot to C++ `->deref()`. No caller today, but it also produced two conflicting `extern "C"` declarations of one symbol. - Flipping `Response::upgrade` to `&self` silently moved method resolution to `ResponseLike::upgrade`, which boxes its argument a second time. Rust probes the receiver by-value first, so an inherent `&mut self` method beats a trait method there; once flipped it no longer matches. Only an arity mismatch made it visible. Restores the debug-only corrupted-`HandleSlot` assert that `Strong::destroy` carried before it became a `ForeignRef`; a bad slot otherwise faults inside JSC with no Rust frame. ## Deliberately not converted | type | why | |---|---| | `AbortSignal`, `NapiEnv`, `JSCArrayBuffer` | already owned by `bun_ptr::ExternalShared`, which models ref/deref, not a single unit | | `Blob` (standalone_graph) | the opaque decl is an erased stand-in for a type declared a tier up | | `JSPropertyIteratorImpl` | freed by its enclosing struct's `Drop` | | `Channel` (c-ares) | see below | | `Loop`, `Heap`, `App` | thread/process-lifetime singletons | `ares_destroy()` invokes `query->callback(query->arg, ARES_EDESTRUCTION, 0, NULL)` for every pending query, and those callbacks re-enter `RefPtr::deref`. `Resolver` declares `ref_count` before `channel`, and Rust drops fields in declaration order, so a `Drop`-based owner would free the refcount's debug tables and then let the callbacks read them. The open-coded teardown in `Drop for GlobalData` is load-bearing. Converting it safely means declaring `channel` first, or keeping an explicit `drop(self.channel.take())`; left for a follow-up. `spng_ctx_free`, `WebPDemuxDelete` and `WebPMuxDelete` stay `unsafe` externs behind a plain wrapper: a `safe fn` taking `&sys::T` would let safe code free a context the handle owns. ## Verification - `cargo build -p bun_bin` clean. `cargo check` is not sufficient here: it stops before codegen, so it never evaluates the `const { assert!(size_of::<T>() == 0) }` guards inside `opaque_deref*`, and a rename that turns `NonNull<X>` from "the C object" into "an 8-byte Rust struct" typechecks fine. - `bun bd` clean, no warnings. - Drove each converted subsystem end to end: TLS handshake + peer certificate + session + the `rejectUnauthorized` reject path, PNG and WebP encode/decode round-trips (output byte-identical to before), cookie get/set through `Bun.serve` routes, `bun pm pack` and tarball extraction, `bun:ffi cc`, `--bytecode`, `.npmrc` regex, HMR WebSocket upgrade, `Bun.connect` failure path. Each hammered a few hundred iterations under `Bun.gc(true)` to check refcount balance. - Also adds a `verify` skill capturing the build-and-drive recipe.
| pub fn from_utf8(input: &[u8]) -> Option<Self> { | ||
| Self::from_string(String::borrow_utf8(input)) | ||
| } |
There was a problem hiding this comment.
🟡 This PR converts bun_jsc::URL into a foreign_handle! owning type — URL::from_utf8 now returns Option<URL> (freed on Drop) and URL::destroy is deleted — but src/CLAUDE.md (lines 158–160) still documents the old Option<NonNull<URL>> return and shows unsafe { URL::destroy(url.as_ptr()) } for cleanup. Since the PR is what invalidates that example, it'd be worth refreshing those three lines here (drop the destroy line, note that the returned handle frees on drop). Doc-only.
Extended reasoning...
What the issue is
src/CLAUDE.md lines 155–160 currently read:
use bun_jsc::URL;
let url = URL::from_utf8(href)?; // Option<NonNull<URL>>
// caller owns the C++ object — destroy it when done:
// unsafe { URL::destroy(url.as_ptr()) }This PR changes bun_jsc::URL in src/jsc/URL.rs to be a foreign_handle! owning type: URL::from_utf8 / from_string / from_js now return Option<Self> (an owned handle whose Drop runs URL__deinit), and pub unsafe fn destroy(this: *mut Self) is removed entirely. The onboarding doc therefore now (a) annotates the wrong return type and (b) references a deleted function.
Why this falls in scope for the PR
The PR directly touches URL::from_utf8 (line 99 in the new file) and deletes URL::destroy, so it is the change that invalidates the example. src/CLAUDE.md is not in the changed-files list. Per the repo's own review guidance in the top-level CLAUDE.md — "Delete dead code in the same PR that makes it dead" and the docs-follow-API convention — a doc example that names a function this PR removes should be updated in the same PR.
Step-by-step proof
- Before this PR,
src/jsc/URL.rsdefinedpub unsafe fn destroy(this: *mut Self)and thefrom_*constructors returnedOption<NonNull<URL>>;src/CLAUDE.md:158-160accurately described that contract. - In this PR,
src/jsc/URL.rsgainsbun_opaque::foreign_handle! { pub struct URL(sys::URL) via URL__deinit; }(line 17),from_utf8now returnsOption<Self>(line 99), andfn destroyno longer appears anywhere in the file (grep confirms zero matches). src/CLAUDE.mdis absent from the PR's changed-files list, and reading lines 155–160 on the PR head shows the old text verbatim.- Therefore, after this PR merges, the onboarding doc names
URL::destroy— a function that no longer exists — and annotatesOption<NonNull<URL>>for a call that now yieldsOption<URL>.
Impact
Documentation only, zero runtime effect. Anyone who copies the example will get a compile error (no function or associated item named destroy`` / type mismatch on .as_ptr()) and immediately discover the correct API from `URL.rs`. The concern is purely that the workspace onboarding doc is now stale in a way this PR directly caused.
Suggested fix
Update the three affected lines to reflect the owning-handle contract, e.g.:
let url = URL::from_utf8(href)?; // Option<URL> — owned; frees on Dropand drop the // unsafe { URL::destroy(url.as_ptr()) } line and the "destroy it when done" comment. The rest of the section (url.protocol(), host/hostname note) remains accurate.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/ffi/ffi_body.rs (1)
631-648: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard
state_ptrinCompileC::compile
Err(...)exits here skip cleanup, and the caller only gets the pointer onOk, so TinyCC state leaks on compile failures. Mirror thescopeguardused inFunction::compileso every path destroys it.🤖 Prompt for 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. In `@src/runtime/ffi/ffi_body.rs` around lines 631 - 648, Ensure TinyCC state is cleaned up on every exit path in CompileC::compile. After successful TCC::State::init, add the same scopeguard pattern used by Function::compile to destroy state_ptr, covering deferred-error and subsequent failure paths while preserving the existing error handling.
🤖 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.
Outside diff comments:
In `@src/runtime/ffi/ffi_body.rs`:
- Around line 631-648: Ensure TinyCC state is cleaned up on every exit path in
CompileC::compile. After successful TCC::State::init, add the same scopeguard
pattern used by Function::compile to destroy state_ptr, covering deferred-error
and subsequent failure paths while preserving the existing error handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b4bff9e7-43c5-43b1-bf6a-21a7103a2dc6
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (128)
.claude/skills/verify/SKILL.mdsrc/boringssl/lib.rssrc/boringssl_sys/boringssl.rssrc/bundler_jsc/analyze_jsc.rssrc/cares_sys/c_ares.rssrc/codegen/cppbind.tssrc/http/HTTPContext.rssrc/http/HTTPThread.rssrc/http/InternalState.rssrc/http/compress_body.rssrc/http/h3_client/ClientContext.rssrc/http/h3_client/ClientSession.rssrc/http/h3_client/PendingConnect.rssrc/http/h3_client/Stream.rssrc/http/h3_client/callbacks.rssrc/http/h3_client/encode.rssrc/http_jsc/websocket_client.rssrc/http_jsc/websocket_client/WebSocketDeflate.rssrc/http_jsc/websocket_client/WebSocketUpgradeClient.rssrc/install/TarballStream.rssrc/install/extract_tarball.rssrc/install_types/Cargo.tomlsrc/install_types/NodeLinker.rssrc/jsc/CachedBytecode.rssrc/jsc/CppTask.rssrc/jsc/DOMFormData.rssrc/jsc/DOMURL.rssrc/jsc/Debugger.rssrc/jsc/FetchHeaders.rssrc/jsc/JSCScheduler.rssrc/jsc/JSObject.rssrc/jsc/JSPromise.rssrc/jsc/JSSecrets.rssrc/jsc/JSUint8Array.rssrc/jsc/MarkedArgumentBuffer.rssrc/jsc/RegularExpression.rssrc/jsc/SourceProvider.rssrc/jsc/Strong.rssrc/jsc/TextCodec.rssrc/jsc/URL.rssrc/jsc/URLSearchParams.rssrc/jsc/VirtualMachine.rssrc/jsc/Weak.rssrc/jsc/ZigException.rssrc/jsc/ZigStackTrace.rssrc/jsc/array_buffer.rssrc/jsc/bindgen.rssrc/jsc/event_loop.rssrc/jsc/lib.rssrc/jsc/rare_data.rssrc/jsc/virtual_machine_exports.rssrc/libarchive/lib.rssrc/libdeflate_sys/libdeflate.rssrc/mimalloc_sys/mimalloc.rssrc/opaque/lib.rssrc/options_types/context.rssrc/runtime/api/Archive.rssrc/runtime/api/BunObject.rssrc/runtime/api/bun/SSLContextCache.rssrc/runtime/api/bun/SecureContext.rssrc/runtime/api/bun/x509.rssrc/runtime/api/filesystem_router.rssrc/runtime/bake/DevServer.rssrc/runtime/cli/Arguments.rssrc/runtime/cli/audit_command.rssrc/runtime/cli/pack_command.rssrc/runtime/cli/publish_command.rssrc/runtime/cli/test_command.rssrc/runtime/crypto/CryptoHasher.rssrc/runtime/crypto/EVP.rssrc/runtime/dispatch.rssrc/runtime/dns_jsc/dns.rssrc/runtime/ffi/ffi_body.rssrc/runtime/image/codec_png.rssrc/runtime/image/codec_webp.rssrc/runtime/ipc_host.rssrc/runtime/jsc_hooks.rssrc/runtime/node/zlib/NativeZstd.rssrc/runtime/server/FileRoute.rssrc/runtime/server/RequestContext.rssrc/runtime/server/ServerConfig.rssrc/runtime/server/StaticRoute.rssrc/runtime/server/mod.rssrc/runtime/server/server_body.rssrc/runtime/socket/Listener.rssrc/runtime/socket/SSLConfig.rssrc/runtime/socket/SocketAddress.rssrc/runtime/socket/UpgradedDuplex.rssrc/runtime/socket/WindowsNamedPipe.rssrc/runtime/socket/WindowsNamedPipeContext.rssrc/runtime/socket/socket_body.rssrc/runtime/socket/tls_socket_functions.rssrc/runtime/socket/udp_socket.rssrc/runtime/socket/uws_dispatch.rssrc/runtime/test_runner/ScopeFunctions.rssrc/runtime/test_runner/jest.rssrc/runtime/valkey_jsc/js_valkey.rssrc/runtime/webcore/Blob.rssrc/runtime/webcore/Body.rssrc/runtime/webcore/CookieMap.rssrc/runtime/webcore/Crypto.rssrc/runtime/webcore/FormData.rssrc/runtime/webcore/TextDecoder.rssrc/runtime/webcore/blob/copy_file.rssrc/runtime/webcore/streams.rssrc/sha_hmac/sha.rssrc/sql_jsc/mysql/MySQLConnection.rssrc/sql_jsc/postgres/PostgresSQLConnection.rssrc/sql_jsc/shared/ConnectionCtorArgs.rssrc/tcc_sys/tcc.rssrc/uws/lib.rssrc/uws_sys/App.rssrc/uws_sys/ConnectingSocket.rssrc/uws_sys/ListenSocket.rssrc/uws_sys/Request.rssrc/uws_sys/Response.rssrc/uws_sys/SocketContext.rssrc/uws_sys/WebSocket.rssrc/uws_sys/h3.rssrc/uws_sys/lib.rssrc/uws_sys/quic/Context.rssrc/uws_sys/quic/PendingConnect.rssrc/uws_sys/quic/Stream.rssrc/uws_sys/socket.rssrc/uws_sys/udp.rssrc/uws_sys/us_socket_t.rssrc/zstd/lib.rstest/internal/dead-code-escape-limits.json
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/runtime/socket/UpgradedDuplex.rs (1)
349-379: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winScopeguard armed before the null check it depends on.
ctx_guardis created first and only afterward doesNonNull::new(ctx).expect(...)validate thatctxis non-null. If a caller ever violates the documented "non-null" precondition, the.expect()panic unwinds through the already-armed guard, whose closure callsbun_boringssl_sys::sys::SSL_CTX::opaque_ref(ctx)on a null pointer. Per theopaque_ffi!contract used elsewhere in this PR (e.g.ZSTD_DStream's doc: "&Selfis ABI-identical to a non-null ...*"), constructing a reference from a null pointer is UB — turning a clean, debuggable panic into undefined behavior on the unwind path.Move the null validation before the guard is armed so the panic can never reach the closure with a null pointer.
🐛 Proposed fix: validate before arming the guard
pub(crate) fn start_tls_with_ctx( &mut self, ctx: *mut bun_boringssl_sys::sys::SSL_CTX, is_client: bool, ) -> Result<(), bun_core::Error> { + let ctx_nn = + NonNull::new(ctx).expect("caller passes a non-null SSL_CTX* with one adopted ref"); // errdefer SSL_CTX_free(ctx) — free the adopted ref on the error path only. let ctx_guard = scopeguard::guard(ctx, |ctx| { bun_boringssl_sys::SSL_CTX_free(bun_boringssl_sys::sys::SSL_CTX::opaque_ref(ctx)); }); - let ctx_nn = - NonNull::new(ctx).expect("caller passes a non-null SSL_CTX* with one adopted ref"); self.wrapper = Some(WrapperType::init_with_ctx(Based on learnings and this repo's memory-safety guidelines: "Pair every acquisition with its release at the acquisition site... New early returns or fallible calls → re-audit everything acquired above them." and "Never let a pointer or slice outlive the memory it points into."
🤖 Prompt for 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. In `@src/runtime/socket/UpgradedDuplex.rs` around lines 349 - 379, Validate ctx with NonNull::new(...).expect(...) before creating ctx_guard, then use the validated pointer when arming the scopeguard and initializing the wrapper. Keep the guard responsible only for releasing a known non-null SSL_CTX, preserving its disarm-on-success behavior in start_tls_with_ctx.Source: Coding guidelines
src/runtime/socket/udp_socket.rs (1)
1772-1778: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale
opaque_mutcall left behind by theopaque_refmigration.Every other call to
uws::udp::Socket::connect(...)in this file was migrated toopaque_ref(e.g. lines 625-627), consistent with the stacked PR's conversion of FFI receivers from&mut selfto&self. This site injs_connectstill usesopaque_mut, which still compiles (auto-deref satisfies&self) but is an inconsistent, unmigrated sibling call to the same method.♻️ Proposed fix
- // `Socket` is an `opaque_ffi!` ZST — `opaque_mut` is the safe deref. - if uws::udp::Socket::opaque_mut(socket).connect(connect_host.as_ptr(), port as u32) == -1 { + // `Socket` is an `opaque_ffi!` ZST — `opaque_ref` is the safe deref. + if uws::udp::Socket::opaque_ref(socket).connect(connect_host.as_ptr(), port as u32) == -1 {As per coding guidelines: "Grep for every sibling site sharing the pattern... every caller of a changed helper... stale call sites compile fine and silently miss the new behavior."
🤖 Prompt for 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. In `@src/runtime/socket/udp_socket.rs` around lines 1772 - 1778, In js_connect, replace the stale uws::udp::Socket::opaque_mut(socket) receiver with uws::udp::Socket::opaque_ref(socket) when calling connect, matching the migrated sibling call sites and the updated &self FFI API.Source: Coding guidelines
src/runtime/api/filesystem_router.rs (1)
845-866: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winDo not free
pathname_backingwhileroute_holderstill references it.Line 852 releases the backing bytes before
params_list_holderis cleared and beforeroute_holderis dropped, leaving stored references dangling. Remove the early resets and let theBox’s declaration-order field drops destroy both reference holders beforepathname_backing.As per coding guidelines, never let pointers or slices outlive the memory they point into.
Proposed fix
fn deinit(mut this: Box<MatchedRoute>) { this.query_string_map.set(None); this.param_map.set(None); - if this.needs_deinit { - this.pathname_backing = ZigStringSlice::EMPTY; - *this.params_list_holder.get_mut() = route_param::List::default(); - } if let Some(p) = this.origin.take() {🤖 Prompt for 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. In `@src/runtime/api/filesystem_router.rs` around lines 845 - 866, In MatchedRoute::deinit, remove the manual resets of pathname_backing and params_list_holder inside the needs_deinit block; rely on Box field declaration-order drops so route_holder and other reference holders are destroyed before the backing allocation, preventing dangling slices.Source: Coding guidelines
src/http/h3_client/PendingConnect.rs (1)
41-48: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winCondense the accessor documentation to three lines.
Keep the single-consumption, disjoint-allocation, and thread-affinity invariants.
As per coding guidelines, keep code comments to three lines maximum.
🤖 Prompt for 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. In `@src/http/h3_client/PendingConnect.rs` around lines 41 - 48, Condense the accessor documentation above the relevant `PendingConnect` accessor to three lines maximum, preserving the invariants that the handle is consumed exactly once, allocated separately from `self`, and accessed only from the HTTP thread.Source: Coding guidelines
src/runtime/socket/Listener.rs (1)
80-88: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winDo not destroy an uninitialized
SocketGroupon named-pipe teardown.The Windows named-pipe constructor never initializes or registers
group, butdeinitunconditionally unregisters and destroys it. Track whether the UWS group was initialized and gate both teardown calls.Proposed fix
pub struct Listener { pub group: JsCell<uws::SocketGroup>, + pub group_initialized: bool, } // Named-pipe constructor group: JsCell::new(uws::SocketGroup::default()), +group_initialized: false, // UWS constructor group: JsCell::new(uws::SocketGroup::default()), +group_initialized: true, - bun_core::asan::unregister_root_region(...); - unsafe { uws::SocketGroup::destroy(self.group.as_ptr()) }; + if self.group_initialized { + bun_core::asan::unregister_root_region(...); + unsafe { uws::SocketGroup::destroy(self.group.as_ptr()) }; + }As per coding guidelines: pair every acquisition with its release and update every lifecycle exit when adding mutable state.
Also applies to: 222-237, 302-319, 817-844
🤖 Prompt for 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. In `@src/runtime/socket/Listener.rs` around lines 80 - 88, Track whether Listener.group was initialized, using state set by the constructor path that successfully initializes/registers the UWS SocketGroup. Update every relevant lifecycle exit, including named-pipe teardown and the other referenced cleanup paths, to conditionally unregister and destroy the group only when that state is true, then clear the state after release.Source: Coding guidelines
src/libarchive/lib.rs (1)
246-269: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftDon't return libarchive-backed slices from
next(&self)
archive_read_data_blockreuses that buffer on the next read, so a previousBlock::bytescan outlive the data it points to oncenext()is called again on the same archive. Return owned bytes, or make the API require exclusive access / immediate consumption.🤖 Prompt for 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. In `@src/libarchive/lib.rs` around lines 246 - 269, Change `next` and `Block` so callers cannot retain libarchive-backed borrowed slices across reads: either make `next` require exclusive archive access and return data with a safe lifetime, or preferably copy the buffer into owned storage before constructing `Block`. Update all related type signatures and call sites to use the owned representation.
🤖 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 @.claude/skills/verify/SKILL.md:
- Line 18: Replace the hardcoded personal executable path in the “any command,
from another cwd” row of the verification table with a portable project-relative
invocation or a clearly documented placeholder that users can adapt to their
local Bun debug binary.
In `@src/bundler_jsc/analyze_jsc.rs`:
- Around line 243-250: Condense the documentation on the `IdentifierArray`
handle to no more than three comment lines, preserving that it owns the entire
C++ `JSC::Identifier[]` allocation via `delete[]` and that methods use shared
access while C++ mutates elements through the pointer.
In `@src/jsc/CachedBytecode.rs`:
- Around line 54-62: The generate_for_esm method exposes a 'static byte slice
whose storage is owned by the droppable CachedBytecode handle, enabling
use-after-free. Refactor CachedBytecode and all affected callers to return the
handle without an independent slice, and provide bytes(&self) -> &[u8] or an
equivalent lifetime tied to &self; update the logic covering the related
generation paths so no pointer or slice can outlive its owner.
In `@src/jsc/SourceProvider.rs`:
- Around line 16-23: Trim the documentation above the owned SourceProvider
handle to no more than three lines, preserving only that it owns one intrusive
refcount reference and that Drop releases it, with sharing enforced by the lack
of mutable access.
In `@src/jsc/URL.rs`:
- Around line 99-101: Update the URL usage example in src/CLAUDE.md to match the
owned return type of URL::from_utf8: document handling Option<URL> directly and
remove the obsolete Option<NonNull<URL>> and explicit URL::destroy(...) usage,
updating all related example code atomically.
In `@src/libarchive/lib.rs`:
- Around line 232-235: Safe wrappers expose libarchive APIs that retain buffer,
client_data, and callback pointers beyond the call, allowing use-after-free when
backing storage is dropped. Update read_open_memory and the callback-based write
API wrappers to be unsafe (or redesign them around an owning lifetime wrapper),
and document the caller’s obligation to keep all retained data and callbacks
alive until archive close/free.
In `@src/runtime/image/codec_png.rs`:
- Around line 51-58: Shorten the comments near the iCCP handling and the
referenced sections to at most three lines each. Retain only the contracts
describing profile ownership, mutation behavior, and allocation transfer; remove
explanatory details about PNG validity, return values, and color types.
In `@src/sha_hmac/sha.rs`:
- Around line 17-22: Shorten the doc comment above the re-exported ENGINE in the
sha module to no more than three concise lines, preserving only the essential
ownership/borrowing clarification and compatibility note.
In `@src/uws_sys/quic/PendingConnect.rs`:
- Around line 30-34: Trim the doc comment for resolved() to no more than three
concise lines, preserving only its essential behavior and ownership/return-type
information.
---
Outside diff comments:
In `@src/http/h3_client/PendingConnect.rs`:
- Around line 41-48: Condense the accessor documentation above the relevant
`PendingConnect` accessor to three lines maximum, preserving the invariants that
the handle is consumed exactly once, allocated separately from `self`, and
accessed only from the HTTP thread.
In `@src/libarchive/lib.rs`:
- Around line 246-269: Change `next` and `Block` so callers cannot retain
libarchive-backed borrowed slices across reads: either make `next` require
exclusive archive access and return data with a safe lifetime, or preferably
copy the buffer into owned storage before constructing `Block`. Update all
related type signatures and call sites to use the owned representation.
In `@src/runtime/api/filesystem_router.rs`:
- Around line 845-866: In MatchedRoute::deinit, remove the manual resets of
pathname_backing and params_list_holder inside the needs_deinit block; rely on
Box field declaration-order drops so route_holder and other reference holders
are destroyed before the backing allocation, preventing dangling slices.
In `@src/runtime/socket/Listener.rs`:
- Around line 80-88: Track whether Listener.group was initialized, using state
set by the constructor path that successfully initializes/registers the UWS
SocketGroup. Update every relevant lifecycle exit, including named-pipe teardown
and the other referenced cleanup paths, to conditionally unregister and destroy
the group only when that state is true, then clear the state after release.
In `@src/runtime/socket/udp_socket.rs`:
- Around line 1772-1778: In js_connect, replace the stale
uws::udp::Socket::opaque_mut(socket) receiver with
uws::udp::Socket::opaque_ref(socket) when calling connect, matching the migrated
sibling call sites and the updated &self FFI API.
In `@src/runtime/socket/UpgradedDuplex.rs`:
- Around line 349-379: Validate ctx with NonNull::new(...).expect(...) before
creating ctx_guard, then use the validated pointer when arming the scopeguard
and initializing the wrapper. Keep the guard responsible only for releasing a
known non-null SSL_CTX, preserving its disarm-on-success behavior in
start_tls_with_ctx.
🪄 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: b4bff9e7-43c5-43b1-bf6a-21a7103a2dc6
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (128)
.claude/skills/verify/SKILL.mdsrc/boringssl/lib.rssrc/boringssl_sys/boringssl.rssrc/bundler_jsc/analyze_jsc.rssrc/cares_sys/c_ares.rssrc/codegen/cppbind.tssrc/http/HTTPContext.rssrc/http/HTTPThread.rssrc/http/InternalState.rssrc/http/compress_body.rssrc/http/h3_client/ClientContext.rssrc/http/h3_client/ClientSession.rssrc/http/h3_client/PendingConnect.rssrc/http/h3_client/Stream.rssrc/http/h3_client/callbacks.rssrc/http/h3_client/encode.rssrc/http_jsc/websocket_client.rssrc/http_jsc/websocket_client/WebSocketDeflate.rssrc/http_jsc/websocket_client/WebSocketUpgradeClient.rssrc/install/TarballStream.rssrc/install/extract_tarball.rssrc/install_types/Cargo.tomlsrc/install_types/NodeLinker.rssrc/jsc/CachedBytecode.rssrc/jsc/CppTask.rssrc/jsc/DOMFormData.rssrc/jsc/DOMURL.rssrc/jsc/Debugger.rssrc/jsc/FetchHeaders.rssrc/jsc/JSCScheduler.rssrc/jsc/JSObject.rssrc/jsc/JSPromise.rssrc/jsc/JSSecrets.rssrc/jsc/JSUint8Array.rssrc/jsc/MarkedArgumentBuffer.rssrc/jsc/RegularExpression.rssrc/jsc/SourceProvider.rssrc/jsc/Strong.rssrc/jsc/TextCodec.rssrc/jsc/URL.rssrc/jsc/URLSearchParams.rssrc/jsc/VirtualMachine.rssrc/jsc/Weak.rssrc/jsc/ZigException.rssrc/jsc/ZigStackTrace.rssrc/jsc/array_buffer.rssrc/jsc/bindgen.rssrc/jsc/event_loop.rssrc/jsc/lib.rssrc/jsc/rare_data.rssrc/jsc/virtual_machine_exports.rssrc/libarchive/lib.rssrc/libdeflate_sys/libdeflate.rssrc/mimalloc_sys/mimalloc.rssrc/opaque/lib.rssrc/options_types/context.rssrc/runtime/api/Archive.rssrc/runtime/api/BunObject.rssrc/runtime/api/bun/SSLContextCache.rssrc/runtime/api/bun/SecureContext.rssrc/runtime/api/bun/x509.rssrc/runtime/api/filesystem_router.rssrc/runtime/bake/DevServer.rssrc/runtime/cli/Arguments.rssrc/runtime/cli/audit_command.rssrc/runtime/cli/pack_command.rssrc/runtime/cli/publish_command.rssrc/runtime/cli/test_command.rssrc/runtime/crypto/CryptoHasher.rssrc/runtime/crypto/EVP.rssrc/runtime/dispatch.rssrc/runtime/dns_jsc/dns.rssrc/runtime/ffi/ffi_body.rssrc/runtime/image/codec_png.rssrc/runtime/image/codec_webp.rssrc/runtime/ipc_host.rssrc/runtime/jsc_hooks.rssrc/runtime/node/zlib/NativeZstd.rssrc/runtime/server/FileRoute.rssrc/runtime/server/RequestContext.rssrc/runtime/server/ServerConfig.rssrc/runtime/server/StaticRoute.rssrc/runtime/server/mod.rssrc/runtime/server/server_body.rssrc/runtime/socket/Listener.rssrc/runtime/socket/SSLConfig.rssrc/runtime/socket/SocketAddress.rssrc/runtime/socket/UpgradedDuplex.rssrc/runtime/socket/WindowsNamedPipe.rssrc/runtime/socket/WindowsNamedPipeContext.rssrc/runtime/socket/socket_body.rssrc/runtime/socket/tls_socket_functions.rssrc/runtime/socket/udp_socket.rssrc/runtime/socket/uws_dispatch.rssrc/runtime/test_runner/ScopeFunctions.rssrc/runtime/test_runner/jest.rssrc/runtime/valkey_jsc/js_valkey.rssrc/runtime/webcore/Blob.rssrc/runtime/webcore/Body.rssrc/runtime/webcore/CookieMap.rssrc/runtime/webcore/Crypto.rssrc/runtime/webcore/FormData.rssrc/runtime/webcore/TextDecoder.rssrc/runtime/webcore/blob/copy_file.rssrc/runtime/webcore/streams.rssrc/sha_hmac/sha.rssrc/sql_jsc/mysql/MySQLConnection.rssrc/sql_jsc/postgres/PostgresSQLConnection.rssrc/sql_jsc/shared/ConnectionCtorArgs.rssrc/tcc_sys/tcc.rssrc/uws/lib.rssrc/uws_sys/App.rssrc/uws_sys/ConnectingSocket.rssrc/uws_sys/ListenSocket.rssrc/uws_sys/Request.rssrc/uws_sys/Response.rssrc/uws_sys/SocketContext.rssrc/uws_sys/WebSocket.rssrc/uws_sys/h3.rssrc/uws_sys/lib.rssrc/uws_sys/quic/Context.rssrc/uws_sys/quic/PendingConnect.rssrc/uws_sys/quic/Stream.rssrc/uws_sys/socket.rssrc/uws_sys/udp.rssrc/uws_sys/us_socket_t.rssrc/zstd/lib.rstest/internal/dead-code-escape-limits.json
| | Need | Use | | ||
| |---|---| | ||
| | run a script, stay in the repo | `bun bd run /path/to/drive.js` | | ||
| | any command, from another cwd | `/Users/jarred/code/bun/build/debug/bun-debug <cmd>` | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Hardcoded personal path in shared skill doc.
/Users/jarred/code/bun/build/debug/bun-debug only resolves on one contributor's machine. Any other user/agent following this table row for "any command, from another cwd" will get a bogus path.
✏️ Proposed fix
-| any command, from another cwd | `/Users/jarred/code/bun/build/debug/bun-debug <cmd>` |
+| any command, from another cwd | `<repo>/build/debug/bun-debug <cmd>` (absolute path required) |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | any command, from another cwd | `/Users/jarred/code/bun/build/debug/bun-debug <cmd>` | | |
| | any command, from another cwd | `<repo>/build/debug/bun-debug <cmd>` (absolute path required) | |
🧰 Tools
🪛 SkillSpector (2.3.7)
[error] 39: [PE3] Credential Access: Code accesses credential files (SSH keys, AWS credentials, etc.). This could indicate credential theft attempts.
Remediation: Remove references to credential paths. Use environment variables or secrets managers. For docs, use placeholder paths (e.g., /path/to/config). Never load .env or token files in production code paths.
(Privilege Escalation (PE3))
🤖 Prompt for 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.
In @.claude/skills/verify/SKILL.md at line 18, Replace the hardcoded personal
executable path in the “any command, from another cwd” row of the verification
table with a portable project-relative invocation or a clearly documented
placeholder that users can adapt to their local Bun debug binary.
| // C++ allocates (`new Identifier[len]`) and hands back the array. One | ||
| // `IdentifierArray` handle owns that whole allocation. | ||
| bun_opaque::foreign_handle! { | ||
| /// Owned handle to a C++ `JSC::Identifier[]`. | ||
| /// | ||
| /// The pointer is the base of the array, so the handle owns every element | ||
| /// (`delete[]`), not one. Every method takes `&self`: C++ writes elements | ||
| /// through the same pointer, so there is no `&mut self` to have. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Condense this handle documentation to three lines.
Keep the array-wide ownership and shared-mutation contract.
As per coding guidelines, keep code comments to three lines maximum.
🤖 Prompt for 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.
In `@src/bundler_jsc/analyze_jsc.rs` around lines 243 - 250, Condense the
documentation on the `IdentifierArray` handle to no more than three comment
lines, preserving that it owns the entire C++ `JSC::Identifier[]` allocation via
`delete[]` and that methods use shared access while C++ mutates elements through
the pointer.
Source: Coding guidelines
| /// Bytecode generation. Each successful call returns the `+1` C++ handed us. | ||
| impl CachedBytecode { | ||
| // SAFETY CONTRACT: the returned `&'static [u8]` actually borrows from the | ||
| // `CachedBytecode` handle and is invalidated when `deref()` is called. Callers own | ||
| // the handle and must call `deref()` (or drop via `allocator()`) to free. | ||
| // `CachedBytecode` handle and is invalidated when that handle is dropped. | ||
| // Callers must keep it alive for as long as they read the slice. | ||
| pub fn generate_for_esm( | ||
| source_provider_url: &mut BunString, | ||
| input: &[u8], | ||
| ) -> Option<(&'static [u8], NonNull<CachedBytecode>)> { | ||
| let mut this: Option<NonNull<CachedBytecode>> = None; | ||
| ) -> Option<(&'static [u8], Self)> { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Do not expose a 'static slice backed by a droppable handle.
Safe callers can drop or discard handle and continue reading bytes, causing use-after-free. Return an owner with bytes(&self) -> &[u8], or otherwise bind the slice lifetime to the handle.
As per coding guidelines, never let a pointer or slice outlive the memory it points into.
Also applies to: 78-88, 94-132
🤖 Prompt for 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.
In `@src/jsc/CachedBytecode.rs` around lines 54 - 62, The generate_for_esm method
exposes a 'static byte slice whose storage is owned by the droppable
CachedBytecode handle, enabling use-after-free. Refactor CachedBytecode and all
affected callers to return the handle without an independent slice, and provide
bytes(&self) -> &[u8] or an equivalent lifetime tied to &self; update the logic
covering the related generation paths so no pointer or slice can outlive its
owner.
Source: Coding guidelines
| /// Owned handle to a C++ `JSC::SourceProvider` (a `WTF::RefCounted`). | ||
| /// | ||
| /// Holds one ref on the intrusive refcount; `Drop` gives it back. There is no | ||
| /// `&mut self` API and no `DerefMut`: a refcount is shared by definition, and | ||
| /// JSC mutates the provider through the same pointer. | ||
| /// | ||
| /// `Option<SourceProvider>` niche-optimizes to a single thin pointer, so it is | ||
| /// exactly the ABI of the C++ `JSC::SourceProvider*` struct field. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Trim this documentation to the three-line repository limit.
Preserve only the ownership and shared-refcount invariant.
As per coding guidelines, keep code comments to three lines maximum.
🤖 Prompt for 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.
In `@src/jsc/SourceProvider.rs` around lines 16 - 23, Trim the documentation above
the owned SourceProvider handle to no more than three lines, preserving only
that it owns one intrusive refcount reference and that Drop releases it, with
sharing enforced by the lack of mutable access.
Source: Coding guidelines
| pub fn from_utf8(input: &[u8]) -> Option<Self> { | ||
| Self::from_string(String::borrow_utf8(input)) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the stale src/CLAUDE.md URL example.
It still documents Option<NonNull<URL>> and explicit URL::destroy(...), which no longer compile with this owned return type.
As per coding guidelines, update every consumer atomically when an API contract changes.
🧰 Tools
🪛 GitHub Check: Claude Code Review
[warning] 99-101: src/CLAUDE.md URL example is stale after URL becomes an owning handle
This PR converts bun_jsc::URL into a foreign_handle! owning type — URL::from_utf8 now returns Option<URL> (freed on Drop) and URL::destroy is deleted — but src/CLAUDE.md (lines 158–160) still documents the old Option<NonNull<URL>> return and shows unsafe { URL::destroy(url.as_ptr()) } for cleanup. Since the PR is what invalidates that example, it'd be worth refreshing those three lines here (drop the destroy line, note that the returned handle frees on drop). Doc-only.
🤖 Prompt for 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.
In `@src/jsc/URL.rs` around lines 99 - 101, Update the URL usage example in
src/CLAUDE.md to match the owned return type of URL::from_utf8: document
handling Option<URL> directly and remove the obsolete Option<NonNull<URL>> and
explicit URL::destroy(...) usage, updating all related example code atomically.
Sources: Coding guidelines, Linters/SAST tools
| pub fn read_open_memory(&self, buf: &[u8]) -> Result { | ||
| // SAFETY: self valid; buf outlives the archive (caller contract, | ||
| // see `BufferReadStream::buf` field comment). | ||
| unsafe { archive_read_open_memory(self.as_mut_ptr(), buf.as_ptr().cast(), buf.len()) } | ||
| // SAFETY: buf outlives the archive (caller contract, see | ||
| // `BufferReadStream::buf` field comment). | ||
| unsafe { archive_read_open_memory(self, buf.as_ptr().cast(), buf.len()) } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
rg -n -C5 \
'archive_read_open_memory|archive_write_open2|client_data' \
src/libarchive vendor/libarchiveRepository: oven-sh/bun
Length of output: 2031
🌐 Web query:
libarchive archive_read_open_memory buffer valid until archive_close archive_write_open2 client_data callbacks retained lifetime
💡 Result:
In libarchive, the lifetime requirements for buffers and client data are strict: 1. archive_read_open_memory: The memory buffer passed to this function must remain valid and unchanged for the entire duration that the archive object is being read, specifically until after the final call to archive_read_close (or archive_read_free) has completed [1][2][3]. The library does not copy this buffer; it reads directly from the provided memory address [1][4]. 2. archive_write_open2 and client_data: When using archive_write_open2 or other callback-based opening functions, the client_data pointer—and any resources it manages—must remain valid until the archive is fully closed and freed [5][6][7][8]. Specifically, the library expects these resources to be accessible during the invocation of any registered callbacks (open, write, close, and free) [5][8]. 3. Callback Lifetime: The client_data remains associated with the archive object from the moment it is passed to the open function until the archive is destroyed [7]. If you require specific cleanup logic for your client_data, you should provide an archive_free_callback to archive_write_open2 [5][6][8]. This free callback is specifically designed to allow the client to release resources associated with client_data once the library is finished with them [5][8]. In summary, the caller is responsible for the complete lifecycle management of the memory buffer and client_data, ensuring they persist at least until the corresponding archive object has been fully closed and freed [2][5][7].
Citations:
- 1: https://www.daemon-systems.org/man/archive_read_open_memory.3.html
- 2: https://docs.rs/libarchive2/latest/libarchive2/struct.ReadArchive.html
- 3: https://man.netbsd.org/archive_read_open.3
- 4: https://fuchsia.googlesource.com/third_party/libarchive/+/refs/heads/upstream/utf8/libarchive/archive_read_open_memory.c
- 5: https://man.archlinux.org/man/core/libarchive/archive_write_open.3.en
- 6: https://man.archlinux.org/man/archive_write_open.3.en
- 7: https://fuchsia.googlesource.com/third_party/libarchive/+/refs/tags/v2.7.1/libarchive/archive_write.c
- 8: https://manpages.debian.org/trixie/libarchive-dev/archive_write_open.3.en.html
Keep the libarchive retained pointers behind an unsafe boundary. archive_read_open_memory keeps buf valid for the archive’s lifetime, and the callback-based write APIs keep client_data and callbacks live until close/free. As written, these safe wrappers can hand out a use-after-free if the backing storage is dropped early. Mark them unsafe or move the lifetime into an owning wrapper.
🤖 Prompt for 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.
In `@src/libarchive/lib.rs` around lines 232 - 235, Safe wrappers expose
libarchive APIs that retain buffer, client_data, and callback pointers beyond
the call, allowing use-after-free when backing storage is dropped. Update
read_open_memory and the callback-based write API wrappers to be unsafe (or
redesign them around an owning lifetime wrapper), and document the caller’s
obligation to keep all retained data and callbacks alive until archive
close/free.
| /// iCCP chunk read/write — PNG carries an optional ICC profile alongside | ||
| /// the pixels for every colour type (including indexed). `spng_get_iccp` | ||
| /// returns non-zero when the source has no iCCP (or the chunk was | ||
| /// malformed); we treat all non-zero returns the same way — drop the | ||
| /// profile — because the pixels are still valid and a PNG without iCCP | ||
| /// is still a valid PNG. The `profile` pointer it hands back is owned by | ||
| /// the context and freed with `spng_ctx_free`; dupe out before then. | ||
| fn spng_get_iccp(ctx: *mut spng_ctx, iccp: *mut Iccp) -> c_int; | ||
| fn spng_set_iccp(ctx: *mut spng_ctx, iccp: *const Iccp) -> c_int; | ||
| /// the context and freed when the owning [`spng_ctx`] handle drops; dupe | ||
| /// out before then. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Shorten these comments to the repository’s three-line limit.
Retain only the ownership, mutation, and allocation-transfer contracts.
As per coding guidelines, keep code comments to three lines maximum.
Also applies to: 75-80, 88-92
🤖 Prompt for 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.
In `@src/runtime/image/codec_png.rs` around lines 51 - 58, Shorten the comments
near the iCCP handling and the referenced sections to at most three lines each.
Retain only the contracts describing profile ownership, mutation behavior, and
allocation transfer; remove explanatory details about PNG validity, return
values, and color types.
Source: Coding guidelines
| /// The C `ENGINE` object, **not** `bun_boringssl_sys`'s owning `ENGINE` | ||
| /// handle: `hash(.., engine)` below only borrows the VM-owned engine pointer | ||
| /// (`s3_signing::credentials` passes null), so nothing here releases a unit. | ||
| /// Kept under the name `ENGINE` so `bun_sha_hmac::sha::ffi::ENGINE` path | ||
| /// consumers need no edit. | ||
| pub use bun_boringssl_sys::sys::ENGINE; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Trim doc comment to repo's 3-line limit.
The comment on ENGINE spans 5 lines. As per coding guidelines, "Keep code comments to 3 lines max - Comments must be concise. If the code needs more explanation than that, it belongs in docs."
✏️ Suggested trim
- /// The C `ENGINE` object, **not** `bun_boringssl_sys`'s owning `ENGINE`
- /// handle: `hash(.., engine)` below only borrows the VM-owned engine pointer
- /// (`s3_signing::credentials` passes null), so nothing here releases a unit.
- /// Kept under the name `ENGINE` so `bun_sha_hmac::sha::ffi::ENGINE` path
- /// consumers need no edit.
+ /// Bare C `ENGINE`, borrowed only (never released) by `hash(.., engine)`.
pub use bun_boringssl_sys::sys::ENGINE;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// The C `ENGINE` object, **not** `bun_boringssl_sys`'s owning `ENGINE` | |
| /// handle: `hash(.., engine)` below only borrows the VM-owned engine pointer | |
| /// (`s3_signing::credentials` passes null), so nothing here releases a unit. | |
| /// Kept under the name `ENGINE` so `bun_sha_hmac::sha::ffi::ENGINE` path | |
| /// consumers need no edit. | |
| pub use bun_boringssl_sys::sys::ENGINE; | |
| /// Bare C `ENGINE`, borrowed only (never released) by `hash(.., engine)`. | |
| pub use bun_boringssl_sys::sys::ENGINE; |
🤖 Prompt for 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.
In `@src/sha_hmac/sha.rs` around lines 17 - 22, Shorten the doc comment above the
re-exported ENGINE in the sha module to no more than three concise lines,
preserving only the essential ownership/borrowing clarification and
compatibility note.
Source: Coding guidelines
| /// The connected socket, or `None` if the name lookup failed. | ||
| /// | ||
| /// Returns `NonNull`, not `&mut Socket`: the socket is C-owned, and minting a | ||
| /// `&mut` from `&self` would let two live `&mut Socket` exist (and trips | ||
| /// `clippy::mut_from_ref`). Callers reborrow via `Socket::opaque_mut`. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Trim doc comment to repo's 3-line limit.
The doc comment on resolved() spans 5 lines. As per coding guidelines, "Keep code comments to 3 lines max - Comments must be concise. If the code needs more explanation than that, it belongs in docs."
✏️ Suggested trim
- /// The connected socket, or `None` if the name lookup failed.
- ///
- /// Returns `NonNull`, not `&mut Socket`: the socket is C-owned, and minting a
- /// `&mut` from `&self` would let two live `&mut Socket` exist (and trips
- /// `clippy::mut_from_ref`). Callers reborrow via `Socket::opaque_mut`.
+ /// Connected socket, or `None` on lookup failure. `NonNull` (not `&mut`)
+ /// avoids aliasing a C-owned `Socket`; reborrow via `Socket::opaque_mut`.
pub fn resolved(&self) -> Option<NonNull<Socket>> {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// The connected socket, or `None` if the name lookup failed. | |
| /// | |
| /// Returns `NonNull`, not `&mut Socket`: the socket is C-owned, and minting a | |
| /// `&mut` from `&self` would let two live `&mut Socket` exist (and trips | |
| /// `clippy::mut_from_ref`). Callers reborrow via `Socket::opaque_mut`. | |
| /// Connected socket, or `None` on lookup failure. `NonNull` (not `&mut`) | |
| /// avoids aliasing a C-owned `Socket`; reborrow via `Socket::opaque_mut`. | |
| pub fn resolved(&self) -> Option<NonNull<Socket>> { |
🤖 Prompt for 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.
In `@src/uws_sys/quic/PendingConnect.rs` around lines 30 - 34, Trim the doc
comment for resolved() to no more than three concise lines, preserving only its
essential behavior and ownership/return-type information.
Source: Coding guidelines
2 participants
What
bun_opaquegains a generic owned handle for C++-allocated objects that Rust releases through an FFI destructor:ForeignOwned— "this opaque type has a release extern"ForeignRef<T>—Deref+Drop,#[repr(transparent)]overNonNull<T>, zero costforeign_owned!(T, release_fn)— one line to get bothFetchHeadersis the first user, and the owned handle takes the public name (FetchHeaders : HeadersRef :: Path : PathBuf, with the raw ZST moved to an extern-onlysysmodule). Four such handles were previously hand-rolled and ~27 more C++ types are still released by hand at every call site; those are the follow-up.The rest of the diff converts the
unsafe { &mut *ptr }pattern this grew out of — raw-pointer fields become the in-tree owners (JsCell,Cell,ParentRef,BackRef,Box<Self>receivers) across the runtime, and receivers that only ever needed&selfnow say so.unsafe { &mut * }goes from 1241 sites to 807.No allocation, lock,
Rc/Arc, or refcount is added anywhere in the diff. (Checked per-round: every addedheap::release/RefPtr::new/Box::leakreplaces an existingBox::into_raw(Box::new(..))1:1.)Why
&selfeverywhere onFetchHeadersAn
opaque_ffi!type is a#[repr(C)] UnsafeCell<[u8;0]>ZST precisely so that&Tcarries nonoalias/readonlyand C++ may mutate through it.&mut selfon such a type asserts an exclusivity that is never true (C++ owns the object and re-enters through the same pointer) and never needed (every FFI shim already takes&T). All 18 receivers were&mut self; that one wrong choice had grown aDerefMut, two&self -> &mutaccessors, a duplicate RAII handle, and forced callers to launder&Tinto*mut T.Behavior fix included
Making the receivers honest stopped
server.fetch(url, { headers })from compiling, which surfaced that it was sharing the caller'sHeaderswith the internalRequestinstead of copying it. Two user-visible consequences: a header the handler set onreq.headersshowed up on the caller'sHeadersobject, and the two owners disagreed about the object's lifetime. It now copies viaclone_this, the same pathnew Response(_, { headers })already takes. Regression test added; the aliasing half is asserted deterministically and the whole test also runs under the AddressSanitizer CI job.Verification
cargo check --workspaceclean;bun bd(debug + ASan) cleanserve/fetch/headers/node-http2/fs/resolve/bunshell: 1,789 pass, 0 real failuresheaders/node-http2/serve(642 tests): 0 reports-Zmiri-tree-borrows: 27 crates, 146 tests, 0 UB