diff --git a/.github/workflows/miri.yml b/.github/workflows/miri.yml index 1ee1447252a2..d253d9db3279 100644 --- a/.github/workflows/miri.yml +++ b/.github/workflows/miri.yml @@ -11,6 +11,7 @@ on: # The FFI-free crate set covered by MIRI_CRATES in scripts/rust-miri.ts - "src/ast/**" - "src/base64/**" + - "src/cares_sys/**" - "src/clap/**" - "src/collections/**" - "src/dispatch/**" diff --git a/scripts/rust-miri.ts b/scripts/rust-miri.ts index 6d690e4a82cf..ef753c041fcc 100644 --- a/scripts/rust-miri.ts +++ b/scripts/rust-miri.ts @@ -33,6 +33,9 @@ const repo = resolve(import.meta.dirname, ".."); const MIRI_CRATES = [ "bun_ast", "bun_base64", + // Declares c-ares externs; its tests only take the paths that complete a + // request without calling one. + "bun_cares_sys", "bun_clap", "bun_collections", "bun_dispatch", diff --git a/src/cares_sys/c_ares.rs b/src/cares_sys/c_ares.rs index 7c07c6724364..03b2914245e2 100644 --- a/src/cares_sys/c_ares.rs +++ b/src/cares_sys/c_ares.rs @@ -311,10 +311,22 @@ pub struct struct_hostent { // stable, so the wrappers below are expressed as a trait: the implementing // type provides the callback as a trait method, and the `extern "C"` thunk is // monomorphized per `T: Trait`. +// +// The request stays a `*mut` through registration, thunk and handler: the +// handler frees it, and c-ares may run the handler before the registering +// call returns, so no frame may hold it as a reference (freeing behind a +// reference argument is UB; `mod tests` below checks this under Miri). // ────────────────────────────────────────────────────────────────────────── pub trait HostentHandler: Sized { - fn on_hostent(&mut self, status: Option, timeouts: i32, results: *mut struct_hostent); + /// # Safety + /// `this` is the registered request, live and unaliased; it may be freed here. + unsafe fn on_hostent( + this: *mut Self, + status: Option, + timeouts: i32, + results: *mut struct_hostent, + ); } impl struct_hostent { @@ -326,13 +338,14 @@ impl struct_hostent { timeouts: c_int, hostent: *mut struct_hostent, ) { - // SAFETY: ctx was passed as `*mut T` to the ares call that registered this thunk. - let this = unsafe { bun_core::callback_ctx::(ctx) }; + let this = ctx.cast::(); if status != ARES_SUCCESS { - this.on_hostent(Error::get(status), timeouts, ptr::null_mut()); + // SAFETY: `ctx` is the `*mut T` registered by `Channel::get_host_by_addr`; delivered once, unused after. + unsafe { T::on_hostent(this, Error::get(status), timeouts, ptr::null_mut()) }; return; } - this.on_hostent(None, timeouts, hostent); + // SAFETY: as above. + unsafe { T::on_hostent(this, None, timeouts, hostent) }; } // One `extern "C"` thunk per lookup name. @@ -343,10 +356,10 @@ impl struct_hostent { buffer: *mut u8, buffer_length: c_int, ) { - // SAFETY: ctx was passed as *mut T to the ares call that registered this thunk. - let this = unsafe { bun_core::callback_ctx::(ctx) }; + let this = ctx.cast::(); if status != ARES_SUCCESS { - this.on_hostent(Error::get(status), timeouts, ptr::null_mut()); + // SAFETY: `ctx` is the `*mut T` registered by `Channel::resolve`; delivered once, unused after. + unsafe { T::on_hostent(this, Error::get(status), timeouts, ptr::null_mut()) }; return; } let mut start: *mut struct_hostent = ptr::null_mut(); @@ -363,10 +376,12 @@ impl struct_hostent { ) }; if result != ARES_SUCCESS { - this.on_hostent(Error::get(result), timeouts, ptr::null_mut()); + // SAFETY: as above. + unsafe { T::on_hostent(this, Error::get(result), timeouts, ptr::null_mut()) }; return; } - this.on_hostent(None, timeouts, start); + // SAFETY: as above. + unsafe { T::on_hostent(this, None, timeouts, start) }; } pub unsafe extern "C" fn callback_wrapper_ns( @@ -376,20 +391,22 @@ impl struct_hostent { buffer: *mut u8, buffer_length: c_int, ) { - // SAFETY: ctx was passed as *mut T to the ares call that registered this thunk. - let this = unsafe { bun_core::callback_ctx::(ctx) }; + let this = ctx.cast::(); if status != ARES_SUCCESS { - this.on_hostent(Error::get(status), timeouts, ptr::null_mut()); + // SAFETY: `ctx` is the `*mut T` registered by `Channel::resolve`; delivered once, unused after. + unsafe { T::on_hostent(this, Error::get(status), timeouts, ptr::null_mut()) }; return; } let mut start: *mut struct_hostent = ptr::null_mut(); // SAFETY: c-ares FFI; pointers are valid stack/null per contract. let result = unsafe { ares_parse_ns_reply(buffer, buffer_length, &raw mut start) }; if result != ARES_SUCCESS { - this.on_hostent(Error::get(result), timeouts, ptr::null_mut()); + // SAFETY: as above. + unsafe { T::on_hostent(this, Error::get(result), timeouts, ptr::null_mut()) }; return; } - this.on_hostent(None, timeouts, start); + // SAFETY: as above. + unsafe { T::on_hostent(this, None, timeouts, start) }; } pub unsafe extern "C" fn callback_wrapper_ptr( @@ -399,10 +416,10 @@ impl struct_hostent { buffer: *mut u8, buffer_length: c_int, ) { - // SAFETY: ctx was passed as *mut T to the ares call that registered this thunk. - let this = unsafe { bun_core::callback_ctx::(ctx) }; + let this = ctx.cast::(); if status != ARES_SUCCESS { - this.on_hostent(Error::get(status), timeouts, ptr::null_mut()); + // SAFETY: `ctx` is the `*mut T` registered by `Channel::resolve`; delivered once, unused after. + unsafe { T::on_hostent(this, Error::get(status), timeouts, ptr::null_mut()) }; return; } let mut start: *mut struct_hostent = ptr::null_mut(); @@ -418,10 +435,12 @@ impl struct_hostent { ) }; if result != ARES_SUCCESS { - this.on_hostent(Error::get(result), timeouts, ptr::null_mut()); + // SAFETY: as above. + unsafe { T::on_hostent(this, Error::get(result), timeouts, ptr::null_mut()) }; return; } - this.on_hostent(None, timeouts, start); + // SAFETY: as above. + unsafe { T::on_hostent(this, None, timeouts, start) }; } /// FFI destroy — frees a c-ares-allocated hostent. @@ -450,8 +469,10 @@ pub trait HostentWithTtlsHandler: Sized { /// parser for [`hostent_with_ttls::callback_wrapper`]. const PARSE: fn(&[u8]) -> Result, Error>; - fn on_hostent_with_ttls( - &mut self, + /// # Safety + /// `this` is the registered request, live and unaliased; it may be freed here. + unsafe fn on_hostent_with_ttls( + this: *mut Self, status: Option, timeouts: i32, results: Option>, @@ -468,10 +489,10 @@ impl hostent_with_ttls { buffer: *mut u8, buffer_length: c_int, ) { - // SAFETY: ctx was passed as *mut T to the ares call that registered this thunk. - let this = unsafe { bun_core::callback_ctx::(ctx) }; + let this = ctx.cast::(); if status != ARES_SUCCESS { - this.on_hostent_with_ttls(Error::get(status), timeouts, None); + // SAFETY: `ctx` is the `*mut T` registered by `Channel::resolve`; delivered once, unused after. + unsafe { T::on_hostent_with_ttls(this, Error::get(status), timeouts, None) }; return; } // SAFETY: c-ares passes the reply buffer it owns; valid for `buffer_length` bytes. @@ -479,8 +500,10 @@ impl hostent_with_ttls { core::slice::from_raw_parts(buffer, usize::try_from(buffer_length).unwrap_or(0)) }; match (T::PARSE)(buffer) { - Ok(result) => this.on_hostent_with_ttls(None, timeouts, Some(result)), - Err(err) => this.on_hostent_with_ttls(Some(err), timeouts, None), + // SAFETY: as above. + Ok(result) => unsafe { T::on_hostent_with_ttls(this, None, timeouts, Some(result)) }, + // SAFETY: as above. + Err(err) => unsafe { T::on_hostent_with_ttls(this, Some(err), timeouts, None) }, } } @@ -557,7 +580,14 @@ pub struct struct_nameinfo { } pub trait NameinfoHandler: Sized { - fn on_nameinfo(&mut self, status: Option, timeouts: i32, info: Option); + /// # Safety + /// `this` is the registered request, live and unaliased; it may be freed here. + unsafe fn on_nameinfo( + this: *mut Self, + status: Option, + timeouts: i32, + info: Option, + ); } impl struct_nameinfo { @@ -570,13 +600,21 @@ impl struct_nameinfo { node: *mut u8, service: *mut u8, ) { - // SAFETY: ctx was passed as *mut T to the ares call that registered this thunk. - let this = unsafe { bun_core::callback_ctx::(ctx) }; + let this = ctx.cast::(); if status != ARES_SUCCESS { - this.on_nameinfo(Error::get(status), timeouts, None); + // SAFETY: `ctx` is the `*mut T` registered by `Channel::get_name_info`; delivered once, unused after. + unsafe { T::on_nameinfo(this, Error::get(status), timeouts, None) }; return; } - this.on_nameinfo(None, timeouts, Some(struct_nameinfo { node, service })); + // SAFETY: as above. + unsafe { + T::on_nameinfo( + this, + None, + timeouts, + Some(struct_nameinfo { node, service }), + ) + }; } } @@ -625,7 +663,14 @@ pub struct AddrInfo { } pub trait AddrInfoHandler: Sized { - fn on_addr_info(&mut self, status: Option, timeouts: i32, results: *mut AddrInfo); + /// # Safety + /// `this` is the registered request, live and unaliased; it may be freed here. + unsafe fn on_addr_info( + this: *mut Self, + status: Option, + timeouts: i32, + results: *mut AddrInfo, + ); } impl AddrInfo { @@ -648,9 +693,8 @@ impl AddrInfo { timeouts: c_int, addr_info: *mut AddrInfo, ) { - // SAFETY: ctx was passed as *mut T to the ares call that registered this thunk. - let this = unsafe { bun_core::callback_ctx::(ctx) }; - this.on_addr_info(Error::get(status), timeouts, addr_info); + // SAFETY: `ctx` is the `*mut T` registered by `Channel::get_addr_info`; delivered once, unused after. + unsafe { T::on_addr_info(ctx.cast::(), Error::get(status), timeouts, addr_info) }; } /// FFI destroy — frees a c-ares-allocated addrinfo chain. @@ -795,12 +839,15 @@ impl Channel { } /// See c-ares `ares_getaddrinfo` documentation. - pub fn get_addr_info( + /// + /// # Safety + /// `ctx` must stay valid until `T`'s handler gets it (once; possibly before this returns). + pub unsafe fn get_addr_info( &mut self, host: &[u8], port: u16, hints: &[AddrInfo_hints], - ctx: &mut T, + ctx: *mut T, ) { let mut host_buf = [0u8; 1024]; let mut port_buf = [0u8; 21]; @@ -821,7 +868,7 @@ impl Channel { } else { ptr::null() }; - // SAFETY: c-ares FFI; host/port/hints are NUL-terminated stack buffers or null; ctx outlives the channel. + // SAFETY: c-ares FFI; host/port/hints are NUL-terminated stack buffers or null; ctx lives until its callback runs. unsafe { ares_getaddrinfo( self, @@ -829,33 +876,27 @@ impl Channel { port_ptr, hints_, AddrInfo::callback_wrapper::, - std::ptr::from_mut::(ctx).cast::(), + ctx.cast::(), ); } } - pub fn resolve(&mut self, name: &[u8], ctx: &mut T) { + /// # Safety + /// `ctx` must stay valid until `T`'s handler gets it (once; possibly before this returns). + pub unsafe fn resolve(&mut self, name: &[u8], ctx: *mut T) { if name.len() >= 1023 || bun_core::strings::contains_char(name, 0) || (name.is_empty() && !(T::LOOKUP_NAME == b"ns" || T::LOOKUP_NAME == b"soa")) { - // SAFETY: thunk handles ARES_EBADNAME path. - unsafe { - T::raw_callback( - std::ptr::from_mut::(ctx).cast::(), - ARES_EBADNAME, - 0, - ptr::null_mut(), - 0, - ) - }; + // SAFETY: this is ctx's one delivery; the thunk handles the ARES_EBADNAME path. + unsafe { T::raw_callback(ctx.cast::(), ARES_EBADNAME, 0, ptr::null_mut(), 0) }; return; } let mut name_buf = [0u8; 1024]; let name_ptr = copy_nul_terminated(&mut name_buf, name); - // SAFETY: c-ares FFI; name_ptr is a NUL-terminated stack buffer; ctx outlives the channel. + // SAFETY: c-ares FFI; name_ptr is a NUL-terminated stack buffer; ctx lives until its callback runs. unsafe { ares_query( self, @@ -863,12 +904,14 @@ impl Channel { NSClass::ns_c_in, T::NS_TYPE, Some(T::raw_callback), - std::ptr::from_mut::(ctx).cast::(), + ctx.cast::(), ); } } - pub fn get_host_by_addr(&mut self, ip_addr: &[u8], ctx: &mut T) { + /// # Safety + /// `ctx` must stay valid until `T`'s handler gets it (once; possibly before this returns). + pub unsafe fn get_host_by_addr(&mut self, ip_addr: &[u8], ctx: *mut T) { // "0000:0000:0000:0000:0000:ffff:192.168.100.228".length = 45 const BUF_SIZE: usize = 46; let mut addr_buf = [0u8; BUF_SIZE]; @@ -888,7 +931,7 @@ impl Channel { // SAFETY: c-ares FFI; addr_ptr is a NUL-terminated stack buffer, addr is 16-byte stack scratch. if unsafe { ares_inet_pton(AF::INET, addr_ptr, addr.as_mut_ptr().cast::()) } > 0 { - // SAFETY: c-ares FFI; addr holds a 4-byte in_addr written by ares_inet_pton; ctx outlives the channel. + // SAFETY: c-ares FFI; addr holds a 4-byte in_addr written by ares_inet_pton; ctx lives until its callback runs. unsafe { ares_gethostbyaddr( self, @@ -896,7 +939,7 @@ impl Channel { 4, AF::INET, Some(struct_hostent::host_callback_wrapper::), - std::ptr::from_mut::(ctx).cast::(), + ctx.cast::(), ); } return; @@ -905,7 +948,7 @@ impl Channel { ares_inet_pton(AF::INET6, addr_ptr, addr.as_mut_ptr().cast::()) } > 0 { - // SAFETY: c-ares FFI; addr holds a 16-byte in6_addr written by ares_inet_pton; ctx outlives the channel. + // SAFETY: c-ares FFI; addr holds a 16-byte in6_addr written by ares_inet_pton; ctx lives until its callback runs. unsafe { ares_gethostbyaddr( self, @@ -913,16 +956,16 @@ impl Channel { 16, AF::INET6, Some(struct_hostent::host_callback_wrapper::), - std::ptr::from_mut::(ctx).cast::(), + ctx.cast::(), ); } return; } } - // SAFETY: invoking the thunk directly with ENOTIMP. + // SAFETY: this is ctx's one delivery: ENOTIMP for an address neither family parsed. unsafe { struct_hostent::host_callback_wrapper::( - std::ptr::from_mut::(ctx).cast::(), + ctx.cast::(), ARES_ENOTIMP, 0, ptr::null_mut(), @@ -931,13 +974,16 @@ impl Channel { } /// https://c-ares.org/ares_getnameinfo.html - pub fn get_name_info(&mut self, sa: &mut sockaddr, ctx: &mut T) { + /// + /// # Safety + /// `ctx` must stay valid until `T`'s handler gets it (once; possibly before this returns). + pub unsafe fn get_name_info(&mut self, sa: &mut sockaddr, ctx: *mut T) { let salen: ares_socklen_t = if sa.sa_family == AF::INET as _ { core::mem::size_of::() as ares_socklen_t } else { core::mem::size_of::() as ares_socklen_t }; - // SAFETY: c-ares FFI; sa is a valid sockaddr of size `salen`; ctx outlives the channel. + // SAFETY: c-ares FFI; sa is a valid sockaddr of size `salen`; ctx lives until its callback runs. unsafe { ares_getnameinfo( self, @@ -947,7 +993,7 @@ impl Channel { // So, it requires setting the ARES_NI_NAMEREQD flag ARES_NI_NAMEREQD | ARES_NI_LOOKUPHOST | ARES_NI_LOOKUPSERVICE, Some(struct_nameinfo::callback_wrapper::), - std::ptr::from_mut::(ctx).cast::(), + ctx.cast::(), ); } } @@ -1195,7 +1241,9 @@ pub trait AresReply: Sized { /// Receiver for a parsed `R` reply (replaces the per-type `*Handler` traits). pub trait ReplyHandler: Sized { - fn on_reply(&mut self, status: Option, timeouts: i32, results: *mut R); + /// # Safety + /// `this` is the registered request, live and unaliased; it may be freed here. + unsafe fn on_reply(this: *mut Self, status: Option, timeouts: i32, results: *mut R); } /// Generic `ares_callback` thunk. Monomorphized per `(R, T)` to a concrete @@ -1208,20 +1256,22 @@ pub unsafe extern "C" fn ares_reply_callback>( buffer: *mut u8, buffer_length: c_int, ) { - // SAFETY: ctx was passed as *mut T to the ares call that registered this thunk. - let this = unsafe { bun_core::callback_ctx::(ctx) }; + let this = ctx.cast::(); if status != ARES_SUCCESS { - this.on_reply(Error::get(status), timeouts, ptr::null_mut()); + // SAFETY: `ctx` is the `*mut T` registered by `Channel::resolve`; delivered once, unused after. + unsafe { T::on_reply(this, Error::get(status), timeouts, ptr::null_mut()) }; return; } let mut start: *mut R = ptr::null_mut(); // SAFETY: c-ares FFI; pointers are valid stack/null per contract. let result = unsafe { R::parse(buffer, buffer_length, &raw mut start) }; if result != ARES_SUCCESS { - this.on_reply(Error::get(result), timeouts, ptr::null_mut()); + // SAFETY: as above. + unsafe { T::on_reply(this, Error::get(result), timeouts, ptr::null_mut()) }; return; } - this.on_reply(None, timeouts, start); + // SAFETY: as above. + unsafe { T::on_reply(this, None, timeouts, start) }; } #[repr(C)] @@ -1402,8 +1452,10 @@ impl Default for struct_any_reply { } pub trait AnyHandler: Sized { - fn on_any( - &mut self, + /// # Safety + /// `this` is the registered request, live and unaliased; it may be freed here. + unsafe fn on_any( + this: *mut Self, status: Option, timeouts: i32, results: Option>, @@ -1420,10 +1472,10 @@ impl struct_any_reply { buffer: *mut u8, buffer_length: c_int, ) { - // SAFETY: ctx was passed as *mut T to the ares call that registered this thunk. - let this = unsafe { bun_core::callback_ctx::(ctx) }; + let this = ctx.cast::(); if status != ARES_SUCCESS { - this.on_any(Error::get(status), timeouts, None); + // SAFETY: `ctx` is the `*mut T` registered by `Channel::resolve`; delivered once, unused after. + unsafe { T::on_any(this, Error::get(status), timeouts, None) }; return; } // SAFETY: c-ares guarantees `buffer` is non-null and readable for @@ -1432,8 +1484,10 @@ impl struct_any_reply { core::slice::from_raw_parts(buffer, usize::try_from(buffer_length).unwrap_or(0)) }; match Self::parse(buffer) { - Ok(reply) => this.on_any(None, timeouts, Some(reply)), - Err(err) => this.on_any(Some(err), timeouts, None), + // SAFETY: as above. + Ok(reply) => unsafe { T::on_any(this, None, timeouts, Some(reply)) }, + // SAFETY: as above. + Err(err) => unsafe { T::on_any(this, Some(err), timeouts, None) }, } } @@ -2020,3 +2074,265 @@ pub fn get_sockaddr(addr: &[u8], port: u16, sa: &mut sockaddr) -> c_int { pub struct in_addr { pub s_addr: u32, } + +// `bun run rust:miri -p bun_cares_sys`: a self-freeing request delivered through +// every thunk, and through the two `Channel` paths that complete without c-ares. +#[cfg(test)] +mod tests { + use super::*; + use std::cell::Cell; + use std::rc::Rc; + + struct Request { + completed: Rc>>, + } + + impl Request { + fn register() -> (*mut Request, Rc>>) { + let completed = Rc::new(Cell::new(None)); + let request = bun_core::heap::into_raw(Box::new(Request { + completed: Rc::clone(&completed), + })); + (request, completed) + } + + fn complete(this: *mut Self, status: Option) { + // SAFETY: `this` came from `register` and is delivered once. + let request = unsafe { bun_core::heap::take(this) }; + request.completed.set(Some( + status.expect("every test completes with an error status"), + )); + } + } + + impl HostentHandler for Request { + unsafe fn on_hostent( + this: *mut Self, + status: Option, + _timeouts: i32, + results: *mut struct_hostent, + ) { + assert!(results.is_null()); + Self::complete(this, status); + } + } + + impl HostentWithTtlsHandler for Request { + const PARSE: fn(&[u8]) -> Result, Error> = + hostent_with_ttls::parse_a; + + unsafe fn on_hostent_with_ttls( + this: *mut Self, + status: Option, + _timeouts: i32, + results: Option>, + ) { + assert!(results.is_none()); + Self::complete(this, status); + } + } + + impl NameinfoHandler for Request { + unsafe fn on_nameinfo( + this: *mut Self, + status: Option, + _timeouts: i32, + info: Option, + ) { + assert!(info.is_none()); + Self::complete(this, status); + } + } + + impl AddrInfoHandler for Request { + unsafe fn on_addr_info( + this: *mut Self, + status: Option, + _timeouts: i32, + results: *mut AddrInfo, + ) { + assert!(results.is_null()); + Self::complete(this, status); + } + } + + impl ReplyHandler for Request { + unsafe fn on_reply( + this: *mut Self, + status: Option, + _timeouts: i32, + results: *mut struct_ares_txt_reply, + ) { + assert!(results.is_null()); + Self::complete(this, status); + } + } + + impl AnyHandler for Request { + unsafe fn on_any( + this: *mut Self, + status: Option, + _timeouts: i32, + results: Option>, + ) { + assert!(results.is_none()); + Self::complete(this, status); + } + } + + // Wired like dns_jsc's CNAME query. + impl ResolveHandler for Request { + const LOOKUP_NAME: &'static [u8] = b"cname"; + const NS_TYPE: NSType = NSType::ns_t_cname; + unsafe extern "C" fn raw_callback( + ctx: *mut c_void, + status: c_int, + timeouts: c_int, + buffer: *mut u8, + buffer_length: c_int, + ) { + // SAFETY: forwarding the thunk's own arguments. + unsafe { + struct_hostent::callback_wrapper_cname::( + ctx, + status, + timeouts, + buffer, + buffer_length, + ) + } + } + } + + type ParseThunk = unsafe extern "C" fn(*mut c_void, c_int, c_int, *mut u8, c_int); + + fn deliver(thunk: ParseThunk, status: c_int) -> Error { + let (request, completed) = Request::register(); + // SAFETY: `request` is live and delivered once; an error status never reads the buffer. + unsafe { thunk(request.cast::(), status, 0, ptr::null_mut(), 0) }; + completed.get().expect("thunk completed the request") + } + + #[test] + fn hostent_thunks_free_the_request() { + assert_eq!( + deliver( + struct_hostent::callback_wrapper_cname::, + ARES_ENOTFOUND + ), + Error::ENOTFOUND + ); + assert_eq!( + deliver( + struct_hostent::callback_wrapper_ns::, + ARES_ESERVFAIL + ), + Error::ESERVFAIL + ); + assert_eq!( + deliver( + struct_hostent::callback_wrapper_ptr::, + ARES_EREFUSED + ), + Error::EREFUSED + ); + + let (request, completed) = Request::register(); + // SAFETY: as in `deliver`. + unsafe { + struct_hostent::host_callback_wrapper::( + request.cast::(), + ARES_ETIMEOUT, + 0, + ptr::null_mut(), + ) + }; + assert_eq!(completed.get(), Some(Error::ETIMEOUT)); + } + + #[test] + fn hostent_with_ttls_thunk_frees_the_request() { + assert_eq!( + deliver( + hostent_with_ttls::callback_wrapper::, + ARES_ECONNREFUSED + ), + Error::ECONNREFUSED + ); + } + + #[test] + fn reply_thunk_frees_the_request() { + assert_eq!( + deliver( + ares_reply_callback::, + ARES_ETIMEOUT + ), + Error::ETIMEOUT + ); + } + + #[test] + fn any_thunk_frees_the_request() { + assert_eq!( + deliver( + struct_any_reply::callback_wrapper::, + ARES_ECANCELLED + ), + Error::ECANCELLED + ); + } + + #[test] + fn nameinfo_thunk_frees_the_request() { + let (request, completed) = Request::register(); + // SAFETY: as in `deliver`. + unsafe { + struct_nameinfo::callback_wrapper::( + request.cast::(), + ARES_ENOTFOUND, + 0, + ptr::null_mut(), + ptr::null_mut(), + ) + }; + assert_eq!(completed.get(), Some(Error::ENOTFOUND)); + } + + #[test] + fn addr_info_thunk_frees_the_request() { + let (request, completed) = Request::register(); + // SAFETY: as in `deliver`. + unsafe { + AddrInfo::callback_wrapper::( + request.cast::(), + ARES_EDESTRUCTION, + 0, + ptr::null_mut(), + ) + }; + assert_eq!(completed.get(), Some(Error::EDESTRUCTION)); + } + + // `Channel` is a ZST (asserted at its definition), so no c-ares channel is needed. + fn channel() -> &'static mut Channel { + Channel::opaque_mut(ptr::NonNull::::dangling().as_ptr()) + } + + #[test] + fn resolve_completes_an_overlong_name_before_returning() { + let (request, completed) = Request::register(); + let name = [b'a'; 1023]; + // SAFETY: `request` is live; a 1023-byte name is completed before `ares_query`. + unsafe { channel().resolve::(&name, request) }; + assert_eq!(completed.get(), Some(Error::EBADNAME)); + } + + #[test] + fn get_host_by_addr_completes_an_unparsable_address_before_returning() { + let (request, completed) = Request::register(); + // SAFETY: `request` is live; an empty address is completed before any c-ares call. + unsafe { channel().get_host_by_addr::(b"", request) }; + assert_eq!(completed.get(), Some(Error::ENOTIMP)); + } +} diff --git a/src/runtime/dns_jsc/dns.rs b/src/runtime/dns_jsc/dns.rs index d6a9b5e20c41..97b9ec2cf090 100644 --- a/src/runtime/dns_jsc/dns.rs +++ b/src/runtime/dns_jsc/dns.rs @@ -679,8 +679,8 @@ impl GetHostByAddrInfoRequest { } impl c_ares::HostentHandler for GetHostByAddrInfoRequest { - fn on_hostent( - &mut self, + unsafe fn on_hostent( + this: *mut Self, status: Option, timeouts: i32, results: *mut c_ares::struct_hostent, @@ -690,7 +690,7 @@ impl c_ares::HostentHandler for GetHostByAddrInfoRequest { } else { Some(results) }; - Self::on_cares_complete(std::ptr::from_mut::(self), status, timeouts, result); + Self::on_cares_complete(this, status, timeouts, result); } } @@ -931,21 +931,13 @@ impl GetNameInfoRequest { impl c_ares::NameinfoHandler for GetNameInfoRequest { #[inline] - fn on_nameinfo( - &mut self, + unsafe fn on_nameinfo( + this: *mut Self, status: Option, timeouts: i32, info: Option, ) { - // SAFETY: `self` is the `heap::alloc`'d heap request registered with - // c-ares; `on_cares_complete` consumes it (heap::take) on every path. - // The c-ares callback wrapper does not touch `self` after this returns. - GetNameInfoRequest::on_cares_complete( - std::ptr::from_mut::(self), - status, - timeouts, - info, - ); + Self::on_cares_complete(this, status, timeouts, info); } } @@ -1464,8 +1456,8 @@ impl GetAddrInfoRequest { // Wires `GetAddrInfoRequest` into `Channel::get_addr_info`. impl c_ares::AddrInfoHandler for GetAddrInfoRequest { - fn on_addr_info( - &mut self, + unsafe fn on_addr_info( + this: *mut Self, status: Option, timeouts: i32, results: *mut c_ares::AddrInfo, @@ -1475,7 +1467,7 @@ impl c_ares::AddrInfoHandler for GetAddrInfoRequest { } else { Some(results) }; - Self::on_cares_complete(std::ptr::from_mut::(self), status, timeouts, result); + Self::on_cares_complete(this, status, timeouts, result); } } @@ -3348,8 +3340,8 @@ macro_rules! impl_cares_record_type { } // Generic reply handler — forwards to `on_cares_complete`. impl c_ares::ReplyHandler<$ty> for ResolveInfoRequest<$ty> { - fn on_reply( - &mut self, + unsafe fn on_reply( + this: *mut Self, status: Option, timeouts: i32, results: *mut $ty, @@ -3357,7 +3349,7 @@ macro_rules! impl_cares_record_type { // SAFETY: `ares_reply_callback` hands over the `ares_parse_*_reply` // allocation, which `destroy` frees. let result = NonNull::new(results).map(|reply| unsafe { OwnedReply::adopt(reply) }); - Self::on_cares_complete(core::ptr::from_mut(self), status, timeouts, result); + Self::on_cares_complete(this, status, timeouts, result); } } }; @@ -3436,8 +3428,8 @@ impl CAresRecordType for c_ares::struct_any_reply { } } impl c_ares::AnyHandler for ResolveInfoRequest { - fn on_any( - &mut self, + unsafe fn on_any( + this: *mut Self, status: Option, timeouts: i32, results: Option>, @@ -3445,7 +3437,7 @@ impl c_ares::AnyHandler for ResolveInfoRequest { // SAFETY: `destroy` re-boxes the allocation released here. let result = results.map(|reply| unsafe { OwnedReply::adopt(bun_core::heap::into_raw_nn(reply)) }); - Self::on_cares_complete(std::ptr::from_mut::(self), status, timeouts, result); + Self::on_cares_complete(this, status, timeouts, result); } } @@ -3474,8 +3466,8 @@ macro_rules! hostent_newtype { } } impl c_ares::HostentHandler for ResolveInfoRequest<$name> { - fn on_hostent( - &mut self, + unsafe fn on_hostent( + this: *mut Self, status: Option, timeouts: i32, results: *mut c_ares::struct_hostent, @@ -3486,7 +3478,7 @@ macro_rules! hostent_newtype { // lends to `GetHostByAddrInfoRequest`); `#[repr(transparent)]` makes the // cast sound. let result = hostent.map(|reply| unsafe { OwnedReply::adopt(reply) }); - Self::on_cares_complete(core::ptr::from_mut(self), status, timeouts, result); + Self::on_cares_complete(this, status, timeouts, result); } } }; @@ -3528,8 +3520,8 @@ macro_rules! hostent_ttls_newtype { impl c_ares::HostentWithTtlsHandler for ResolveInfoRequest<$name> { const PARSE: fn(&[u8]) -> Result, c_ares::Error> = c_ares::hostent_with_ttls::$parse; - fn on_hostent_with_ttls( - &mut self, + unsafe fn on_hostent_with_ttls( + this: *mut Self, status: Option, timeouts: i32, results: Option>, @@ -3539,7 +3531,7 @@ macro_rules! hostent_ttls_newtype { let result = results.map(|reply| unsafe { OwnedReply::adopt(bun_core::heap::into_raw_nn(reply).cast::<$name>()) }); - Self::on_cares_complete(core::ptr::from_mut(self), status, timeouts, result); + Self::on_cares_complete(this, status, timeouts, result); } } }; @@ -5118,11 +5110,9 @@ impl Resolver { // SAFETY: `request` just heap-allocated in `init()`; `tail` points at its inline `head`. let promise = unsafe { (*(*request).tail).promise.value() }; - // SAFETY: `request` is the heap-allocated GetHostByAddrInfoRequest; channel - // stores it as the c-ares ctx and calls back via HostentHandler::on_hostent. - unsafe { - (*channel).get_host_by_addr(ip, &mut *request); - } + // SAFETY: `channel` is the live c-ares channel owned by `self`; the heap + // `request` lives until its handler consumes it (possibly during this call). + unsafe { (*channel).get_host_by_addr(ip, request) }; // SAFETY: `bun_vm()` returns the live VM back-ptr. self.request_sent(global_this.bun_vm()); @@ -5442,11 +5432,9 @@ impl Resolver { // SAFETY: `request` just heap-allocated in `init()`; `tail` points at its inline `head`. let promise = unsafe { (*(*request).tail).promise.value() }; - // SAFETY: `channel` is the live c-ares channel owned by `self`; `request` - // is the freshly heap-allocated ResolveInfoRequest. c-ares stores the ctx - // pointer and calls `T::RAW_CALLBACK` (→ `on_cares_complete`) which - // consumes the request, so the `&mut` borrow is not held past this call. - unsafe { (*channel).resolve(name, &mut *request) }; + // SAFETY: `channel` is the live c-ares channel owned by `self`; the heap + // `request` lives until `on_cares_complete` consumes it (possibly during this call). + unsafe { (*channel).resolve(name, request) }; // SAFETY: bun_vm() returns a live VM pointer for the duration of the call. self.request_sent(global_this.bun_vm()); @@ -5497,12 +5485,9 @@ impl Resolver { // SAFETY: `request` just heap-allocated in `init()`; `tail` points at its inline `head`. let promise = unsafe { (*(*request).tail).promise.value() }; - // SAFETY: `channel` is the live c-ares channel owned by `self`; `request` - // is the freshly heap-allocated GetAddrInfoRequest. c-ares stores the ctx - // pointer and calls `AddrInfo::callback_wrapper::` - // (→ `on_cares_complete`) which consumes the request, so the `&mut` - // borrow is not held past this call. - unsafe { (*channel).get_addr_info(&query.name, query.port, &hints_buf, &mut *request) }; + // SAFETY: `channel` is the live c-ares channel owned by `self`; the heap + // `request` lives until `on_cares_complete` consumes it (possibly during this call). + unsafe { (*channel).get_addr_info(&query.name, query.port, &hints_buf, request) }; // SAFETY: bun_vm() returns a live VM pointer for the duration of the call. self.request_sent(global_this.bun_vm()); @@ -6022,15 +6007,15 @@ impl Resolver { // SAFETY: `request` just heap-allocated in `init()`; `tail` points at its inline `head`. let promise = unsafe { (*(*request).tail).promise.value() }; - // SAFETY: `channel` is the live c-ares channel; `sa` is a valid - // sockaddr_storage reborrowed as sockaddr; `request` was just - // `heap::alloc`'d and is owned by c-ares until the callback fires. + // SAFETY: `channel` is the live c-ares channel; `sa` is a valid sockaddr_storage + // reborrowed as sockaddr; the heap `request` lives until `on_cares_complete` + // consumes it (possibly during this call). unsafe { (*channel).get_name_info( // See `get_sockaddr` call above — inferred `sockaddr` type is // platform-dependent and unnameable on Windows from this crate. &mut *(&raw mut sa).cast(), - &mut *request, + request, ); } diff --git a/test/internal/source-lints/self-receiver-cares-complete.test.ts b/test/internal/source-lints/self-receiver-cares-complete.test.ts new file mode 100644 index 000000000000..1b3ed195cd33 --- /dev/null +++ b/test/internal/source-lints/self-receiver-cares-complete.test.ts @@ -0,0 +1,113 @@ +import { file } from "bun"; +import { expect, test } from "bun:test"; +import { realpathSync } from "fs"; +import path from "path"; +import { globAllSources } from "../../../scripts/glob-sources.ts"; + +// `on_cares_complete(this: *mut Self, ..)` (src/runtime/dns_jsc/dns.rs, one per +// c-ares request type) reclaims the request's allocation on every path, so a +// `&self` / `&mut self` method must not hand it its own receiver: +// `Self::on_cares_complete(ptr::from_mut(self), ..)` frees the allocation +// while the receiver argument is still live, which is UB under the aliasing +// models whether or not `self` is used again (Miri: "the strongly protected +// tag disallows deallocations"). The c-ares handler traits hand the request +// over as `this: *mut Self` for exactly this reason (the `#[cfg(test)]` module +// at the end of src/cares_sys/c_ares.rs drives that under Miri), so the impls +// forward `this`; this lint keeps any other spelling out. +// +// This is the `on_cares_complete` case of the hazard self-receiver-reclaim +// .test.ts bans for the reclaim primitives themselves (same receiver spellings, +// plus bare `self`, which coerces to `*mut Self` at a raw-pointer parameter); +// a lint keyed on consuming-helper names generally is where this entry +// belongs once one exists. +// +// Sibling guards: self-receiver-reclaim.test.ts, fn-long-mut-reborrow.test.ts. + +const root = path.resolve(import.meta.dir, "..", "..", ".."); +const rustSources = globAllSources().rust.filter(p => p.endsWith(".rs")); + +// Only scan files tracked in HEAD (a `git stash` round-trip can leave stray +// `.rs` files in the working tree; CI runs on a clean checkout). Same guard as +// dead-code-escapes.test.ts. +const tracked: Set | null = (() => { + const r = Bun.spawnSync({ + cmd: ["git", "-C", root, "ls-tree", "-r", "--name-only", "-z", "HEAD"], + stdout: "pipe", + stderr: "ignore", + }); + if (!r.success) return null; + return new Set(r.stdout.toString().split("\0").filter(Boolean)); +})(); + +// The receiver as the first argument: bare `self`, or any spelling of its +// address. `(?!\s*\.)` keeps a field's address (`&raw mut *self.head`) out. +const SELF_AS_POINTER = [ + String.raw`self\s*[,)]`, + String.raw`(?:[\w:]+::)?from_(?:mut|ref)(?:::<[^>]*>)?\(\s*self\s*\)`, + String.raw`(?:[\w:]+::)?NonNull::from\(\s*self\s*\)`, + String.raw`self\s+as\s+\*(?:mut|const)\b`, + String.raw`&\s*(?:raw\s+(?:mut|const)|mut)\s+\*\s*self\b(?!\s*\.)`, + String.raw`(?:[\w:]+::)?addr_of(?:_mut)?!\s*\(\s*\*\s*self\s*\)`, +].join("|"); + +// `\s*` after the paren so a rustfmt-wrapped argument list still matches. +const BANNED = new RegExp(String.raw`\bon_cares_complete\s*\(\s*(?:${SELF_AS_POINTER})`, "g"); + +function offendersIn(source: string, content: string): string[] { + // Strip full-line comments so prose mentions don't count. `[ \t]*`, not + // `\s*`: `\s` crosses newlines and would shift the reported line numbers. + const stripped = content.replace(/^[ \t]*\/\/.*$/gm, ""); + return [...stripped.matchAll(BANNED)].map( + m => `${source}:${stripped.slice(0, m.index).split("\n").length}: ${m[0].replace(/\s+/g, " ")}`, + ); +} + +const offenders: string[] = []; +let scanned = 0; +for (const abs of rustSources) { + const source = path.relative(root, abs).replaceAll(path.sep, "/"); + // `src/cli` is a symlink into `src/runtime/cli`; count each file once under + // its canonical path. + if (path.relative(root, realpathSync(abs)).replaceAll(path.sep, "/") !== source) continue; + if (tracked !== null && !tracked.has(source)) continue; + scanned++; + offenders.push(...offendersIn(source, await file(abs).text())); +} + +test("scans a non-empty set of tracked Rust sources", () => { + // Guards against the filters above over-firing and leaving nothing to scan, + // which would make the ban below pass vacuously. + expect(scanned).toBeGreaterThan(0); +}); + +test("the pattern recognizes the spellings it claims to", () => { + const banned = [ + // The dns.rs impls as they were, one-line and rustfmt-wrapped. + "Self::on_cares_complete(std::ptr::from_mut::(self), status, timeouts, result);", + "Self::on_cares_complete(core::ptr::from_mut(self), status, timeouts, result);", + "GetNameInfoRequest::on_cares_complete(\n std::ptr::from_mut::(self),\n status,\n timeouts,\n info,\n);", + // Other spellings of the receiver. + "Self::on_cares_complete(self, status, timeouts, result);", + "Self::on_cares_complete(self as *mut Self, status, timeouts, result);", + "Self::on_cares_complete(&raw mut *self, status, timeouts, result);", + "Self::on_cares_complete(core::ptr::addr_of_mut!(*self), status, timeouts, result);", + "Self::on_cares_complete(NonNull::from(self), status, timeouts, result);", + ]; + const allowed = [ + // The pointer comes in as a parameter. + "Self::on_cares_complete(this, status, timeouts, result);", + "GetNameInfoRequest::on_cares_complete(\n this,\n status,\n timeouts,\n info,\n);", + // A request the receiver owns, and the definition itself. + "Request::on_cares_complete(self.request, status, timeouts, result);", + "Request::on_cares_complete(&raw mut *self.request, status, timeouts, result);", + "fn on_cares_complete(\n this: *mut Self,\n err_: Option,\n) {", + // Prose. + "// forwards to `on_cares_complete(self)` in spirit", + ]; + expect(banned.filter(s => offendersIn("x.rs", s).length === 0)).toEqual([]); + expect(allowed.flatMap(s => offendersIn("x.rs", s))).toEqual([]); +}); + +test("no method hands its own receiver to on_cares_complete", () => { + expect(offenders).toEqual([]); +});