Repository navigation
Remove 125 unsafe blocks from IPC, N-API, bun:ffi, node:zlib, c-ares, BoringSSL, node:quic and the parser - #47944
Conversation
… is up `CHANNEL` held `Option<NonNull<IPCInstance>>` and `get_ipc_instance` returned `Option<*mut IPCInstance>`, so `send`, `close_child_ipc`, `global_object_connected_ipc` and `get_ipc_instance` itself dereferenced a raw pointer. A channel that came up is never freed. The only free was `IPCInstance::deinit`, on the two paths where the socket (on Windows the pipe) could not be set up, and it was needed only because the box was made and published before that attempt. `get_ipc_instance` sets the transport up on the `RefPtr<SendQueue>` it holds and boxes it after that has succeeded: `Box::leak` gives the `&'static IPCInstance` that `CHANNEL` stores and the function returns. On failure it detaches the queue and the `RefPtr` drops. `IPCInstance::new` and `deinit` are gone, and the socket group stays the `&mut SocketGroup` that `spawn_ipc_group` returns. What differs when it runs: `CHANNEL` is set after `from_fd` and `set_timeout` (on Windows after `windows_configure_client`) instead of before. Only `get_ipc_instance`, `global_object_connected_ipc` and `global_object_has_ipc` read it, and none of them runs in between: `us_socket_from_fd` arms the poll, links the socket and may start the date header timer. The failing path neither allocates the box nor touches `CHANNEL`, and it releases the queue after the warning is printed instead of before. The comment on the test of the failing path in spawn.ipc.test.ts named `IPCInstance.deinit`. It says what the path does now.
Each was declared with a raw handle pointer and called in an `unsafe` block from a safe method on `&self`, which passed its `as_mut_ptr()`. The handle is the only pointer they take. They take it by reference, as the `safe fn` shims over `JSGlobalObject` do: - `Bun__napi_check_gc` reads `nm_version`, which never changes, and asks the heap whether the current thread is doing GC work, which any thread may ask. It can abort the process. - `NapiHandleScope__escape` answers false once the escape slot is used or the scope is closed. It took the value as `usize`; it takes `JSValue`, which is `repr(transparent)` over it. The empty value is fine: the write barrier and the visitor skip a null cell. The other shims over `NapiEnv` stay unsafe imports: they belong on the env's JS thread, and a `&NapiEnv` is also held on pool threads and by the collector. Same calls with the same arguments.
…`&T`
The 44 N-API functions written in Rust that begin with `get_env!`, `preamble!`
or `ungated!` took `env_: *mut NapiEnv`, and `get_env!` made the reference with
`unsafe { $env.as_ref() }`. `napi_escape_handle` did the same with its scope,
`napi_ref_threadsafe_function` and `napi_unref_threadsafe_function` with the env
in a debug assertion, and three functions that only Bun's C++ calls with
`&*env_` or `as_ref()`. `napi_open_callback_scope` and
`napi_close_callback_scope` ignore their env and get the same type.
`NapiEnv` and `NapiHandleScope` are `opaque_ffi!` types: zero-sized, align 1 and
`!Freeze`, so every non-null pointer is a valid reference to one, and
`Option<&T>` has the ABI of a nullable pointer. The parameter types say what the
macro did: null is `None` and still answers `napi_invalid_arg`.
Call sites of the three that addons cannot reach:
- `Bun__napi_register_cleanup_zig(env: &NapiEnv)`: `this`, in the body of the
`NapiEnv` constructor (napi.h).
- `Bun__napi_run_finalizers_after_teardown_wait(env: &NapiEnv)`: `this`, in
`NapiEnv::cleanup()` (napi.h).
- `Bun__napi_enqueue_finalizer(env: Option<&NapiEnv>, ..)`, five: `this` in
`enqueueRefFinalizer` and `doFinalizer` (napi.h), an env dereferenced just
before in `drainOneRefFinalizer` (NapiRef.cpp), one that
`node_api_post_finalizer` has tested for null (napi.cpp), and `RefPtr::get()`
in `NapiFinalizer::call` (napi_finalizer.cpp), for which it keeps its null test.
Every null test is where it was. `type napi_env` stays for the declarations of
the functions written in C++ and for the callbacks into addons.
…e borrows `Finalizer::run(&mut self)` only reads its fields, and it made an unbounded `&NapiEnv` from `self.env.get()`. It takes `&self` and borrows the env from the `NapiEnvRef` it holds, so the reference lives as long as that ref. Its three callers own the `Finalizer` (two locals, one box just taken back) and drop `mut`. `NapiFinalizerTask::schedule` made the same unbounded reference twice. Both go through the `NapiEnvRef` as well. Each borrow ends with its statement, before `self` is given away: `vm_handle()` returns a `BorrowedRef`, which has no lifetime, and `bun_vm()` returns `&'static VirtualMachine`. The one other use of `global_this` was `global_this.bun_vm()`, which is `vm`. `as_mut()` reads the thread-local itself and uses its receiver in a debug assertion only, so a release build runs the same code. `napi_async_work::run_from_js` dereferenced `env` a second time for `surface_exception`. `env_ref`, made from the same pointer, is still in use after that line: the handle scope guard holds it to the end of the function. `napi_create_promise` read the promise back through the raw pointer it had just made from its box. It reads the value first, with `JSPromiseStrong::value()`, and gives the box away after, so `*deferred` is stored after that read instead of before it. `get().as_value(env.to_js())` reached the same value the long way: it downcast the value in the strong slot to `*mut JSPromise` and turned that pointer back into a `JSValue`. Two FFI calls go (the downcast, `JSC__JSValue__asPromise`, and `NapiEnv__globalObject` for an argument that `as_value` ignores), and with them the `is_cell` test and the null test in `as_promise`, the unwrap of its result and the null test in `opaque_mut`. `napi_node_version::release` was a `*const c_char` to a literal, which needed an `unsafe impl Sync` for the static. It is a `&'static [u8; 5]`, `b"node\0"`: a thin pointer at the same offset, and the struct is `Sync` by itself. The inventory of the `vm-thread-door` lint drops the entry.
`Config::options` was an `Option<NonNull<ZStr>>`, "kept as a raw ptr rather than a borrow", and `State::init` turned it back into a reference in two `unsafe` blocks on the caller's word that the string outlives the call. Both callers start from a `&ZStr` and build the `Config` in the argument of `init`, and `tcc_set_options` copies what it parses, so a borrow for the call says the same. `Config` has a lifetime and holds the `&ZStr`. In `CompileC::compile` the string can borrow `self.flags`, so the `*mut CompileC` for the error handler is taken before that borrow starts instead of in the `Config` literal. The handler only pushes to `deferred_errors`. Same pointers and the same calls.
`make_napi_env_if_needed` cast the `*mut c_void` from `make_napi_env_for_ffi` to `&'static NapiEnv`. Its one caller hands the result to `Function::compile`, which turned it back into an address for `add_symbol`, where it is a symbol's value and is not dereferenced. `'static` was true (`makeNapiEnvForFFI` returns `&out.leakRef()`, so the allocation is never freed), but nothing used the reference as a reference. The function returns `NonNull::new(..)` of the pointer it got, an `Option<NonNull<c_void>>` with the same niche, and `compile` passes `as_ptr()`. `CompileC::compile` already passes that pointer as it comes. The comment at `add_symbol` called the env VM-owned; it names the leaked ref. The doc of `make_napi_env_for_ffi` called it "owned by this global" and ended "callers in `bun_runtime` cast to `*mut NapiEnv`", which neither caller does. It says that C++ leaks a ref, and the clause is gone. `to_buffer` built a `&mut [u8]` over the caller's foreign memory for `create_buffer_with_ctx`, a private function with this one caller that split the slice into pointer and length again for C++. Its call of `JSBuffer__bufferFromPointerAndLengthAndDeinit` is written into `to_buffer`, with the pointer and length that `get_ptr_slice` returned. Debug builds lose the precondition check inside `from_raw_parts_mut`; release builds make the same call. The helper declared that symbol itself, so the call was made from `bun_runtime` before too; the declaration stands at the head of `to_buffer`. `JSValue::create_buffer_with_ctx` in `bun_jsc` makes the same call but is neither generic nor `#[inline]`, so it would stand between `to_buffer` and C++ wherever the linker does not inline it. `to_array_buffer` built the same kind of slice only for `ArrayBuffer::from_bytes`, which stores the slice's pointer, its length in `len` and `byte_len`, and the type, and takes the other fields from `Default`. A struct literal sets the same four fields from the pointer and length of `get_ptr_slice` and takes the rest from `Default` too. The `unsafe` block stays for `to_js_with_context`, an `unsafe fn`. Debug builds lose the same check here. `noop_bytes_deallocator` has an empty body and is not an `unsafe fn`. It still coerces to the `unsafe extern "C" fn` pointer that the deallocator slot holds.
`json::decode_ipc_message` wraps an ASCII line in an external string and gave the address of a local `bool` as the context of the string's free callback, which set the flag through the `*mut bool` in an `unsafe` block. `create_external` is generic over the context and documents it as a raw pointer or `&T`. The flag is a `Cell<bool>`, the context is `&Cell<bool>` and the callback calls `set(true)`. The callback has two callers: `create_external` itself when the line is too long, and `ExternalStringImpl::freeExternalBuffer` when the string is destroyed, with the `m_freeCtx` it stored at creation (`releaseBufferEarly` also calls it; nothing calls that). The `unsafe` on `create_external` stays, and its comment names the flag too: the string carries the lifetime of neither borrow. The same byte is stored and read back.
`SqlRuntimeHooks::ssl_config_from_js` was an `unsafe fn(&JSGlobalObject, JSValue)`
and its one call sat in an `unsafe` block whose comment named no obligation ("may
run JS getters / throw"). The one implementation calls `SSLConfig::from_js` and
boxes the result, all safe. The field is a `fn`, the implementation and the call
are safe.
`timer_heap` was an `unsafe fn(*mut VirtualMachine) -> *mut c_void` and states no
obligation either: the one implementation ignores its argument and calls the safe
`timer_all()`. The field and the implementation are a `fn`. The block at its one
call stays, for the dereference of the result.
An `unsafe fn` does not coerce to a `fn` field, so an implementation of either
hook has to be safe. The same indirect calls.
`ChromeProcess::spawn` and the macOS `HostProcess::spawn` are safe functions that took `vm: *mut VirtualMachine` and dereferenced it twice. Each has one caller, which holds a `&JSGlobalObject`: `Bun__Chrome__ensure` passed `bun_vm_ptr()`, and `Bun__WebViewHost__ensure` cast the `&'static VirtualMachine` from `bun_vm()` to `*mut`. Both callers pass `global.bun_vm()`. `VirtualMachine::event_loop` and `EventLoopHandle::init` are safe functions, so that block goes in each. The block that dereferences the raw `transpiler.env` stays. Its comment in ChromeProcess.rs called `map` a `&mut Map` slot; it is a field of the loader, and the sentence goes. In a release build `bun_vm()` and `bun_vm_ptr()` are the same read of the thread-local, so the same code runs.
…unction `uv_posix::unsupported` reached the crash handler, which is Rust, through its C symbol. It declared `CrashHandler__unsupportedUVFunction` itself, returning `()` where the definition returns `!`, called it in an `unsafe` block with the pointer of its `&'static CStr`, and put `unreachable!` after the call. The body of the symbol moves into `pub fn unsupported_uv_function(name_bytes: &'static [u8]) -> !`. The symbol, which `__bun_throw_not_implemented` in uv-posix-polyfills.c calls, keeps its `cstr` and calls that function. `uv_posix` calls it too and has no declaration of its own. Only the C symbol needed the NUL terminator, so `unsupported`, `handle_type` and `ref_state` take the name as `&'static [u8]`, and the six literals are `b"uv_close"` and so on. The literals are one byte shorter. The rest differs only on a path that ends the process: from `uv_posix` the length of the name comes with the slice instead of from a `strlen`, which is one more instruction before the call in each of the six. From C the counter is bumped after the `strlen` instead of before it, and the reported stack starts one frame deeper unless the function is inlined into the symbol.
`run_from_js_thread` and `release_unrun` were `unsafe fn(*mut T)`. Each turned
the pointer into a `ParentRef` behind an `expect` and released the ref that
`write()` took with `unsafe { T::deref(this_ptr) }`, once on each of their four
exits.
Both now take `RefPtr<T>`, read the stream through it and `drop` it where the
`deref` was, after `poll_ref.unref`. Their two callers, the dispatch arm and
`Taskable::release_unrun`, keep one `unsafe` block each: `RefPtr::from_raw` on
the queued pointer, which adds no ref. `RefPtr::drop` calls `rc_deref`, which
the derive forwards to the same `CellRefCounted::deref`, so every path still
releases exactly one ref. The two `expect` null tests are gone.
The slice was taken inside the `with_mut` closure and rebuilt with `slice::from_raw_parts` to get it out of the closure's `&mut` borrow. The closure now only fills the copy and says whether the allocation worked. The slice is taken afterwards from `input_copy().get()`, a shared borrow that lasts as long as `&self`, which is what the signature already promised. The range check is the same one `get(range)`, moved after the closure. `write()` does not touch `input_copy` between this call and `set_buffers`.
…ast_result` as a `c_int` `do_work` copied `next_in` into a local, passed the local's address to brotli and then advanced the field with `self.next_in.add(local - self.next_in)`, in both modes. It now passes the address of the field, as it does for `next_out`. brotli only advances `*next_in` and does not keep the address. `last_result` was a union of the encoder's `c_int` and the decoder's `BrotliDecoderResult`, read with `unsafe` in three places. It is a `c_int`: the decode arm stores `result as c_int` and compares the enum value it still holds, and `get_error_info` compares with `needs_more_input as c_int`. The slot keeps its size and its initial value 0. The union had no other user.
`AresReply::parse` was an `unsafe fn` whose six impls each forwarded their three arguments to the matching `ares_parse_*_reply` extern from inside an `unsafe` block. The trait now has `const PARSE: unsafe extern "C" fn(..)`, the shape `HostentWithTtlsHandler::PARSE` has, and each impl names its extern. The six externs have exactly that signature. `ares_reply_callback`, the only caller, keeps its `unsafe` block and calls `(R::PARSE)(..)`, which is a direct call to the C function once the callback is monomorphised.
`get_sockaddr` took `&mut sockaddr`, 16 bytes, and wrote a 28-byte `sockaddr_in6` through it. Its one caller, `Resolver::global_lookup_service`, cast its `sockaddr_storage` to that type in an `unsafe` block, twice. Both functions now take the `sockaddr_storage` the caller has. `get_name_info` takes it shared: `ares_getnameinfo` has `const struct sockaddr *` and copies the address. The first cast block in dns.rs goes, the second shrinks to `&mut *request`. Inside `get_sockaddr`, the two address stores went through `(&raw mut field).cast().write(..)`. They assign `sin_addr.s_addr` and `sin6_addr.s6_addr`, which libc and `bun_windows_sys::ws2_32` both name so: the same 4-byte and 16-byte stores. `get_host_by_addr` reported an unparsable address by calling its own C thunk with `ARES_ENOTIMP`. The thunk only turns the pointer back into `&mut T` and the status into `Some(Error::ENOTIMP)`, so it calls `ctx.on_hostent` with that. Only `Error::ENOTIMP` names `ARES_ENOTIMP` now, so it is a plain `const` like its neighbours that only the enum names. The comment on the `winsock` import in cares_sys is cut to the one line that still holds.
…arameter
`x509` was a `*mut X509` turned into a reference with `unsafe { as_mut() }`,
and `out_peer` a `*mut String` that `set_peer` tested for null and wrote with
`unsafe { out_peer.write(..) }`.
They are `Option<&mut X509>` and `Option<&mut MaybeUninit<String>>`: the same
two null tests, and `MaybeUninit::write` stores without dropping, as
`ptr::write` did. The function stays `unsafe` for `host_ptr`.
Its one caller is `X509View::checkHost` in ncrypto.cpp. It returns early when
`cert_` is null, and passes either null or the address of a local `BunString`
that it initialises to the empty string and does not touch during the call.
`X509` is an opaque zero-sized `!Unpin` type, so `&mut X509` claims nothing
about the certificate's memory.
…ee without fn-pointer transmutes `X509_get_subject_name` (crypto/x509/x509_cmp.cc:69) returns the address of a member and reads nothing. It is a `safe fn` over `&X509`. `common_names` is its only Rust caller. `GeneralNames::drop` transmuted `GENERAL_NAME_free` to `fn(*mut c_void)` and passed it to `sk_pop_free_ex` as `free_func`, and its trampoline unwrapped that argument and transmuted it back before calling it. `OPENSSL_sk_pop_free_ex` (crypto/stack/stack.cc:112) only passes `free_func` on to the trampoline, so `drop` passes `None` and the trampoline calls `GENERAL_NAME_free` by name. That removes two transmutes and an `expect`, and the indirect call per element becomes a direct one.
…DigestInit` is a `safe fn` The 16 `EVP_*()` getters returned `*const EVP_MD`, so `EVP::init` was a safe fn with a `# Safety` section about its `md` pointer. Each getter returns the address of a static object (crypto/fipsmodule/digest/digests.cc.inc through `DEFINE_DATA` in delocate.h:62, and crypto/digest/digest_extra.cc:276, 301, 326, 359, 393 and 416), so they return `&'static EVP_MD`. `EVP_get_digestbyname` (digest_extra.cc:218) and `EVP_get_digestbynid` (:79) walk one table (:43) and return what one of these getters or `EVP_md5_sha1` returns, or null. `EVP_md5_sha1` (:455) returns the address of a constant at file scope too. Both return `Option<&'static EVP_MD>`. `Algorithm::md()` and the `EVP::md` field follow. `Option<&EVP_MD>` is one word where `Option<*const EVP_MD>` was two. A result that goes on to a `*const EVP_MD` parameter coerces, so those calls are unchanged. Two callers tested a result for null. `EVP::by_name` matches on the `Option`. In `X509::tls_server_end_point_hash` the test stood after a `match` with an `EVP_sha256()` arm and an `EVP_get_digestbynid()` arm. The second arm ends in `?` and the test goes, so that arm has one null test as before and the first has none. `EVP_DigestInit` (crypto/fipsmodule/digest/digest.cc.inc:162) starts with `EVP_MD_CTX_init`, which overwrites the three pointers of the context before anything reads them, then calls `EVP_DigestInit_ex`, which keeps `type` in `ctx->digest` (:157). It becomes `safe fn(&mut EVP_MD_CTX, &'static EVP_MD)`. `EVP::init` calls it instead of `EVP_MD_CTX_init` followed by `EVP_DigestInit_ex`: the same work in one call. The `new_evp!` hashers in sha.rs called `EVP_MD_CTX_init` and then `EVP_DigestInit`. They lose the first call and the block around the second: one FFI call fewer per `init`, for the 8 types. `EVP_MD_CTX_init` has no Rust caller left and its declaration goes. bun-sema links no BoringSSL and has weak stand-ins in src/sema/standalone/native_libraries.rs: its `EVP_sha256` (:167) returns the address of a static, and its `EVP_DigestInit` (:199) aborts on any other `type_` and overwrites the whole context, so both keep what the signatures promise. Its `EVP_MD_CTX_init` (:187) stays, because a test in that file calls it. `EVP::size` asked `EVP_MD_CTX_size(&self.ctx)`, which is `EVP_MD_size(ctx->digest)` after a null test. `ctx.digest` is `self.md` from `init` on, so it asks the new `safe fn EVP_MD_size(&EVP_MD)` (digest.cc.inc:36). `ctx` was a `pub` field that nothing outside EVP.rs used. It is private, so only EVP.rs can break that equality. `EVP_MD_CTX_size` has no caller left and its declaration goes. `EVP_MD_do_all_sorted` has no caller either, and its declaration goes too. The comment on `md` repeated its type and goes. `EVP::reset` is unchanged.
…t>` it releases
The hook was `unsafe extern "C" fn(*mut c_void)` and ended in
`unsafe { BlockList::deref(ptr.cast()) }`, the only use of that inherent
`unsafe fn deref`.
It now takes `bun_ptr::RefPtr<BlockList>` by value, which is
`repr(transparent)` over `NonNull`, so C++ passes the same pointer. It removes
the table entry under the lock as before and then drops the `RefPtr`, which
calls the same `ThreadSafeRefCount::deref`: one ref down per call, matching the
one ref up in `BlockList__onStructuredCloneSerialize`. `BlockList::deref` goes.
SerializedScriptValue.cpp calls it in two places, the destructor (line 4300)
and `releaseSerializedRefs` in `create` (line 4957). Both walk a vector whose
only `append` (line 1440) stores the `blockList->wrapped()` that the line
before passed to the serialize hook as `&BlockList`, and each vector is
walked once.
`QuicEndpoint::on_new_conn` wraps lsquic's pointer in a `Conn` at its top. Two
later exits wrapped the same pointer again with `unsafe { Conn::from_raw }`
and a null test to call `abort_silent`, and two more called
`lsquic_conn_abort_error` on the raw pointer. All four use the first `Conn`.
`Conn::abort_error` takes a `&CStr`, so the two refusals pass `c""` where they
passed null. `lsquic_conn_abort_error` (lsquic_conn.c:208) replaces a null
reason with `""`, so the frame is the same. The argument is now the address of
a constant instead of zero.
No other crate names `lsquic_conn_abort_error`, so the declaration is private.
…rt` stays typed
`Bun__nodeQuic__drainEndpoint` and `Bun__nodeQuic__processEndpoint` were
`unsafe extern "C" fn(*mut c_void)` that cast the pointer through
`unsafe fn from_driver_owner` and called `run_driver_pass(&self)`. They take
`&QuicEndpoint`, and `from_driver_owner` goes.
Each has one call site, node_quic_shim.c:601 and :615, both under
`if (d->owner)`. `owner` is written only by `us_nq_loop_register` (:571), whose
one caller, `link_loop_driver`, passes `from_ref(self)`. `unlink_loop_driver`
takes the node off the list before the endpoint can be freed. A walk that is
under way has read its successor already, so it can still call an endpoint that
the pass before it closed and unlinked; `run_driver_pass` returns at once for
it, as before. The reference lives exactly as long as the `&self` of
`run_driver_pass` did.
`lookup_cert` already held `this: &QuicEndpoint` and still ended in
`unsafe { get_ssl_ctx(owner, local) }`, which turned the `void*` back into the
same reference behind a second null test. The body of `get_ssl_ctx` moves to
`fn server_ssl_ctx(&self)`, and both callbacks call it.
`lookup_cert` was the one use of the form `ctx: *mut c_void as r: &T` of
`lsquic_callback!`. The two arms for that form were otherwise only what the two
short arms forwarded to, so the short arms hold the expansion themselves and the
forwarding goes. No other use of the macro changes.
…eates the cell
The constructor built the struct with defaults, moved it into its JS cell, and
then stored each option it read through the raw payload pointer, in six
`unsafe { (*raw).field.set(..) }` blocks.
It now reads the options into locals first and puts them in the struct
literal, so seven fields are stored once instead of twice. The two blocks that
need the cell's address or the wrapper (the vtable, and `state`/`stats`) stay.
What changes in order: the six property reads (`address`, `blockList`,
`blockListPolicy`, `disableStatelessReset`, `statelessResetBurst`,
`statelessResetRate`) keep their order and their conversions, but run before
the cell and the boxed vtable are allocated instead of after. If one of them
throws, the cell and the vtable are never allocated, and the default bind host
and the `Strong` on the block list are freed on return instead of when the GC
finalizes the abandoned cell. Nothing could reach that cell, and its `Drop`
has nothing to release at that point. The one caller, `new Endpoint_(options)`
in quic.ts, passes a fresh null-prototype object of data properties.
…reference they make
Both constructors form `&*payload` to finish setting the object up and then
returned the raw pointer, so each caller went back through
`unsafe { (*session).x() }` with "was just created" as the reason.
They return `&'a Self`, and `'a` is the borrow of the owner.
`QuicSession::create` takes `endpoint: &'a QuicEndpoint`, `QuicStream::create`
takes `session: &'a QuicSession`, and `create_stream` borrows from `&self` and
passes the reference on. `global` has a lifetime of its own. Both constructors
took the owner as `*mut`, and all five call sites passed
`from_ref(self).cast_mut()`. They pass `self`, and the cast stands at the one
place in each constructor that stores the back-pointer. In every caller the
owner is the caller's own `&self`, so no body can choose a longer `'a`. That is
how `live_session` and `endpoint_ref` bound what they return.
`apply_server_session_options` takes `&QuicSession`. Seven blocks go: in
`on_new_conn`, `apply_server_session_options`, `connect` and
`connect_verneg_probe`, two in `on_remote_stream` and one in `open_stream`. The
large block in `connect` shrinks to the `lsquic_conn_set_ctx` call.
The payload belongs to the GC, and the compiler does not check that it lives for
`'a`. `create` roots the wrapper with `set_strong` before it returns, and only
`teardown` (for a stream, also `release_close_root`) undoes that. After that the
frame that has the reference holds the cell, which is what
`JsWrapper::payload_ref` asks of its caller. Of the callers only `connect` tears
the session down, and it returns right after. Nothing forms a `&mut` to either
payload before the sweeper drops it.
Where a pointer is stored (`sessions`, `pending_new_sessions`, `provisional`,
`pending_verneg`, `streams`, the conn and stream ctx, the two back-pointers) it
is `ptr::from_ref(x).cast_mut()`, the form these files use for `self`. Every use
of those pointers forms `&T`. The operations and their order are unchanged.
…Stream>`
`QuicSession::conn` and `QuicStream::raw` were `Cell<*mut _>`. Nine places
wrapped the field again with `unsafe { from_raw(..) }` before using it, among
them the safe accessors `conn()` and `ls()`. Ten safe functions took a raw
lsquic pointer that they or a later function trusted: `bind_conn`,
`cache_sockaddrs`, `conn_peer_addr`, `on_remote_stream`, `bind_raw`,
`QuicEndpoint::on_new_conn`, `defer_stream_abort`, `QuicSession::create`,
`create_stream` and `QuicStream::create`. `bind_conn`,
`on_remote_stream` and `bind_raw` wrapped their parameter, and `connect` wrapped
at its end the pointer it had tested for null further up.
The fields are `Cell<Option<Conn>>` and `Cell<Option<Stream>>`, and those ten
functions take the handle or an `Option` of it. 13 blocks go and none is added.
In `connect`, `from_raw` joins the block that calls `lsquic_engine_connect`. The
block in `QuicEndpoint::on_new_conn` moves into the `on_new_conn` callback with
its SAFETY comment. What is left to prove sits where lsquic hands a pointer over
(the `on_new_conn` callback, `connect`, `on_new_stream`, `on_stream_read`). The
fields are cleared where they were set to null. `DeferredAbort::ls` stays a raw
pointer with its own `from_raw`, because what keeps it alive is a different
argument. `defer_stream_abort` stores `ls.raw()` in it.
In `connect` the local `Conn` is used after `apply_options`, as the raw pointer
was. The SAFETY comment at `from_raw` says why no script runs in between when
quic.ts calls.
`Conn` and `Stream` hold a `NonNull` and are `repr(transparent)`, so the
`Option` is one pointer with null as `None`. Each `is_null()` becomes
`is_none()`, `is_some()` or `if let Some`, one for one. Six null tests go:
- `connect` tested the pointer from `lsquic_engine_connect`, and again in
`from_raw` at its end.
- `bind_conn` matched on `from_raw` of its parameter.
- `on_remote_stream` did the same.
- `refresh_conn_stats` tested the field and then `from_raw` of it.
- `bind_raw` had `from_raw(..).expect(..)`. Its panic path goes too.
- The `on_new_conn` callback tested its pointer, and
`QuicEndpoint::on_new_conn` tested it again in `from_raw`.
`report_close` matches on the field instead of testing it in a guard and reading
it again. 39 lines in lsquic_sys pass `self.0.as_ptr()`. `Conn` gains `raw()`,
which `Stream` had. The other operations and their order are unchanged.
…dentifier `to_expr` took a `*mut c_void` context and a `ToExprWrapper`: a back reference to the arena and a fn pointer to a trampoline that cast the context back to `&mut P` or `&mut ExportHoist`. All four call sites hold that `&mut` and the arena, so they pass both, and name the method (`Self::wrap_identifier_namespace`, `Self::wrap_identifier_hoisting`, `ExportHoist::wrap_identifier`) instead of a trampoline around it. `to_expr` is generic over the context and the function. The function is a zero-sized fn item, so nothing is passed for it and the call to it is direct. The cost is code size: `to_expr` is compiled once per method (four copies in bun_js_parser, one in bun_bundler) and not once in bun_ast. In a `--release` build bun_js_parser grows by 526 instructions in 343,000 and bun_bundler by 78, and bun_ast shrinks by 553. `ToExprWrapper` goes away, and with it the `expect` on its arena for every array and object pattern, the two fields of `P` that held it (32 bytes) and the code in `prepare_for_visit_pass` that filled them. One behaviour differs: `to_expr` on a `P` before `prepare_for_visit_pass` used to panic and now works.
…ds it
`P::init(out: &mut MaybeUninit<P>, ..)` returned `Result<(), Error>`. `init_p!`
called it, moved the slot into a drop guard and gave the guard to the caller.
Each of the four users of the macro then took its `&mut P` out with
`unsafe { __p.assume_init_mut() }`, and so did the fifth caller of `P::init`, the
copy of the macro that runtime/test_runner/snapshot.rs writes out by hand because
the macro is local to its crate.
`init` cannot fail: its body has no `?`, `return`, `Err`, `unwrap`, `expect` or
`panic!`. And `MaybeUninit::write` returns the reference the callers want. So the
signature is
pub fn init<'o>(out: &'o mut MaybeUninit<Self>, ..) -> &'o mut Self
The guard holds that reference and the macro binds the caller's `p` to it. The
slot and the guard are locals of the expansion, which the caller cannot name. The
guard's closure runs `drop_in_place` on the reference, which is what
`assume_init_drop` does. snapshot.rs does the same by hand: its guard type
`InitializedParser` holds the reference, and its `Drop` runs `drop_in_place`.
Five `unsafe` blocks go, and with the `?` the macro's hidden early return.
Compiled code, in a `--release` build of bun_js_parser (default flags, no LTO),
the parent of the commit before this one against this commit, so with the 32
bytes that that commit takes off `P`:
- Moving the slot into the guard was a `memcpy` of the whole `P` (4344 bytes) in
every caller. `_parse::<false>` and both `_scan_imports` copied it a second time
to drop it. All eight copies are gone.
- Stack frames: `_parse::<false>` 13240 to 11016 bytes, `_parse::<true>` 12856 to
10648, `_scan_imports` 9480 to 5176, `to_lazy_export_ast` 10008 to 5816.
- Instructions: `_parse::<false>` 5133 to 5075, `_parse::<true>` 5553 to 5477,
`_scan_imports::<false>` 895 to 830, `to_lazy_export_ast` 582 to 569, and 4
fewer in each of the four instances of `init`.
- `init` returns its first argument, and LLVM sees that. In the five callers
that were read (both `_parse`, both `_scan_imports`, `to_lazy_export_ast`) the
code after the call ignores the returned register and addresses `p`'s fields
off the stack pointer, as before, without a spilled pointer to `p`.
- `P` is dropped as before: inline in `_scan_imports`, by a call to its drop glue
in `_parse::<false>` and `to_lazy_export_ast`, and by a call to the guard's drop
glue in `_parse::<true>`.
No cost was found in the assembly. Run time was not measured.
`WriterTrait::write_reserved` got a raw pointer to `n` spare bytes from `reserve`, copied the bytes there and committed them with `advance`. On `BufferWriter`, the only `WriterContext`, that is `Vec::reserve`, a copy and `set_len`, which is what `print_slice` does through `Vec::extend_from_slice`. The three callers use `print_slice`, two of them through `self.print` like the code around them, and `write_reserved`, `advance` and `advance_by` go away. `reserve` has two more callers, `print_ast` and `print_with_writer_and_platform`, which pre-size the buffer and ignore the pointer, so `reserve` and `reserve_next` stay and return nothing. `print_slice` is `#[inline]` down to the `Vec`. `reserve_next` and `advance_by` are not, and the printer is instantiated in other crates, so without LTO each `write_reserved` made two calls into bun_js_printer.
…e `safe fn` Four imported functions took raw pointers to locals of the caller, and every call was an `unsafe` block. Typed as references, the signature says that the pointers are valid: - `JSC__JSValue__borrowBytesForOffThread` (bindings.cpp) writes `*out_ptr` and `*out_len`, or neither, and reads neither. - `tj3GetScalingFactors` (turbojpeg.c) stores a constant in `*n` and returns its static table. - `Bun__AOT__initializeOptions` (BunAOTCompile.cpp) calls `WTF::initialize()`, which is a `call_once`, and assigns `makeDefaultCompileOptions()`: zeroes, three integers, a double and two enumerators of enums generated from the schema that also generates the Rust type. One of the integers comes from `WTF::numberOfProcessorCores()`, which caches its answer in a plain `static int`; two first calls at once both store the same value, and safe Rust could already make them through `EngineOptions::new`. - `mi_process_info` (mimalloc, src/util/stats.c) tests each of its eight pointers for null before it writes through it, so they are `Option<&mut usize>`. `&mut T` and `Option<&mut T>` are passed like the pointers they replace. Image.rs also had its own declaration of `JSC__JSValue__unpinArrayBuffer`. bun_jsc declares that function `safe` and wraps it as `JSValue::unpin_array_buffer`, which other files of bun_runtime call. The two calls in Image.rs use the wrapper. That is the removal of a duplicate declaration, not a proof: the compiler checks nothing new at those two sites, and the C++ function still wants a value that was pinned. In Image.rs that holds by construction, because `Pin`'s field is private and a `Pin` is built only for a result of 2.
…ump` `call` turned the map entry and the arena into `*const Macro` and `*const Arena` "so the closure does not extend `&mut self`", and the closure made references of them again with two `unsafe` blocks. Both are borrows of one field each. What else `call` touches while they live is other fields (`self.javascript_object`, and before that `self.env` and `self.transform_options`), so the borrow checker accepts `&Macro` and `&Arena` as they are. `call` holds `&mut self` across the macro's JS as before, and `Runner::run` got the same two shared references. `disabled` and `vm` were copied out of the entry "so the borrow can be released". They are read through `macro_`, `vm` after the `disabled` test and not before it.
`JsonTape` is five `Vec<_, TapeAlloc>` and a fieldless enum. `TapeAlloc` and what the element types hold (`StoreStr`, `StoreRef<ObjectJSON>`, `StoreRef<ArrayJSON>`) have `Send` and `Sync` impls of their own, none behind a `cfg`, so `JsonTape` is `Send + Sync` as an auto trait and its two impls said nothing more. sql_jsc's handle to the runtime's `SSLConfig` was declared `Send`. No code asks for that. The type is not named outside the crate, and none of its four holders looks at the bound: - `MySQLConnection` sits in `JSMySQLConnection` as a `JsCell<MySQLConnection>`, and `JsCell<T>` is `Send + Sync` for every `T`. - `PostgresSQLConnection` and `ConnectParams` hold an `Rc<SSLConfig>`, which is never `Send`. - `ConnectionCtorArgs` is `pub(crate)`, and nothing in the crate puts a bound on it. `Send` occurs nowhere else in the code of sql_jsc or sql, and the crate has no platform `cfg`. Without the impl the handle is `!Send`, like the `NonNull` in it. The entry for src/sql_jsc/jsc.rs leaves vm-thread-door.inventory.json.
`IdentOrRef::as_ident` built a `&[u8]` with `from_raw_parts` only to turn it into the `*const [u8]` that `Ident` stores. `slice_from_raw_parts` builds that pointer and no reference. `arena_slice_newtype!` gave its three types an `unsafe fn as_slice` that calls the safe `v()`. It has no caller.
…onst` array
- `JSValkeyClient::on_valkey_connect` put `self.as_ctx_ptr()` in its drop guard
and dereferenced it there. Both calls in the guard take `&self`, and `&Self` is
`Copy`, so the guard holds `self`. The eight other drop guards in js_valkey.rs
held `BackRef::new(self)` (or `this`) for the same kind of call. They hold the
plain reference too, with no other change.
- `BlobReadChain::start` made a `&mut Blob` of what `as_::<Blob>()` returned and
only called `read_bytes_to_handler(&self, ..)` on it, which can run the handler,
and through it JS, before it returns. `as_class_ref` is `as_` plus the shared
dereference, so the same tests run.
- `CachedStructure::build_from_columns` made its array of `MaybeUninit` with
`MaybeUninit::uninit().assume_init()`. `[const { .. }; 70]` is the safe
spelling. In an optimised build neither form writes anything; at opt-level 0
(the debug build) the old form copied the whole array with a `memcpy` and the
new one fills it with a `memset`, 70 × 32 = 2240 bytes either way.
- `Symbol::merge_contents_with` only reads `old`, so it takes `&Symbol`. No
`unsafe` goes away with this one: its caller makes both references from raw
pointers.
`OutputSlice::put_words` (`&[u32]`), the test helper `bytes_of` (`&[u64]`) and the markdown parser's copy of `current_block_lines` into `block_bytes` (`&[VerbatimLine]`) each built the `&[u8]` with `from_raw_parts`. `slice_as_bytes` is that same expression, `#[inline(always)]`, behind the bound `T: NoUninit`. `bytes_of` would only be another name for it, so it goes and its four call sites, all in tests, call `slice_as_bytes`. `VerbatimLine` (`#[repr(C)]`, three `u32`) gets the bound from `impl_no_uninit!`, which compiles only if rustc can prove that the type has no padding.
`Rope::next` was a `*mut Rope`. `append` walked the chain through `StoreRef`, and `next_ref` turned the pointer into a `&'a Rope` of any lifetime with an `unsafe` block for `get_or_put_object`. Every segment is allocated in the arena that the head is allocated in (`ropealloc` in the INI parser; `bun init` has a single segment), so the link is a `&'a mut` into that arena. `append` walks a `&mut Option<..>` cursor, and `get_or_put_object` reads `rope.next.as_deref()`. `append` returned `Result<*mut Rope, AllocError>`, always `Ok`, and its three call sites dropped the pointer. It returns nothing. The null niche keeps the layout. In a `--release` build of bun_ast, `get_or_put_object` compiles to the same instructions and `append` loses the two that built its return value.
`ResolveHandler::raw_callback` was an `unsafe extern "C" fn` in the trait. Its one impl, for `ResolveInfoRequest<T>`, forwarded its five arguments to `T::RAW_CALLBACK` from inside an `unsafe` block. The trait now has `const RAW_CALLBACK: unsafe extern "C" fn(..)`, the shape `CAresRecordType::RAW_CALLBACK` and `AresReply::PARSE` have, and the impl names `T::RAW_CALLBACK`. `Channel::resolve`, the only user, keeps its two `unsafe` blocks: it calls the constant for a bad name and hands it to `ares_query` otherwise. c-ares then calls the record type's thunk itself, and not a thunk that calls it.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it is a 35-commit, 58-file rewrite of pointer/ownership shapes across IPC, N-API, bun:ffi, BoringSSL/X509, c-ares and node:quic, a human look at the lifetime arguments would still be worthwhile.
What was reviewed:
- IPC:
CHANNELnow set only after the socket/pipe is up; the error path'sdata.detach()plusRefPtrdrop matches the olddeinitrelease sequence, and the pre-CHANNELwindow was checked for readers. - node:quic:
Option<Conn>/Option<Stream>and&'areturns fromcreatewere checked against option-application paths that can run JS (session.rs, endpoint.rs) and against theQuicStreamDrop SAFETY text. - N-API:
Option<&NapiEnv>ABI and thesafe fnimports (Bun__napi_check_gc,NapiHandleScope__escapenow takingJSValue) were checked for nullable-pointer layout and unchanged null tests. - zlib
borrow_input_copyprovenance throughJsCell::get, and the BoringSSLGeneralNamesdrop/free_func path, were examined and not found to change behavior.
Extended reasoning...
The change is a +870/-1499 unsafe-reduction refactor across 56 Rust files plus a test comment and the vm-thread-door lint inventory, replacing raw pointers and type-erased trampolines with references, Option, RefPtr and safe fn imports. It touches security-sensitive surfaces: BoringSSL/X509 host checking and digest getters, N-API add-on entry-point ABI, bun:ffi buffer views, and the node:quic TLS/lsquic bindings. The automated hunt ran dry with no findings and eight candidate issues (IPC CHANNEL window, quic UAF during option application, zlib provenance, BoringSSL leak, SAFETY comment accuracy) were ruled out, but the size, the number of distinct subsystems, and the fact that correctness rests on lifetime reasoning the compiler only partially checks at FFI boundaries mean a human should also confirm the ownership arguments before merge.
|
BuildKite build 137632 of 37adb5b has finished, and it is red. Nothing fails only here.
On GitHub every check passes on 37adb5b: Generated by Claude Code |
|
No actionable comments were generated in the recent review. WalkthroughThis pull request updates Rust APIs and call sites across parser, runtime, networking, and FFI code. It replaces several raw-pointer interfaces with references, option types, and typed handles. It also updates related initialization, callback, and cleanup paths. ChangesSafe APIs and ownership
Suggested reviewers: Merge Risk:
|
One conflict, in the imports of generateCodeForFileInChunkJS.rs. #47944 removed the ToExprWrapper import that stood next to the bun_ast import this branch extends. The merged line keeps the three added names and drops ToExprWrapper.
…rust-17fiip One conflict, in src/runtime/image/codec_jpeg.rs: main made the declaration of tj3GetScalingFactors safe and took the unsafe block off its call (#47944). This branch has neither any more, so the file stays as it is here.
What does this PR do?
35 separate changes, one commit each: 10 in IPC, N-API,
bun:ffiand their neighbours, 15 innode:zlib, the c-ares and BoringSSL bindings andnode:quic, and 10 in the parser, the printer, CSS,Bun.Imageand smaller crates. They follow #47291, #47317, #47338, #47382, #47393, #47414, #47871, #47893, #47895, #47901 and #47940 and work the same way: signatures name the type and the lifetime that the value already has, hand-written pointer code becomes plain ownership and borrows that the compiler checks, and what was never needed is deleted.Together: 125 fewer
unsafeblocks, 17 fewerunsafe fns and 4 fewerunsafe impls, +870/−1499 over 58 files. No safe function becameunsafe, and no wrapper type was added. Where the compiled code changes lists every place where the program does different work.unsafeCHANNELholds&'static IPCInstance, boxed once the transport is upunsafe fnBun__napi_check_gcandNapiHandleScope__escapearesafe fnOption<&T>or&TFinalizer::runandscheduleborrow their env, and three more borrowsunsafe implConfig::optionsisOption<&'a ZStr>unsafe fn&Cell<bool>ssl_config_from_jsandtimer_heaphooks are safefnsunsafe fnspawnfunctions take&VirtualMachineuv_posixcallsunsupported_uv_functionas a Rust functionRefPtr<T>unsafe fnborrow_input_copyborrows the copy throughJsCell::getNativeBrotlipasses&raw mut self.next_inand keepslast_resultas ac_intAresReplynames its parser as an associated constget_sockaddrandget_name_infotakesockaddr_storageBun__X509__checkHosttypes its certificate and its out-parameterX509_get_subject_nameis asafe fn, and a stack free without fn-pointer transmutesunsafe fn&'static EVP_MD, andEVP_DigestInitis asafe fnBlockList__onStructuredCloneDestroytakes theRefPtr<BlockList>it releasesunsafe fnon_new_connuses theConnit already has&QuicEndpoint;lookup_certstays typedunsafe fnQuicEndpoint::constructorreads its options before it creates the cellQuicSession::createandQuicStream::createreturn the reference they makeOption<Conn>and a stream holdsOption<Stream>Binding::to_exprtakes&mut Cand the method that wraps an identifierP::initreturns the&mut Pit wrote, andinit_p!binds itwrite_reservedcallers useprint_slicesafe fnMacroContext::callborrowsself.macrosandself.bumpunsafe impl Send/Syncthat nothing needsunsafe implunsafein values/ident.rsunsafe fn&Selfguards,&Blob,&Symbol, aconstarraybun_core::slice_as_bytesRope<'a>links its segments withOption<&'a mut Rope<'a>>ResolveHandlernames its callback as an associated constThe commits can be reviewed one at a time.
Worth knowing before reading on:
test/, a comment inspawn.ipc.test.tsnamed a function that change 1 deletes, and the inventory of thevm-thread-doorsource lint loses the entries of removedunsafe impls (changes 4 and 30).bun:ffi'stoBufferandtoArrayBuffermade a&mut [u8]over memory that belongs to the caller.spawnfunctions of the webview were safe functions that dereferenced a*mut VirtualMachine.get_sockaddrtook a&mut sockaddr, 16 bytes, and wrote a 28-bytesockaddr_in6through it.node:quictook a raw lsquic pointer that they or a later function trusted.BlobReadChain::startheld a&mut Blobacross a call that can run JS.memcpyof 4,344 bytes per parse, and most callers made a second one to drop it. Both are gone (change 26), andPis 32 bytes smaller (25).Option<&NapiEnv>has the ABI of a nullable pointer, null isNone, and every null test is where it was.node:quicthe lifetimes come from the owner:createreturns a reference that borrows from the endpoint or the session (change 23), and a session holdsOption<Conn>and a streamOption<Stream>, one pointer each, with six repeated null tests fewer (24).safe fns:Bun__napi_check_gcandNapiHandleScope__escape(change 2),X509_get_subject_name(17),EVP_DigestInitand the digest getters, which return&'static EVP_MD(18), and four that only write their out-parameters (28). Each message says what the C side reads and writes, for every argument, in every state and on every thread. The other shims overNapiEnvstay unsafe imports, because they belong on the env's JS thread.IPCInstance::newanddeinit(change 1),write_reserved,advanceandadvance_by(27), three declarations of BoringSSL functions that nothing calls (18), threeunsafe impl Send/Sync(30) and anunsafe fn as_slice(31).get_ipc_instance), 9 (the macOSHostProcess::spawnand the Windows arms of Chrome'sspawn), 15 (the WinSock names) and 28 (mi_process_infoon Windows).Where the compiled code changes
I compared the release assembly (x86-64 Linux,
--emit=asm) of all 134 crates of the workspace, before and after. For 95 of them the files are the same byte for byte. The other 39 I compared function by function, with names demangled and labels normalised. In 26 of the 39 (30,267 functions) no function differs: what differs is the debug information. 13 crates differ:bun_astbun_js_parserbun_bundlerbun_runtimebun_js_printerbun_install,bun_sourcemap_jscbun_sha_hmacbun_boringssl_sysbun_inibun_js_parser_jscbun_cares_sysbun_crash_handlerThe last five columns are net, and count the functions that exist on one side only: 38 only before and 27 only after, which were removed, renamed or inlined, or are no longer inlined. Where only constants differ, each is an offset into the parser's
Pand is 32 lower, becausePis 32 bytes smaller (change 25). The names that differ:Store::appendhas the same 112 instructions forE::ObjectandE::Array, and the compiler keeps one copy inbun_js_parser. That copy was named afterE::Objectand is named afterE::Array, at 173 call sites.do_resolve_caresload the address of another function (change 35, in the last table), and five functions ofBun.Imagecall a wrapper (change 28, below).What costs something. No change adds an allocation, a lock or a reference count, and none adds a test to the source. These are the places where the compiled code does more than before:
Binding::to_expris compiled once per wrapping method (change 25). One copy inbun_ast(505 instructions), which called the method through a function pointer, becomes four inbun_js_parser(210 to 223) and one inbun_bundler(225), which call it directly or have it inline. That is about 590 instructions of code more. Each copy is less than half as long as the one it replaces, and a call of it loses the indirect calls and anexpectper array or object pattern.maybe_relocate_vars_to_top_levelwas one copy (134) for two instances of the parser and is two (130 each), because each calls its ownto_expr.E::ArrayandE::Spreadwere stored throughinto_data_store(115 and 111) and twonew_expr(77 each). They are stored throughStore::append(112 and 108), which is inline in some parser functions and not in others, in both directions:parse_type_script_decoratorsandpfx_t_open_braceof the bundler's parser are 88 and 86 shorter,pfx_t_open_bracketof one instance 56 shorter,parse_prefixandparse_jsx_elementof another 31 and 30 longer. In the bundler twofrom_bump_vec(101 and 100) are functions of their own.has_globals_of_vitest(35) is a function of its own (change 26). It was inline in both_parse, which call it once each.Bun.Imageunpins its input throughJSValue::unpin_array_buffer(change 28). Six call sites in five functions called the C++ function through a declaration of their own. They call the wrapper thatbun_jschas for it, asnode:zlibandnode:httpdo. The wrapper is one jump, which goes where the link inlines across crates.EVPare laid out in another order (change 18).mdis a reference now, and the compiler puts it first, so the context is at offset 8.EVP::drop(1 to 2),CryptoHasher::updateand its export fornode:crypto(1 more each) add 8 to a pointer that they used as it was, andEVP::final(40 to 43) keeps that address in one more register.byteLength,digestandgetDigestSizeare 1 longer for another reason: they loadmdforEVP_MD_size, whereEVP_MD_CTX_sizeloaded it from the context after a null test.QuicEndpointgoes from 557 to 635 (change 22), with 3 branches, 3 compares and 3 call sites more. It reads its options before it makes the cell, so when a read throws it has to free the bind host and theStrongon the block list itself. Seven fields are stored once instead of twice. It runs once per endpoint.uv_*stand-ins are 1 longer (change 10): they load the length of their name before the call that ends the process.Checks. The source makes no check that it did not make before. These are all the other functions in which the listing shows more conditional branches, compares or panic call sites than before, and why:
FmtAdapter::write_strandwrite_char,print_binding,print_identifier_utf16. 15 copies inbun_bundler,bun_install,bun_runtimeandbun_sourcemap_jsc.reserve_nextandadvance_byinbun_js_printer. The capacity test of the first and the store of the second are inline now, and growing the buffer is the only call left. Thememcpyis the same. Inwrite_strthe compiler also skips it for an empty string.FFI.toBuffer()Everything else that differs:
get_ipc_instancegoes from 197 to 188, with 2 calls fewer.Bun__closeChildIPC,sendandemitHandleIPCMessagekeep their length.CHANNELis set after the socket is set up instead of before. Nothing reads it in between. When the set-up fails, no box is made or freed.napi_create_promisegoes from 76 to 75, with one call fewer: it reads the promise withvalue(), and stores*deferredafter that read instead of before it.CrashHandler__unsupportedUVFunction(46 to 6) isunsupported_uv_function(45), anduv_posix::unsupported(2) forwards to it.run_task::handlegoes from 3,119 to 3,101 with 3 branches, 3 calls and 3 panic sites fewer. The threerelease_unrungo from 69 to 63 with one of each fewer.writego from 517, 519 and 519 to 514, 516 and 516.native_brotli_impl::Context::do_workgoes from 53 to 45.AresReply::parse, one jump each, go.Resolver::reversegoes from 547 to 545, with one call fewer: for an address that does not parse it callson_hostentand not the C thunk around it.call_general_name_freegoes from 10 to 2: it callsGENERAL_NAME_freeby name, and loses a null test with its panic.GeneralNames::dropkeeps its length and passes null where it passed that function.Algorithm::mdgoes from 63 to 25, because its 16 calls are tail calls. Theinitof the 8 hashers goes from 28 to 26, andgetByName,getFromOtherandcopyare 2 shorter: one call ofEVP_MD_CTX_initfewer each, whichEVP_DigestInitrepeats.EVP::by_namegoes from 53 to 48,tls_server_end_point_hashfrom 98 to 96 with one null test fewer.on_new_conngoes from 683 to 684. Two exits callConn::abort_error, which calls lsquic, where they called lsquic. The two exits that callConn::abort_silentshared a call site and have one each.QuicStream::bind_rawgoes from 264 to 258 with a branch, a call and a panic site fewer.createandcreate_streamare 2 shorter,on_new_streamis 1 longer.QuicEndpoint.connect()goes from 1,032 to 1,035: the exit for a failed connect is laid out twice, soteardownhas 2 call sites. Four functions keep their length.createis tested for null instead of by a tag.prepare_for_visit_passthat stored them go.to_expron aPbeforeprepare_for_visit_passused to panic and now works. Nothing does that._parsegoes from 5,133 to 5,075 and from 5,553 to 5,477,_scan_importsfrom 895 to 830 and from 930 to 865,to_lazy_export_astfrom 582 to 569,parse_onlyfrom 1,674 to 1,662, the drop glue of the guard in_parse::<true>from 615 to 556, and each instance ofP::initis 4 shorter. Inbun_runtime,ContextParameter::ofgoes from 2,047 to 1,969 andwrite_inline_snapshotsfrom 2,946 to 2,732. The stack frames of the eight callers ofP::initnamed here are 2,208 to 4,736 bytes smaller.P, 4,344 bytes, fewer per parse.write_inline_snapshotscopiedPin pieces, and copies fewer of them.advance_by(2) goes, andreserve_nextgoes from 19 to 10 because it returns nothing.__bun_macro_context_callgoes from 1,876 to 1,870 and its closure from 337 to 323.Rope::appendgoes from 24 to 22. Inbun_ini,prepare_strgoes from 1,030 to 1,018 with 9 branches fewer,str_to_ropefrom 143 to 136 andcommit_rope_partfrom 66 to 61.raw_callback(33 to 52) had the record type's thunk inline. They go, and the twelve thunks are functions under their own names, with the same instructions apart from change 14.Every function that differs and is not named above differs by 18 instructions or fewer, and has the same or a smaller number of branches, compares and panic call sites. Changes 2, 3, 5, 7, 8, 9, 16, 19, 21, 30, 31, 32 and 33 do not show up in this comparison. What changes 1, 9, 15 and 28 do to code for Windows and macOS is not compiled on Linux.
The comparison covers x86-64 Linux only, one crate at a time. The build that ships also inlines across crates when it links.
The changes in detail
1. ipc:
CHANNELholds&'static IPCInstance, boxed once the transport is upCHANNELheldOption<NonNull<IPCInstance>>andget_ipc_instancereturnedOption<*mut IPCInstance>, sosend,close_child_ipc,global_object_connected_ipcandget_ipc_instanceitself dereferenced a raw pointer. A channel that came up is never freed. The only free wasIPCInstance::deinit, on the two paths where the socket (on Windows the pipe) could not be set up, and it was needed only because the box was made and published before that attempt.get_ipc_instancesets the transport up on theRefPtr<SendQueue>it holds and boxes it after that has succeeded:Box::leakgives the&'static IPCInstancethatCHANNELstores and the function returns. On failure it detaches the queue and theRefPtrdrops.IPCInstance::newanddeinitare gone, and the socket group stays the&mut SocketGroupthatspawn_ipc_groupreturns.What differs when it runs:
CHANNELis set afterfrom_fdandset_timeout(on Windows afterwindows_configure_client) instead of before. Onlyget_ipc_instance,global_object_connected_ipcandglobal_object_has_ipcread it, and none of them runs in between:us_socket_from_fdarms the poll, links the socket and may start the date header timer. The failing path neither allocates the box nor touchesCHANNEL, and it releases the queue after the warning is printed instead of before.The comment on the test of the failing path in spawn.ipc.test.ts named
IPCInstance.deinit. It says what the path does now.2. napi:
Bun__napi_check_gcandNapiHandleScope__escapearesafe fnEach was declared with a raw handle pointer and called in an
unsafeblock from a safe method on&self, which passed itsas_mut_ptr(). The handle is the only pointer they take. They take it by reference, as thesafe fnshims overJSGlobalObjectdo:Bun__napi_check_gcreadsnm_version, which never changes, and asks the heap whether the current thread is doing GC work, which any thread may ask. It can abort the process.NapiHandleScope__escapeanswers false once the escape slot is used or the scope is closed. It took the value asusize; it takesJSValue, which isrepr(transparent)over it. The empty value is fine: the write barrier and the visitor skip a null cell.The other shims over
NapiEnvstay unsafe imports: they belong on the env's JS thread, and a&NapiEnvis also held on pool threads and by the collector.Same calls with the same arguments.
3. napi: entry points take the env and scope handles as
Option<&T>or&TThe 44 N-API functions written in Rust that begin with
get_env!,preamble!orungated!tookenv_: *mut NapiEnv, andget_env!made the reference withunsafe { $env.as_ref() }.napi_escape_handledid the same with its scope,napi_ref_threadsafe_functionandnapi_unref_threadsafe_functionwith the env in a debug assertion, and three functions that only Bun's C++ calls with&*env_oras_ref().napi_open_callback_scopeandnapi_close_callback_scopeignore their env and get the same type.NapiEnvandNapiHandleScopeareopaque_ffi!types: zero-sized, align 1 and!Freeze, so every non-null pointer is a valid reference to one, andOption<&T>has the ABI of a nullable pointer. The parameter types say what the macro did: null isNoneand still answersnapi_invalid_arg.Call sites of the three that addons cannot reach:
Bun__napi_register_cleanup_zig(env: &NapiEnv):this, in the body of theNapiEnvconstructor (napi.h).Bun__napi_run_finalizers_after_teardown_wait(env: &NapiEnv):this, inNapiEnv::cleanup()(napi.h).Bun__napi_enqueue_finalizer(env: Option<&NapiEnv>, ..), five:thisinenqueueRefFinalizeranddoFinalizer(napi.h), an env dereferenced just before indrainOneRefFinalizer(NapiRef.cpp), one thatnode_api_post_finalizerhas tested for null (napi.cpp), andRefPtr::get()inNapiFinalizer::call(napi_finalizer.cpp), for which it keeps its null test.Every null test is where it was.
type napi_envstays for the declarations of the functions written in C++ and for the callbacks into addons.4. napi:
Finalizer::runandscheduleborrow their env, and three more borrowsFinalizer::run(&mut self)only reads its fields, and it made an unbounded&NapiEnvfromself.env.get(). It takes&selfand borrows the env from theNapiEnvRefit holds, so the reference lives as long as that ref. Its three callers own theFinalizer(two locals, one box just taken back) and dropmut.NapiFinalizerTask::schedulemade the same unbounded reference twice. Both go through theNapiEnvRefas well. Each borrow ends with its statement, beforeselfis given away:vm_handle()returns aBorrowedRef, which has no lifetime, andbun_vm()returns&'static VirtualMachine. The one other use ofglobal_thiswasglobal_this.bun_vm(), which isvm.as_mut()reads the thread-local itself and uses its receiver in a debug assertion only, so a release build runs the same code.napi_async_work::run_from_jsdereferencedenva second time forsurface_exception.env_ref, made from the same pointer, is still in use after that line: the handle scope guard holds it to the end of the function.napi_create_promiseread the promise back through the raw pointer it had just made from its box. It reads the value first, withJSPromiseStrong::value(), and gives the box away after, so*deferredis stored after that read instead of before it.get().as_value(env.to_js())reached the same value the long way: it downcast the value in the strong slot to*mut JSPromiseand turned that pointer back into aJSValue. Two FFI calls go (the downcast,JSC__JSValue__asPromise, andNapiEnv__globalObjectfor an argument thatas_valueignores), and with them theis_celltest and the null test inas_promise, the unwrap of its result and the null test inopaque_mut.napi_node_version::releasewas a*const c_charto a literal, which needed anunsafe impl Syncfor the static. It is a&'static [u8; 5],b"node\0": a thin pointer at the same offset, and the struct isSyncby itself. The inventory of thevm-thread-doorlint drops the entry.5. tcc:
Config::optionsisOption<&'a ZStr>Config::optionswas anOption<NonNull<ZStr>>, "kept as a raw ptr rather than a borrow", andState::initturned it back into a reference in twounsafeblocks on the caller's word that the string outlives the call. Both callers start from a&ZStrand build theConfigin the argument ofinit, andtcc_set_optionscopies what it parses, so a borrow for the call says the same.Confighas a lifetime and holds the&ZStr. InCompileC::compilethe string can borrowself.flags, so the*mut CompileCfor the error handler is taken before that borrow starts instead of in theConfigliteral. The handler only pushes todeferred_errors.Same pointers and the same calls.
6. ffi: no reference is made from an address only to turn it back into one
make_napi_env_if_neededcast the*mut c_voidfrommake_napi_env_for_ffito&'static NapiEnv. Its one caller hands the result toFunction::compile, which turned it back into an address foradd_symbol, where it is a symbol's value and is not dereferenced.'staticwas true (makeNapiEnvForFFIreturns&out.leakRef(), so the allocation is never freed), but nothing used the reference as a reference. The function returnsNonNull::new(..)of the pointer it got, anOption<NonNull<c_void>>with the same niche, andcompilepassesas_ptr().CompileC::compilealready passes that pointer as it comes.The comment at
add_symbolcalled the env VM-owned; it names the leaked ref. The doc ofmake_napi_env_for_fficalled it "owned by this global" and ended "callers inbun_runtimecast to*mut NapiEnv", which neither caller does. It says that C++ leaks a ref, and the clause is gone.to_bufferbuilt a&mut [u8]over the caller's foreign memory forcreate_buffer_with_ctx, a private function with this one caller that split the slice into pointer and length again for C++. Its call ofJSBuffer__bufferFromPointerAndLengthAndDeinitis written intoto_buffer, with the pointer and length thatget_ptr_slicereturned. Debug builds lose the precondition check insidefrom_raw_parts_mut; release builds make the same call.The helper declared that symbol itself, so the call was made from
bun_runtimebefore too; the declaration stands at the head ofto_buffer.JSValue::create_buffer_with_ctxinbun_jscmakes the same call but is neither generic nor#[inline], so it would stand betweento_bufferand C++ wherever the linker does not inline it.to_array_bufferbuilt the same kind of slice only forArrayBuffer::from_bytes, which stores the slice's pointer, its length inlenandbyte_len, and the type, and takes the other fields fromDefault. A struct literal sets the same four fields from the pointer and length ofget_ptr_sliceand takes the rest fromDefaulttoo. Theunsafeblock stays forto_js_with_context, anunsafe fn. Debug builds lose the same check here.noop_bytes_deallocatorhas an empty body and is not anunsafe fn. It still coerces to theunsafe extern "C" fnpointer that the deallocator slot holds.7. ipc: the JSON decoder's free callback takes
&Cell<bool>json::decode_ipc_messagewraps an ASCII line in an external string and gave the address of a localboolas the context of the string's free callback, which set the flag through the*mut boolin anunsafeblock.create_externalis generic over the context and documents it as a raw pointer or&T. The flag is aCell<bool>, the context is&Cell<bool>and the callback callsset(true). The callback has two callers:create_externalitself when the line is too long, andExternalStringImpl::freeExternalBufferwhen the string is destroyed, with them_freeCtxit stored at creation (releaseBufferEarlyalso calls it; nothing calls that).The
unsafeoncreate_externalstays, and its comment names the flag too: the string carries the lifetime of neither borrow.The same byte is stored and read back.
8. sql: the
ssl_config_from_jsandtimer_heaphooks are safefnsSqlRuntimeHooks::ssl_config_from_jswas anunsafe fn(&JSGlobalObject, JSValue)and its one call sat in anunsafeblock whose comment named no obligation ("may run JS getters / throw"). The one implementation callsSSLConfig::from_jsand boxes the result, all safe. The field is afn, the implementation and the call are safe.timer_heapwas anunsafe fn(*mut VirtualMachine) -> *mut c_voidand states no obligation either: the one implementation ignores its argument and calls the safetimer_all(). The field and the implementation are afn. The block at its one call stays, for the dereference of the result.An
unsafe fndoes not coerce to afnfield, so an implementation of either hook has to be safe. The same indirect calls.9. webview: both
spawnfunctions take&VirtualMachineChromeProcess::spawnand the macOSHostProcess::spawnare safe functions that tookvm: *mut VirtualMachineand dereferenced it twice. Each has one caller, which holds a&JSGlobalObject:Bun__Chrome__ensurepassedbun_vm_ptr(), andBun__WebViewHost__ensurecast the&'static VirtualMachinefrombun_vm()to*mut.Both callers pass
global.bun_vm().VirtualMachine::event_loopandEventLoopHandle::initare safe functions, so that block goes in each. The block that dereferences the rawtranspiler.envstays. Its comment in ChromeProcess.rs calledmapa&mut Mapslot; it is a field of the loader, and the sentence goes.In a release build
bun_vm()andbun_vm_ptr()are the same read of the thread-local, so the same code runs.10. crash_handler:
uv_posixcallsunsupported_uv_functionas a Rust functionuv_posix::unsupportedreached the crash handler, which is Rust, through its C symbol. It declaredCrashHandler__unsupportedUVFunctionitself, returning()where the definition returns!, called it in anunsafeblock with the pointer of its&'static CStr, and putunreachable!after the call.The body of the symbol moves into
pub fn unsupported_uv_function(name_bytes: &'static [u8]) -> !. The symbol, which__bun_throw_not_implementedin uv-posix-polyfills.c calls, keeps itscstrand calls that function.uv_posixcalls it too and has no declaration of its own. Only the C symbol needed the NUL terminator, sounsupported,handle_typeandref_statetake the name as&'static [u8], and the six literals areb"uv_close"and so on.The literals are one byte shorter. The rest differs only on a path that ends the process: from
uv_posixthe length of the name comes with the slice instead of from astrlen, which is one more instruction before the call in each of the six. From C the counter is bumped after thestrleninstead of before it, and the reported stack starts one frame deeper unless the function is inlined into the symbol.11. node:zlib: the write's completion owns its ref as a
RefPtr<T>run_from_js_threadandrelease_unrunwereunsafe fn(*mut T). Each turned the pointer into aParentRefbehind anexpectand released the ref thatwrite()took withunsafe { T::deref(this_ptr) }, once on each of their four exits.Both now take
RefPtr<T>, read the stream through it anddropit where thederefwas, afterpoll_ref.unref. Their two callers, the dispatch arm andTaskable::release_unrun, keep oneunsafeblock each:RefPtr::from_rawon the queued pointer, which adds no ref.RefPtr::dropcallsrc_deref, which the derive forwards to the sameCellRefCounted::deref, so every path still releases exactly one ref. The twoexpectnull tests are gone.12. node:zlib:
borrow_input_copyborrows the copy throughJsCell::getThe slice was taken inside the
with_mutclosure and rebuilt withslice::from_raw_partsto get it out of the closure's&mutborrow.The closure now only fills the copy and says whether the allocation worked. The slice is taken afterwards from
input_copy().get(), a shared borrow that lasts as long as&self, which is what the signature already promised. The range check is the same oneget(range), moved after the closure.write()does not touchinput_copybetween this call andset_buffers.13. node:zlib:
NativeBrotlipasses&raw mut self.next_inand keepslast_resultas ac_intdo_workcopiednext_ininto a local, passed the local's address to brotli and then advanced the field withself.next_in.add(local - self.next_in), in both modes. It now passes the address of the field, as it does fornext_out. brotli only advances*next_inand does not keep the address.last_resultwas a union of the encoder'sc_intand the decoder'sBrotliDecoderResult, read withunsafein three places. It is ac_int: the decode arm storesresult as c_intand compares the enum value it still holds, andget_error_infocompares withneeds_more_input as c_int. The slot keeps its size and its initial value 0. The union had no other user.14. c-ares:
AresReplynames its parser as an associated constAresReply::parsewas anunsafe fnwhose six impls each forwarded their three arguments to the matchingares_parse_*_replyextern from inside anunsafeblock.The trait now has
const PARSE: unsafe extern "C" fn(..), the shapeHostentWithTtlsHandler::PARSEhas, and each impl names its extern. The six externs have exactly that signature.ares_reply_callback, the only caller, keeps itsunsafeblock and calls(R::PARSE)(..), which is a direct call to the C function once the callback is monomorphised.15. c-ares:
get_sockaddrandget_name_infotakesockaddr_storageget_sockaddrtook&mut sockaddr, 16 bytes, and wrote a 28-bytesockaddr_in6through it. Its one caller,Resolver::global_lookup_service, cast itssockaddr_storageto that type in anunsafeblock, twice. Both functions now take thesockaddr_storagethe caller has.get_name_infotakes it shared:ares_getnameinfohasconst struct sockaddr *and copies the address. The first cast block in dns.rs goes, the second shrinks to&mut *request.Inside
get_sockaddr, the two address stores went through(&raw mut field).cast().write(..). They assignsin_addr.s_addrandsin6_addr.s6_addr, which libc andbun_windows_sys::ws2_32both name so: the same 4-byte and 16-byte stores.get_host_by_addrreported an unparsable address by calling its own C thunk withARES_ENOTIMP. The thunk only turns the pointer back into&mut Tand the status intoSome(Error::ENOTIMP), so it callsctx.on_hostentwith that. OnlyError::ENOTIMPnamesARES_ENOTIMPnow, so it is a plainconstlike its neighbours that only the enum names.The comment on the
winsockimport in cares_sys is cut to the one line that still holds.16. boringssl:
Bun__X509__checkHosttypes its certificate and its out-parameterx509was a*mut X509turned into a reference withunsafe { as_mut() }, andout_peera*mut Stringthatset_peertested for null and wrote withunsafe { out_peer.write(..) }.They are
Option<&mut X509>andOption<&mut MaybeUninit<String>>: the same two null tests, andMaybeUninit::writestores without dropping, asptr::writedid. The function staysunsafeforhost_ptr.Its one caller is
X509View::checkHostin ncrypto.cpp. It returns early whencert_is null, and passes either null or the address of a localBunStringthat it initialises to the empty string and does not touch during the call.X509is an opaque zero-sized!Unpintype, so&mut X509claims nothing about the certificate's memory.17. boringssl_sys:
X509_get_subject_nameis asafe fn, and a stack free without fn-pointer transmutesX509_get_subject_name(crypto/x509/x509_cmp.cc:69) returns the address of a member and reads nothing. It is asafe fnover&X509.common_namesis its only Rust caller.GeneralNames::droptransmutedGENERAL_NAME_freetofn(*mut c_void)and passed it tosk_pop_free_exasfree_func, and its trampoline unwrapped that argument and transmuted it back before calling it.OPENSSL_sk_pop_free_ex(crypto/stack/stack.cc:112) only passesfree_funcon to the trampoline, sodroppassesNoneand the trampoline callsGENERAL_NAME_freeby name. That removes two transmutes and anexpect, and the indirect call per element becomes a direct one.18. boringssl_sys: the digest getters return
&'static EVP_MD, andEVP_DigestInitis asafe fnThe 16
EVP_*()getters returned*const EVP_MD, soEVP::initwas a safe fn with a# Safetysection about itsmdpointer. Each getter returns the address of a static object (crypto/fipsmodule/digest/digests.cc.inc throughDEFINE_DATAin delocate.h:62, and crypto/digest/digest_extra.cc:276, 301, 326, 359, 393 and 416), so they return&'static EVP_MD.EVP_get_digestbyname(digest_extra.cc:218) andEVP_get_digestbynid(:79) walk one table (:43) and return what one of these getters orEVP_md5_sha1returns, or null.EVP_md5_sha1(:455) returns the address of a constant at file scope too. Both returnOption<&'static EVP_MD>.Algorithm::md()and theEVP::mdfield follow.Option<&EVP_MD>is one word whereOption<*const EVP_MD>was two.A result that goes on to a
*const EVP_MDparameter coerces, so those calls are unchanged. Two callers tested a result for null.EVP::by_namematches on theOption. InX509::tls_server_end_point_hashthe test stood after amatchwith anEVP_sha256()arm and anEVP_get_digestbynid()arm. The second arm ends in?and the test goes, so that arm has one null test as before and the first has none.EVP_DigestInit(crypto/fipsmodule/digest/digest.cc.inc:162) starts withEVP_MD_CTX_init, which overwrites the three pointers of the context before anything reads them, then callsEVP_DigestInit_ex, which keepstypeinctx->digest(:157). It becomessafe fn(&mut EVP_MD_CTX, &'static EVP_MD).EVP::initcalls it instead ofEVP_MD_CTX_initfollowed byEVP_DigestInit_ex: the same work in one call. Thenew_evp!hashers in sha.rs calledEVP_MD_CTX_initand thenEVP_DigestInit. They lose the first call and the block around the second: one FFI call fewer perinit, for the 8 types.EVP_MD_CTX_inithas no Rust caller left and its declaration goes.bun-sema links no BoringSSL and has weak stand-ins in src/sema/standalone/native_libraries.rs: its
EVP_sha256(:167) returns the address of a static, and itsEVP_DigestInit(:199) aborts on any othertype_and overwrites the whole context, so both keep what the signatures promise. ItsEVP_MD_CTX_init(:187) stays, because a test in that file calls it.EVP::sizeaskedEVP_MD_CTX_size(&self.ctx), which isEVP_MD_size(ctx->digest)after a null test.ctx.digestisself.mdfrominiton, so it asks the newsafe fn EVP_MD_size(&EVP_MD)(digest.cc.inc:36).ctxwas apubfield that nothing outside EVP.rs used. It is private, so only EVP.rs can break that equality.EVP_MD_CTX_sizehas no caller left and its declaration goes.EVP_MD_do_all_sortedhas no caller either, and its declaration goes too. The comment onmdrepeated its type and goes.EVP::resetis unchanged.19. net:
BlockList__onStructuredCloneDestroytakes theRefPtr<BlockList>it releasesThe hook was
unsafe extern "C" fn(*mut c_void)and ended inunsafe { BlockList::deref(ptr.cast()) }, the only use of that inherentunsafe fn deref.It now takes
bun_ptr::RefPtr<BlockList>by value, which isrepr(transparent)overNonNull, so C++ passes the same pointer. It removes the table entry under the lock as before and then drops theRefPtr, which calls the sameThreadSafeRefCount::deref: one ref down per call, matching the one ref up inBlockList__onStructuredCloneSerialize.BlockList::derefgoes.SerializedScriptValue.cpp calls it in two places, the destructor (line 4300) and
releaseSerializedRefsincreate(line 4957). Both walk a vector whose onlyappend(line 1440) stores theblockList->wrapped()that the line before passed to the serialize hook as&BlockList, and each vector is walked once.20. node:quic:
on_new_connuses theConnit already hasQuicEndpoint::on_new_connwraps lsquic's pointer in aConnat its top. Two later exits wrapped the same pointer again withunsafe { Conn::from_raw }and a null test to callabort_silent, and two more calledlsquic_conn_abort_erroron the raw pointer. All four use the firstConn.Conn::abort_errortakes a&CStr, so the two refusals passc""where they passed null.lsquic_conn_abort_error(lsquic_conn.c:208) replaces a null reason with"", so the frame is the same. The argument is now the address of a constant instead of zero.No other crate names
lsquic_conn_abort_error, so the declaration is private.21. node:quic: the loop-driver callbacks take
&QuicEndpoint;lookup_certstays typedBun__nodeQuic__drainEndpointandBun__nodeQuic__processEndpointwereunsafe extern "C" fn(*mut c_void)that cast the pointer throughunsafe fn from_driver_ownerand calledrun_driver_pass(&self). They take&QuicEndpoint, andfrom_driver_ownergoes.Each has one call site, node_quic_shim.c:601 and :615, both under
if (d->owner).owneris written only byus_nq_loop_register(:571), whose one caller,link_loop_driver, passesfrom_ref(self).unlink_loop_drivertakes the node off the list before the endpoint can be freed. A walk that is under way has read its successor already, so it can still call an endpoint that the pass before it closed and unlinked;run_driver_passreturns at once for it, as before. The reference lives exactly as long as the&selfofrun_driver_passdid.lookup_certalready heldthis: &QuicEndpointand still ended inunsafe { get_ssl_ctx(owner, local) }, which turned thevoid*back into the same reference behind a second null test. The body ofget_ssl_ctxmoves tofn server_ssl_ctx(&self), and both callbacks call it.lookup_certwas the one use of the formctx: *mut c_void as r: &Toflsquic_callback!. The two arms for that form were otherwise only what the two short arms forwarded to, so the short arms hold the expansion themselves and the forwarding goes. No other use of the macro changes.22. node:quic:
QuicEndpoint::constructorreads its options before it creates the cellThe constructor built the struct with defaults, moved it into its JS cell, and then stored each option it read through the raw payload pointer, in six
unsafe { (*raw).field.set(..) }blocks.It now reads the options into locals first and puts them in the struct literal, so seven fields are stored once instead of twice. The two blocks that need the cell's address or the wrapper (the vtable, and
state/stats) stay.What changes in order: the six property reads (
address,blockList,blockListPolicy,disableStatelessReset,statelessResetBurst,statelessResetRate) keep their order and their conversions, but run before the cell and the boxed vtable are allocated instead of after. If one of them throws, the cell and the vtable are never allocated, and the default bind host and theStrongon the block list are freed on return instead of when the GC finalizes the abandoned cell. Nothing could reach that cell, and itsDrophas nothing to release at that point. The one caller,new Endpoint_(options)in quic.ts, passes a fresh null-prototype object of data properties.23. node:quic:
QuicSession::createandQuicStream::createreturn the reference they makeBoth constructors form
&*payloadto finish setting the object up and then returned the raw pointer, so each caller went back throughunsafe { (*session).x() }with "was just created" as the reason.They return
&'a Self, and'ais the borrow of the owner.QuicSession::createtakesendpoint: &'a QuicEndpoint,QuicStream::createtakessession: &'a QuicSession, andcreate_streamborrows from&selfand passes the reference on.globalhas a lifetime of its own. Both constructors took the owner as*mut, and all five call sites passedfrom_ref(self).cast_mut(). They passself, and the cast stands at the one place in each constructor that stores the back-pointer. In every caller the owner is the caller's own&self, so no body can choose a longer'a. That is howlive_sessionandendpoint_refbound what they return.apply_server_session_optionstakes&QuicSession. Seven blocks go: inon_new_conn,apply_server_session_options,connectandconnect_verneg_probe, two inon_remote_streamand one inopen_stream. The large block inconnectshrinks to thelsquic_conn_set_ctxcall.The payload belongs to the GC, and the compiler does not check that it lives for
'a.createroots the wrapper withset_strongbefore it returns, and onlyteardown(for a stream, alsorelease_close_root) undoes that. After that the frame that has the reference holds the cell, which is whatJsWrapper::payload_refasks of its caller. Of the callers onlyconnecttears the session down, and it returns right after. Nothing forms a&mutto either payload before the sweeper drops it.Where a pointer is stored (
sessions,pending_new_sessions,provisional,pending_verneg,streams, the conn and stream ctx, the two back-pointers) it isptr::from_ref(x).cast_mut(), the form these files use forself. Every use of those pointers forms&T. The operations and their order are unchanged.24. node:quic: a session holds
Option<Conn>and a stream holdsOption<Stream>QuicSession::connandQuicStream::rawwereCell<*mut _>. Nine places wrapped the field again withunsafe { from_raw(..) }before using it, among them the safe accessorsconn()andls(). Ten safe functions took a raw lsquic pointer that they or a later function trusted:bind_conn,cache_sockaddrs,conn_peer_addr,on_remote_stream,bind_raw,QuicEndpoint::on_new_conn,defer_stream_abort,QuicSession::create,create_streamandQuicStream::create.bind_conn,on_remote_streamandbind_rawwrapped their parameter, andconnectwrapped at its end the pointer it had tested for null further up.The fields are
Cell<Option<Conn>>andCell<Option<Stream>>, and those ten functions take the handle or anOptionof it. 13 blocks go and none is added. Inconnect,from_rawjoins the block that callslsquic_engine_connect. The block inQuicEndpoint::on_new_connmoves into theon_new_conncallback with its SAFETY comment. What is left to prove sits where lsquic hands a pointer over (theon_new_conncallback,connect,on_new_stream,on_stream_read). The fields are cleared where they were set to null.DeferredAbort::lsstays a raw pointer with its ownfrom_raw, because what keeps it alive is a different argument.defer_stream_abortstoresls.raw()in it.In
connectthe localConnis used afterapply_options, as the raw pointer was. The SAFETY comment atfrom_rawsays why no script runs in between when quic.ts calls.ConnandStreamhold aNonNulland arerepr(transparent), so theOptionis one pointer with null asNone. Eachis_null()becomesis_none(),is_some()orif let Some, one for one. Six null tests go:connecttested the pointer fromlsquic_engine_connect, and again infrom_rawat its end.bind_connmatched onfrom_rawof its parameter.on_remote_streamdid the same.refresh_conn_statstested the field and thenfrom_rawof it.bind_rawhadfrom_raw(..).expect(..). Its panic path goes too.on_new_conncallback tested its pointer, andQuicEndpoint::on_new_conntested it again infrom_raw.report_closematches on the field instead of testing it in a guard and reading it again. 39 lines in lsquic_sys passself.0.as_ptr().Conngainsraw(), whichStreamhad. The other operations and their order are unchanged.25. ast:
Binding::to_exprtakes&mut Cand the method that wraps an identifierto_exprtook a*mut c_voidcontext and aToExprWrapper: a back reference to the arena and a fn pointer to a trampoline that cast the context back to&mut Por&mut ExportHoist. All four call sites hold that&mutand the arena, so they pass both, and name the method (Self::wrap_identifier_namespace,Self::wrap_identifier_hoisting,ExportHoist::wrap_identifier) instead of a trampoline around it.to_expris generic over the context and the function. The function is a zero-sized fn item, so nothing is passed for it and the call to it is direct.The cost is code size:
to_expris compiled once per method (four copies in bun_js_parser, one in bun_bundler) and not once in bun_ast. In a--releasebuild bun_js_parser grows by 526 instructions in 343,000 and bun_bundler by 78, and bun_ast shrinks by 553.ToExprWrappergoes away, and with it theexpecton its arena for every array and object pattern, the two fields ofPthat held it (32 bytes) and the code inprepare_for_visit_passthat filled them. One behaviour differs:to_expron aPbeforeprepare_for_visit_passused to panic and now works.26. js_parser:
P::initreturns the&mut Pit wrote, andinit_p!binds itP::init(out: &mut MaybeUninit<P>, ..)returnedResult<(), Error>.init_p!called it, moved the slot into a drop guard and gave the guard to the caller. Each of the four users of the macro then took its&mut Pout withunsafe { __p.assume_init_mut() }, and so did the fifth caller ofP::init, the copy of the macro that runtime/test_runner/snapshot.rs writes out by hand because the macro is local to its crate.initcannot fail: its body has no?,return,Err,unwrap,expectorpanic!. AndMaybeUninit::writereturns the reference the callers want. So the signature isThe guard holds that reference and the macro binds the caller's
pto it. The slot and the guard are locals of the expansion, which the caller cannot name. The guard's closure runsdrop_in_placeon the reference, which is whatassume_init_dropdoes. snapshot.rs does the same by hand: its guard typeInitializedParserholds the reference, and itsDroprunsdrop_in_place. Fiveunsafeblocks go, and with the?the macro's hidden early return.Compiled code, in a
--releasebuild of bun_js_parser (default flags, no LTO), the parent of the commit before this one against this commit, so with the 32 bytes that that commit takes offP:memcpyof the wholeP(4344 bytes) in every caller._parse::<false>and both_scan_importscopied it a second time to drop it. All eight copies are gone._parse::<false>13240 to 11016 bytes,_parse::<true>12856 to 10648,_scan_imports9480 to 5176,to_lazy_export_ast10008 to 5816._parse::<false>5133 to 5075,_parse::<true>5553 to 5477,_scan_imports::<false>895 to 830,to_lazy_export_ast582 to 569, and 4 fewer in each of the four instances ofinit.initreturns its first argument, and LLVM sees that. In the five callers that were read (both_parse, both_scan_imports,to_lazy_export_ast) the code after the call ignores the returned register and addressesp's fields off the stack pointer, as before, without a spilled pointer top.Pis dropped as before: inline in_scan_imports, by a call to its drop glue in_parse::<false>andto_lazy_export_ast, and by a call to the guard's drop glue in_parse::<true>.No cost was found in the assembly. Run time was not measured.
27. js_printer: the three
write_reservedcallers useprint_sliceWriterTrait::write_reservedgot a raw pointer tonspare bytes fromreserve, copied the bytes there and committed them withadvance. OnBufferWriter, the onlyWriterContext, that isVec::reserve, a copy andset_len, which is whatprint_slicedoes throughVec::extend_from_slice. The three callers useprint_slice, two of them throughself.printlike the code around them, andwrite_reserved,advanceandadvance_bygo away.reservehas two more callers,print_astandprint_with_writer_and_platform, which pre-size the buffer and ignore the pointer, soreserveandreserve_nextstay and return nothing.print_sliceis#[inline]down to theVec.reserve_nextandadvance_byare not, and the printer is instantiated in other crates, so without LTO eachwrite_reservedmade two calls into bun_js_printer.28. image, aot, mimalloc: imports that only write their out-parameters are
safe fnFour imported functions took raw pointers to locals of the caller, and every call was an
unsafeblock. Typed as references, the signature says that the pointers are valid:JSC__JSValue__borrowBytesForOffThread(bindings.cpp) writes*out_ptrand*out_len, or neither, and reads neither.tj3GetScalingFactors(turbojpeg.c) stores a constant in*nand returns its static table.Bun__AOT__initializeOptions(BunAOTCompile.cpp) callsWTF::initialize(), which is acall_once, and assignsmakeDefaultCompileOptions(): zeroes, three integers, a double and two enumerators of enums generated from the schema that also generates the Rust type. One of the integers comes fromWTF::numberOfProcessorCores(), which caches its answer in a plainstatic int; two first calls at once both store the same value, and safe Rust could already make them throughEngineOptions::new.mi_process_info(mimalloc, src/util/stats.c) tests each of its eight pointers for null before it writes through it, so they areOption<&mut usize>.&mut TandOption<&mut T>are passed like the pointers they replace.Image.rs also had its own declaration of
JSC__JSValue__unpinArrayBuffer. bun_jsc declares that functionsafeand wraps it asJSValue::unpin_array_buffer, which other files of bun_runtime call. The two calls in Image.rs use the wrapper. That is the removal of a duplicate declaration, not a proof: the compiler checks nothing new at those two sites, and the C++ function still wants a value that was pinned. In Image.rs that holds by construction, becausePin's field is private and aPinis built only for a result of 2.29. js_parser_jsc:
MacroContext::callborrowsself.macrosandself.bumpcallturned the map entry and the arena into*const Macroand*const Arena"so the closure does not extend&mut self", and the closure made references of them again with twounsafeblocks.Both are borrows of one field each. What else
calltouches while they live is other fields (self.javascript_object, and before thatself.envandself.transform_options), so the borrow checker accepts&Macroand&Arenaas they are.callholds&mut selfacross the macro's JS as before, andRunner::rungot the same two shared references.disabledandvmwere copied out of the entry "so the borrow can be released". They are read throughmacro_,vmafter thedisabledtest and not before it.30. ast, sql_jsc: remove three
unsafe impl Send/Syncthat nothing needsJsonTapeis fiveVec<_, TapeAlloc>and a fieldless enum.TapeAllocand what the element types hold (StoreStr,StoreRef<ObjectJSON>,StoreRef<ArrayJSON>) haveSendandSyncimpls of their own, none behind acfg, soJsonTapeisSend + Syncas an auto trait and its two impls said nothing more.sql_jsc's handle to the runtime's
SSLConfigwas declaredSend. No code asks for that. The type is not named outside the crate, and none of its four holders looks at the bound:MySQLConnectionsits inJSMySQLConnectionas aJsCell<MySQLConnection>, andJsCell<T>isSend + Syncfor everyT.PostgresSQLConnectionandConnectParamshold anRc<SSLConfig>, which is neverSend.ConnectionCtorArgsispub(crate), and nothing in the crate puts a bound on it.Sendoccurs nowhere else in the code of sql_jsc or sql, and the crate has no platformcfg. Without the impl the handle is!Send, like theNonNullin it.The entry for src/sql_jsc/jsc.rs leaves vm-thread-door.inventory.json.
31. css: two needless
unsafein values/ident.rsIdentOrRef::as_identbuilt a&[u8]withfrom_raw_partsonly to turn it into the*const [u8]thatIdentstores.slice_from_raw_partsbuilds that pointer and no reference.arena_slice_newtype!gave its three types anunsafe fn as_slicethat calls the safev(). It has no caller.32. valkey, image, sql_jsc, ast:
&Selfguards,&Blob,&Symbol, aconstarrayJSValkeyClient::on_valkey_connectputself.as_ctx_ptr()in its drop guard and dereferenced it there. Both calls in the guard take&self, and&SelfisCopy, so the guard holdsself. The eight other drop guards in js_valkey.rs heldBackRef::new(self)(orthis) for the same kind of call. They hold the plain reference too, with no other change.BlobReadChain::startmade a&mut Blobof whatas_::<Blob>()returned and only calledread_bytes_to_handler(&self, ..)on it, which can run the handler, and through it JS, before it returns.as_class_refisas_plus the shared dereference, so the same tests run.CachedStructure::build_from_columnsmade its array ofMaybeUninitwithMaybeUninit::uninit().assume_init().[const { .. }; 70]is the safe spelling. In an optimised build neither form writes anything; at opt-level 0 (the debug build) the old form copied the whole array with amemcpyand the new one fills it with amemset, 70 × 32 = 2240 bytes either way.Symbol::merge_contents_withonly readsold, so it takes&Symbol. Nounsafegoes away with this one: its caller makes both references from raw pointers.33. aot, md: three byte views go through
bun_core::slice_as_bytesOutputSlice::put_words(&[u32]), the test helperbytes_of(&[u64]) and the markdown parser's copy ofcurrent_block_linesintoblock_bytes(&[VerbatimLine]) each built the&[u8]withfrom_raw_parts.slice_as_bytesis that same expression,#[inline(always)], behind the boundT: NoUninit.bytes_ofwould only be another name for it, so it goes and its four call sites, all in tests, callslice_as_bytes.VerbatimLine(#[repr(C)], threeu32) gets the bound fromimpl_no_uninit!, which compiles only if rustc can prove that the type has no padding.34. ast:
Rope<'a>links its segments withOption<&'a mut Rope<'a>>Rope::nextwas a*mut Rope.appendwalked the chain throughStoreRef, andnext_refturned the pointer into a&'a Ropeof any lifetime with anunsafeblock forget_or_put_object.Every segment is allocated in the arena that the head is allocated in (
ropeallocin the INI parser;bun inithas a single segment), so the link is a&'a mutinto that arena.appendwalks a&mut Option<..>cursor, andget_or_put_objectreadsrope.next.as_deref().appendreturnedResult<*mut Rope, AllocError>, alwaysOk, and its three call sites dropped the pointer. It returns nothing.The null niche keeps the layout. In a
--releasebuild of bun_ast,get_or_put_objectcompiles to the same instructions andappendloses the two that built its return value.35. c-ares:
ResolveHandlernames its callback as an associated constResolveHandler::raw_callbackwas anunsafe extern "C" fnin the trait. Its one impl, forResolveInfoRequest<T>, forwarded its five arguments toT::RAW_CALLBACKfrom inside anunsafeblock.The trait now has
const RAW_CALLBACK: unsafe extern "C" fn(..), the shapeCAresRecordType::RAW_CALLBACKandAresReply::PARSEhave, and the impl namesT::RAW_CALLBACK.Channel::resolve, the only user, keeps its twounsafeblocks: it calls the constant for a bad name and hands it toares_queryotherwise. c-ares then calls the record type's thunk itself, and not a thunk that calls it.How did you verify your code works?
cargo fmt --check,cargo clippy --workspace(0 warnings), the source lints andcargo check --workspace --releaseon Linux, andcargo check --workspace --releaseforx86_64-pc-windows-msvc,aarch64-apple-darwinandx86_64-unknown-freebsd, because four changes touch code that Linux does not compile.unsafeblocks fromBlob,Bun.write,FileSink,fetch,Responseand the S3 client #47895, which is under these, and a rebase brought in one more commit ofmain(install: send configured credentials to tarball URLs that the project names #47900). Together they touch 17 files. This PR touches one of them, Image.rs, in other functions, and all 35 patches here stayed as they were. Then changes 2 and 17 were narrowed: six imports that they had markedsafe fnstay unsafe imports and are called as onmain. I compared the release assembly of all crates before and after that, and no function differs. The static checks ran last.macro-test,css.test.ts,image.test.ts,image-adversarial,bun-cryptohasher,x509,node-https-checkServerIdentity,blocklist-gc,napi-value-ffi,cc.test.tsandptr-byte-offsetofbun:ffi,ini.test.ts,valkey-gc, 4 files on IPC (spawn.ipc,spawn-ipc-gc,bun-ipc-inherit,child_process_ipc), 4 oftest/js/node/zliband 3 oftest/js/node/quic.node:quicpass:test-quic-blocklist,test-quic-connection-limits,test-quic-endpoint-busyandtest-quic-internal-endpoint-options.node-dns.test.jspasses 144 tests and fails 28, the same 28 as on a debug build without these commits. They need a DNS server, which I did not have.snapshot.test.tswas stopped at 50 seconds, also when it ran alone. No test had failed by then. Its groupinline snapshots, which runs the code that change 26 touches in snapshot.rs, passes when run by name: 35 tests, and 1 that the file skips.transpiler.test.js,decorators,decorator-metadata,es-decorators, andvitest-flavorandexpect-deferred, whose callbacks go throughContextParameter::of.bake/dev/esm.test.ts, for the bundler'sto_expr, passes 12 tests and fails 7, the same 7 as on a debug build without these commits. They needhappy-dom, which I did not have.ffi.test.jswas stopped at 50 seconds. No test had failed by then.Generated by Claude Code