Skip to content
Merged
8 changes: 4 additions & 4 deletions src/bun_alloc/heap_breakdown.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,11 @@ impl Zone {
/// `'static`: `malloc_set_zone_name` may keep the pointer instead of copying.
#[cfg(target_os = "macos")]
pub fn init(name: &'static CStr) -> &'static Zone {
// SAFETY: malloc_create_zone is safe to call with (0, 0); returns a
// process-lifetime zone pointer. `name` is NUL-terminated and outlives the
// process.
let zone = malloc_create_zone(0, 0);
assert!(!zone.is_null(), "malloc_create_zone failed");
// SAFETY: the zone is not null and nothing destroys it. `name` is NUL-terminated and
// outlives the process.
unsafe {
let zone = malloc_create_zone(0, 0);
malloc_set_zone_name(zone, name.as_ptr());
&*zone
}
Expand Down
9 changes: 7 additions & 2 deletions src/io/posix_event_loop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -626,7 +626,11 @@ impl FilePoll {
EPOLL::CTL_ADD
};

let ctl = linux::epoll_ctl(watcher_fd, op, fd.native(), Some(&event));
// SAFETY: `watcher_fd` is the epoll instance of `loop_`, whose dispatch decodes a
// `Pollable`. `deinit` removes the entry, or leaves one that `EPOLLONESHOT` disarmed
// when it fired, which is not reported again. `Store::put` then holds the slot back,
// marked `IgnoreUpdates`, until a tick returns, when no event fetched for it is left.
Comment on lines +629 to +632

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 pre-existing, not blocking: Pre-existing: a FilePoll slot can be recycled while the outer epoll/kqueue batch still holds an event for it, so a handler that nests a tick can get a stale event dispatched to a fresh poll or a poisoned/freed slot. The SAFETY text at src/io/posix_event_loop.rs:631-632 says the slot is held until a tick returns with no fetched event left, but process_deferred_frees runs from on_after_event_loop after every tick, nested ones included. Fix: free deferred FilePoll slots only when no outer dispatch is in progress (e.g. gate the after-loop thunk on the loop's tick_depth being 0, as loop.c:505 does for closed sockets), so the invariant the comment states holds for every caller of tick_with_timeout/tick_without_idle.
A small fix can ride a push you are already making; otherwise a short reply is enough.

Why this was flagged

Trigger: an outer tick fetched a batch with events for slots A and B (packages/bun-usockets/src/eventing/epoll_kqueue.c:536, dispatched at :260). A's handler deinit()s B (src/io/posix_event_loop.rs:361-388 puts B on the deferred list with IgnoreUpdates at :1377) and deactivates the last registered poll so num_polls is 0, then runs user JS that waits synchronously (src/jsc/event_loop.rs:1270-1273 wait_for_promise -> auto_tick). The nested us_loop_run_bun_tick returns at epoll_kqueue.c:479-480 without overwriting num_ready_polls, so the outer batch stays live, yet src/runtime/jsc_hooks.rs:1167 still calls on_after_event_loop, which runs process_deferred_frees and hive.puts B. When the outer loop reaches B's entry, Bun__internal_dispatch_ready_poll (src/io/posix_event_loop.rs:1483-1484) reads flags of a freed slot, or, if A's handler got slot B back from get_init, delivers A's batch event to the new owner. Base branch behaves the same; the diff adds the SAFETY claim that rules it out.

Verification: pre-existing. auto_tick (src/runtime/jsc_hooks.rs:1167) calls (*vm).on_after_event_loop() unconditionally after every tick, nested or not. process_deferred_frees (:1307-1324) then calls hive.put(current). The outer loop resumes with its original batch and calls Bun__internal_dispatch_ready_poll for B, which reads (*B).flags at src/io/posix_event_loop.rs:1483-1484 from a slot already returned to the hive.

let ctl = unsafe { linux::epoll_ctl(watcher_fd, op, fd.native(), Some(&event)) };
self.flags.insert(Flags::WasEverRegistered);
if let Some(errno) = errno_sys(ctl, sys::Tag::epoll_ctl) {
self.deactivate(loop_);
Expand Down Expand Up @@ -940,7 +944,8 @@ impl FilePoll {
{
use bun_sys::linux::{self, EPOLL};
// CTL_DEL keys on fd alone, so both directions are removed together.
let ctl = linux::epoll_ctl(watcher_fd, EPOLL::CTL_DEL, fd.native(), None);
// SAFETY: CTL_DEL registers nothing.
let ctl = unsafe { linux::epoll_ctl(watcher_fd, EPOLL::CTL_DEL, fd.native(), None) };

match sys::get_errno(ctl) {
sys::E::SUCCESS => {}
Expand Down
6 changes: 4 additions & 2 deletions src/runtime/ffi/ffi_body.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1361,10 +1361,12 @@ impl FFI {
pub(crate) fn close<'a>(&self, this_value: JsWrapper<'a, Self>, cx: &mut JsScope<'a>) {
unsafe extern "C" {
/// Type-checks `library` (`dynamicDowncast`) before use.
safe fn Bun__FFI__closeFunctions(global: &JSGlobalObject, library: JSValue) -> bool;
fn Bun__FFI__closeFunctions(global: &JSGlobalObject, library: JSValue) -> bool;
}
jsc::mark_binding();
let is_running = Bun__FFI__closeFunctions(cx.global(), this_value.to_js());
// SAFETY: C++ indexes `functionsValue` unchecked up to its length. It is empty or the array
// that `cc` filled from 0 to its length, which JS never sees.
let is_running = unsafe { Bun__FFI__closeFunctions(cx.global(), this_value.to_js()) };
if is_running {
// The running call returns into the compiled code, so leak it instead of letting `do_close` free it.
self.shared_state.set(None);
Expand Down
11 changes: 6 additions & 5 deletions src/runtime/webcore/TextDecoder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use crate::webcore::jsc::{self as jsc, JSGlobalObject, JSString, JSValue};
use bun_core::strings;
use bun_jsc::HostReturn as _;
use bun_jsc::bindgen::{self, JsHeap, JsLocal, JsScope};
use core::mem::MaybeUninit;
use std::collections::TryReserveError;

use jsc::StringJsc as _;
Expand Down Expand Up @@ -496,10 +497,10 @@ pub(crate) extern "C" fn TextDecoder__createForStream(
label: JSValue,
fatal: bool,
ignore_bom: bool,
out_utf8_fast_path: &mut bool,
out_encoding: &mut EncodingLabel,
out_utf8_fast_path: &mut MaybeUninit<bool>,
out_encoding: &mut MaybeUninit<EncodingLabel>,
) -> *mut TextDecoder {
*out_utf8_fast_path = false;
out_utf8_fast_path.write(false);
let encoding = if label.is_undefined() {
EncodingLabel::Utf8
} else {
Expand All @@ -524,9 +525,9 @@ pub(crate) extern "C" fn TextDecoder__createForStream(
}
}
};
*out_encoding = encoding;
out_encoding.write(encoding);
if matches!(encoding, EncodingLabel::Utf8) && !fatal {
*out_utf8_fast_path = true;
out_utf8_fast_path.write(true);
return core::ptr::null_mut();
}
bun_core::heap::into_raw(Box::new(TextDecoder {
Expand Down
20 changes: 13 additions & 7 deletions src/spawn/process.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3603,12 +3603,17 @@ mod spawn_process_body {
}

let mut receipts: [libc::kevent; 5] = bun_core::ffi::zeroed();
match bun_sys::kevent(
kq_fd,
&changes_buf[..changes_len],
&mut receipts[..changes_len],
None,
) {
// SAFETY: the loop below reads `udata` as a number. The one other reader of `kq_fd`,
// `NoOrphansTracker::killTracked`, throws the events away.
let registered = unsafe {
bun_sys::kevent(
kq_fd,
&changes_buf[..changes_len],
&mut receipts[..changes_len],
None,
)
};
match registered {
Err(err) => return Some(Err(err)),
Ok(_) => {}
}
Expand Down Expand Up @@ -3652,7 +3657,8 @@ mod spawn_process_body {
let mut child_exited = false;
let mut child_status: Option<Status> = None;
loop {
let got = match bun_sys::kevent(kq_fd, &[], &mut events[..], None) {
// SAFETY: an empty changelist registers nothing.
let got = match unsafe { bun_sys::kevent(kq_fd, &[], &mut events[..], None) } {
Err(err) => return Some(Err(err)),
Ok(c) => c,
};
Expand Down
24 changes: 20 additions & 4 deletions src/sys/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5590,9 +5590,20 @@ pub mod linux {
unsafe { libc::ppoll(fds.as_mut_ptr(), fds.len() as _, timeout, sigmask) }
}
/// `EPOLL_CTL_DEL` takes no `event`.
///
/// # Safety
/// Whoever waits on `epfd` gets `event.u64` back and may dereference it. It must be what they
/// expect for as long as `fd` is registered, and until they are done with the events that
/// they had fetched by then.
#[inline]
pub fn epoll_ctl(epfd: c_int, op: c_int, fd: c_int, event: Option<&epoll_event>) -> c_int {
super::linux_syscall::epoll_ctl(epfd, op, fd, event)
pub unsafe fn epoll_ctl(
epfd: c_int,
op: c_int,
fd: c_int,
event: Option<&epoll_event>,
) -> c_int {
// SAFETY: the caller's contract.
unsafe { super::linux_syscall::epoll_ctl(epfd, op, fd, event) }
Comment thread
dylan-conway marked this conversation as resolved.
Resolved
}

// ── raw syscall thunks ──
Expand Down Expand Up @@ -7518,16 +7529,21 @@ pub fn kqueue() -> Maybe<Fd> {

/// `kevent()` — slice-wrapped Maybe form. Retries on EINTR.
/// Returns the number of events written into `eventlist`.
///
/// # Safety
/// Whoever waits on `fd` gets the `udata` of each change back and may dereference it. It must be
/// what they expect for as long as the change is registered, and until they are done with the
/// events that they had fetched by then.
#[cfg(any(target_os = "macos", target_os = "freebsd"))]
pub fn kevent(
pub unsafe fn kevent(
fd: Fd,
changelist: &[libc::kevent],
eventlist: &mut [libc::kevent],
timeout: Option<&libc::timespec>,
) -> Maybe<usize> {
loop {
// SAFETY: fd is a valid kqueue; slices give exact (ptr,len); timeout
// is either null or a valid timespec.
// is either null or a valid timespec. The caller answers for each `udata`.
let rc = unsafe {
libc::kevent(
fd.native(),
Expand Down
12 changes: 10 additions & 2 deletions src/sys/linux_syscall.rs
Original file line number Diff line number Diff line change
Expand Up @@ -420,11 +420,19 @@ pub(crate) fn read_raw(fd: i32, buf: &mut [u8]) -> isize {
/// (c) synthesizing EINVAL for unknown `op` values ourselves rather than
/// letting the kernel reject them.
/// Passing the raw quad straight through avoids all three.
///
/// # Safety
/// As for `linux::epoll_ctl`.
#[inline]
pub(crate) fn epoll_ctl(epfd: i32, op: i32, fd: i32, event: Option<&libc::epoll_event>) -> i32 {
pub(crate) unsafe fn epoll_ctl(
epfd: i32,
op: i32,
fd: i32,
event: Option<&libc::epoll_event>,
) -> i32 {
let event = event.map_or(core::ptr::null(), core::ptr::from_ref);
// SAFETY: the kernel validates `epfd`/`op`/`fd`; `event` is live, or null, which is
// EFAULT for every `op` but CTL_DEL.
// EFAULT for every `op` but CTL_DEL. The caller answers for `event.u64`.
unsafe { libc::syscall(libc::SYS_epoll_ctl, epfd, op, fd, event) as i32 }
}

Expand Down
64 changes: 46 additions & 18 deletions src/uws_sys/Loop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -239,12 +239,17 @@ impl PosixLoop {
pub fn drain_quic_if_necessary(&self) {
if !self.internal_loop_data.nq_head.get().is_null() {
// Full pass with close dispatch deferred to the next loop point.
c::us_nq_loop_drain(self);
// SAFETY: `self` is this thread's live loop. The walk holds its successor while JS
// runs, and relies on an endpoint staying rooted until its close is delivered.
unsafe { c::us_nq_loop_drain(self) };
}
if !self.internal_loop_data.has_quic() {
return;
}
c::us_quic_loop_flush_if_pending(self);
// SAFETY: `self` is this thread's live loop. `processing` keeps this out of every lsquic
// callback but the `on_stream_open` that `us_quic_socket_make_stream` makes, and that one
// does not flush.
unsafe { c::us_quic_loop_flush_if_pending(self) };
}

/// `None` if epoll/kqueue cannot be created (EMFILE).
Expand All @@ -262,18 +267,25 @@ impl PosixLoop {
}

pub fn tick(&self) {
c::us_loop_run_bun_tick(self, None, NOW_NS_UNKNOWN);
// SAFETY: as in `tick_with_timeout`.
unsafe { c::us_loop_run_bun_tick(self, None, NOW_NS_UNKNOWN) };
}

pub fn tick_without_idle(&self) {
c::us_loop_run_bun_tick(self, Some(&Timespec { sec: 0, nsec: 0 }), NOW_NS_UNKNOWN);
// SAFETY: as in `tick_with_timeout`.
unsafe {
c::us_loop_run_bun_tick(self, Some(&Timespec { sec: 0, nsec: 0 }), NOW_NS_UNKNOWN)
};
}

/// `now_ns` is the CLOCK_MONOTONIC reading the caller took to pick `timespec` (see
/// `timer::All::get_timeout`), reused by the JS park hook's idle-sweep rate limit rather
/// than read again. `NOW_NS_UNKNOWN` if the caller has none to share.
pub fn tick_with_timeout(&self, timespec: Option<&Timespec>, now_ns: u64) {
c::us_loop_run_bun_tick(self, timespec, now_ns);
// SAFETY: `self` is this thread's live loop, and `jsc_vm` is null or its live VM. A handler
// may tick (`tick_depth`), but that refills the receive buffer and can dispatch the same
// socket again: such a handler is done with what the dispatch lent it.
unsafe { c::us_loop_run_bun_tick(self, timespec, now_ns) };
Comment thread
dylan-conway marked this conversation as resolved.
Resolved
}

/// Free everything queued on `loop->data.closed_head` /
Expand All @@ -298,7 +310,10 @@ impl PosixLoop {
/// loop — covers Listener/App-owned groups that `RareData`'s static field
/// list doesn't enumerate. Returns whether any group was linked.
pub fn close_all_groups(&self) -> bool {
c::us_loop_close_all_groups(self) != 0
// SAFETY: `self` is this thread's live loop. The walk reads its successor after the close
// handlers ran, so none may free another group. The one caller runs at teardown, with the
// owners of the groups stopped and script forbidden.
unsafe { c::us_loop_close_all_groups(self) != 0 }
}

pub fn should_enable_date_header_timer(&self) -> bool {
Expand Down Expand Up @@ -393,22 +408,29 @@ impl WindowsLoop {
/// Windows the park hook is driven from `us_loop_run` (libuv.c), which reads libuv's
/// already-refreshed clock via `uv_now` rather than taking one of its own.
pub fn tick_with_timeout(&self, _: Option<&Timespec>, _now_ns: u64) {
c::us_loop_run(self);
// SAFETY: as in `run`.
unsafe { c::us_loop_run(self) };
}

pub fn tick_without_idle(&self) {
c::us_loop_pump(self);
// SAFETY: as in `run`.
unsafe { c::us_loop_pump(self) };
}

pub fn drain_quic_if_necessary(&self) {
if !self.internal_loop_data.nq_head.get().is_null() {
// Full pass with close dispatch deferred to the next loop point.
c::us_nq_loop_drain(self);
// SAFETY: `self` is this thread's live loop. The walk holds its successor while JS
// runs, and relies on an endpoint staying rooted until its close is delivered.
unsafe { c::us_nq_loop_drain(self) };
}
if !self.internal_loop_data.has_quic() {
return;
}
c::us_quic_loop_flush_if_pending(self);
// SAFETY: `self` is this thread's live loop. `processing` keeps this out of every lsquic
// callback but the `on_stream_open` that `us_quic_socket_make_stream` makes, and that one
// does not flush.
unsafe { c::us_quic_loop_flush_if_pending(self) };
}

/// `None` if `uv_loop_init` fails (handle exhaustion).
Expand All @@ -421,7 +443,10 @@ impl WindowsLoop {
}

pub fn run(&self) {
c::us_loop_run(self);
// SAFETY: `self` is this thread's live loop, and `jsc_vm` is null or its live VM. A handler
// may tick (`tick_depth`, `poll_cb_depth`), though libuv does not provide for `uv_run` in a
// callback: such a handler is done with what the dispatch lent it, its handle included.
unsafe { c::us_loop_run(self) };
}

// TODO: remove these two aliases
Expand Down Expand Up @@ -464,7 +489,10 @@ impl WindowsLoop {
}

pub fn close_all_groups(&self) -> bool {
c::us_loop_close_all_groups(self) != 0
// SAFETY: `self` is this thread's live loop. The walk reads its successor after the close
// handlers ran, so none may free another group. The one caller runs at teardown, with the
// owners of the groups stopped and script forbidden.
unsafe { c::us_loop_close_all_groups(self) != 0 }
}

pub fn update_date(&self) {
Expand Down Expand Up @@ -508,21 +536,21 @@ mod c {
ext_size: c_uint,
) -> *mut Loop;
pub(super) fn us_loop_free(loop_: *mut Loop);
pub(super) safe fn us_quic_loop_flush_if_pending(loop_: &Loop);
pub(super) safe fn us_nq_loop_drain(loop_: &Loop);
pub(super) fn us_quic_loop_flush_if_pending(loop_: &Loop);
pub(super) fn us_nq_loop_drain(loop_: &Loop);
#[cfg(windows)]
pub(super) safe fn us_loop_run(loop_: &Loop);
pub(super) fn us_loop_run(loop_: &Loop);
#[cfg(windows)]
pub(super) safe fn us_loop_pump(loop_: &Loop);
pub(super) fn us_loop_pump(loop_: &Loop);
pub fn us_wakeup_loop(loop_: *const Loop);
#[cfg(not(windows))]
pub(super) safe fn us_loop_run_bun_tick(
pub(super) fn us_loop_run_bun_tick(
loop_: &Loop,
timeout_ms: Option<&Timespec>,
now_ns: u64,
);
pub(super) fn us_internal_free_closed_sockets(loop_: &Loop);
pub(super) safe fn us_loop_close_all_groups(loop_: &Loop) -> c_int;
pub(super) fn us_loop_close_all_groups(loop_: &Loop) -> c_int;
#[cfg(not(windows))]
pub(super) safe fn uws_get_loop() -> *mut Loop;
#[cfg(windows)]
Expand Down
6 changes: 4 additions & 2 deletions src/watcher/KEventWatcher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,15 +56,17 @@ pub(crate) fn watch_loop_cycle(this: &mut Watcher) -> bun_sys::Result<()> {

let mut changelist: [libc::kevent; CHANGELIST_COUNT] = bun_core::ffi::zeroed();

let mut count = bun_sys::kevent(fd, &[], &mut changelist, None)?;
// SAFETY: an empty changelist registers nothing.
let mut count = unsafe { bun_sys::kevent(fd, &[], &mut changelist, None) }?;

// Give the events more time to coalesce
if count < CHANGELIST_COUNT / 2 {
let ts = libc::timespec {
tv_sec: 0,
tv_nsec: 100_000,
}; // 0.0001 seconds
count += bun_sys::kevent(fd, &[], &mut changelist[count..], Some(&ts))?;
// SAFETY: as above.
count += unsafe { bun_sys::kevent(fd, &[], &mut changelist[count..], Some(&ts)) }?;
}

let changes = &changelist[..count];
Expand Down
3 changes: 2 additions & 1 deletion src/watcher/Watcher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -517,7 +517,8 @@ impl Watcher {
// Basically:
// - We register the event here.
// our while(true) loop above receives notification of changes to any of the events created here.
let _ = bun_sys::kevent(self.platform.fd, &[event], &mut [], None);
// SAFETY: only `watch_loop_cycle` waits on this kqueue, and it reads `udata` as an index.
let _ = unsafe { bun_sys::kevent(self.platform.fd, &[event], &mut [], None) };
}

fn append_file_assume_capacity<const CLONE_FILE_PATH: bool>(
Expand Down
Loading