commit e8ae8de3704554192a72a39a0789fc38f333d03c parent ee092f72a6c842eda599d93b85baec63c5a99248 Author: Gabriele Svelto <gsvelto@mozilla.com> Date: Tue, 14 Oct 2025 13:02:48 +0000 Bug 1982016 - Improve ownership of the ancillary data types used by the crash helper code r=afranchuk Differential Revision: https://phabricator.services.mozilla.com/D267247 Diffstat:
15 files changed, 218 insertions(+), 239 deletions(-)
diff --git a/toolkit/crashreporter/crash_helper_client/src/lib.rs b/toolkit/crashreporter/crash_helper_client/src/lib.rs @@ -5,8 +5,8 @@ use anyhow::{bail, Result}; use crash_helper_common::{ messages::{self}, - AncillaryData, BreakpadString, IPCClientChannel, IPCConnector, ProcessHandle, - INVALID_ANCILLARY_DATA, + AncillaryData, BreakpadString, IPCClientChannel, IPCConnector, IntoRawAncillaryData, + ProcessHandle, RawAncillaryData, INVALID_ANCILLARY_DATA, }; #[cfg(any(target_os = "android", target_os = "linux"))] use minidump_writer::minidump_writer::{AuxvType, DirectAuxvDumpInfo}; @@ -34,13 +34,12 @@ mod platform; pub struct CrashHelperClient { connector: IPCConnector, spawner_thread: Option<JoinHandle<Result<ProcessHandle>>>, - helper_process: Option<ProcessHandle>, } impl CrashHelperClient { fn set_crash_report_path(&mut self, path: OsString) -> Result<()> { let message = messages::SetCrashReportPath::new(path); - self.connector.send_message(&message)?; + self.connector.send_message(message)?; Ok(()) } @@ -57,48 +56,32 @@ impl CrashHelperClient { bail!("The crash helper process failed to launch"); }; - self.helper_process = Some(process_handle); + self.connector.set_process(process_handle); } - if self.helper_process.is_none() { - bail!("The crash helper process is not available"); - }; - - // The endpoint will be sent to the crash helper process (and essentially dup'd on unix), - // so we have to retain ownership of the server_endpoint using `as_ancillary()` until the - // message is sent. - let Ok(ancillary_data) = server_endpoint.as_ancillary(&self.helper_process) else { - bail!("Could not convert the server IPC endpoint"); - }; - - let message = messages::RegisterChildProcess::new(ancillary_data); - self.connector.send_message(&message)?; - // We use `into_ancillary()` because the returned fd will stay in this process (so we don't - // want to close it). - let Ok(ancillary_data) = client_endpoint.into_ancillary(/* dst_process */ &None) else { - bail!("Could not convert the local IPC endpoint"); - }; - - Ok(ancillary_data) + let message = messages::RegisterChildProcess::new(server_endpoint.into_ancillary()); + self.connector.send_message(message)?; + + Ok(client_endpoint.into_ancillary()) } #[cfg(any(target_os = "android", target_os = "linux"))] fn register_auxv_info(&mut self, pid: Pid, auxv_info: DirectAuxvDumpInfo) -> Result<()> { let message = messages::RegisterAuxvInfo::new(pid, auxv_info); - self.connector.send_message(&message)?; + self.connector.send_message(message)?; Ok(()) } #[cfg(any(target_os = "android", target_os = "linux"))] fn unregister_auxv_info(&mut self, pid: Pid) -> Result<()> { let message = messages::UnregisterAuxvInfo::new(pid); - self.connector.send_message(&message)?; + self.connector.send_message(message)?; Ok(()) } fn transfer_crash_report(&mut self, pid: Pid) -> Result<CrashReport> { let message = messages::TransferMinidump::new(pid); - self.connector.send_message(&message)?; + self.connector.send_message(message)?; // HACK: Workaround for a macOS-specific bug #[cfg(target_os = "macos")] @@ -232,10 +215,10 @@ pub unsafe extern "C" fn set_crash_report_path( #[no_mangle] pub unsafe extern "C" fn register_child_ipc_channel( client: *mut CrashHelperClient, -) -> AncillaryData { +) -> RawAncillaryData { let client = client.as_mut().unwrap(); if let Ok(client_endpoint) = client.register_child_process() { - client_endpoint + client_endpoint.into_raw() } else { INVALID_ANCILLARY_DATA } @@ -311,7 +294,7 @@ pub unsafe fn report_external_exception( let server_addr = crash_helper_common::server_addr(main_process_pid); if let Ok(connector) = IPCConnector::connect(&server_addr) { let _ = connector - .send_message(&message) + .send_message(message) .and_then(|_| connector.recv_reply::<messages::WindowsErrorReportingMinidumpReply>()); } } @@ -389,7 +372,7 @@ pub unsafe extern "C" fn unregister_child_auxv_info( // signal/exception-safe. We will access this endpoint only from within the // exception handler with bare syscalls so we can leave the `IPCConnector` // object behind. -static CHILD_IPC_ENDPOINT: OnceLock<Box<AncillaryData>> = OnceLock::new(); +static CHILD_IPC_ENDPOINT: OnceLock<Box<RawAncillaryData>> = OnceLock::new(); static RENDEZVOUS_FAILED: AtomicBool = AtomicBool::new(false); /// Let a client rendez-vous with the crash helper process. This step ensures @@ -402,8 +385,8 @@ static RENDEZVOUS_FAILED: AtomicBool = AtomicBool::new(false); /// a valid pipe handle (on Windows) or a valid file descriptor (on all other /// platforms). #[no_mangle] -pub unsafe extern "C" fn crash_helper_rendezvous(client_endpoint: AncillaryData) { - let Ok(connector) = IPCConnector::from_ancillary(client_endpoint) else { +pub unsafe extern "C" fn crash_helper_rendezvous(client_endpoint: RawAncillaryData) { + let Ok(connector) = IPCConnector::from_raw_ancillary(client_endpoint) else { RENDEZVOUS_FAILED.store(true, Ordering::Relaxed); return; }; @@ -413,7 +396,7 @@ pub unsafe extern "C" fn crash_helper_rendezvous(client_endpoint: AncillaryData) CrashHelperClient::prepare_for_minidump(message.crash_helper_pid); assert!( CHILD_IPC_ENDPOINT - .set(Box::new(connector.into_ancillary(&None).unwrap())) + .set(Box::new(connector.into_raw_ancillary())) .is_ok(), "The crash_helper_rendezvous() function must only be called once" ); diff --git a/toolkit/crashreporter/crash_helper_client/src/platform/android.rs b/toolkit/crashreporter/crash_helper_client/src/platform/android.rs @@ -3,21 +3,20 @@ * You can obtain one at http://mozilla.org/MPL/2.0/. */ use anyhow::Result; -use crash_helper_common::{IPCConnector, Pid}; -use std::os::fd::{FromRawFd, OwnedFd, RawFd}; +use crash_helper_common::{IPCConnector, Pid, RawAncillaryData}; use crate::CrashHelperClient; impl CrashHelperClient { - pub(crate) fn new(server_socket: RawFd) -> Result<CrashHelperClient> { + pub(crate) fn new(server_socket: RawAncillaryData) -> Result<CrashHelperClient> { // SAFETY: The `server_socket` passed in from the application is valid - let server_socket = unsafe { OwnedFd::from_raw_fd(server_socket) }; - let connector = IPCConnector::from_fd(server_socket)?; + let connector = unsafe { + IPCConnector::from_raw_ancillary(server_socket)? + }; Ok(CrashHelperClient { connector, spawner_thread: None, - helper_process: Some(()), }) } diff --git a/toolkit/crashreporter/crash_helper_client/src/platform/unix.rs b/toolkit/crashreporter/crash_helper_client/src/platform/unix.rs @@ -34,7 +34,6 @@ impl CrashHelperClient { Ok(CrashHelperClient { connector: client_endpoint, spawner_thread: None, - helper_process: Some(()), }) } diff --git a/toolkit/crashreporter/crash_helper_client/src/platform/windows.rs b/toolkit/crashreporter/crash_helper_client/src/platform/windows.rs @@ -52,7 +52,6 @@ impl CrashHelperClient { Ok(CrashHelperClient { connector: client_endpoint, spawner_thread: Some(spawner_thread), - helper_process: None, }) } diff --git a/toolkit/crashreporter/crash_helper_common/src/errors.rs b/toolkit/crashreporter/crash_helper_common/src/errors.rs @@ -40,6 +40,9 @@ pub enum IPCError { ParseError, #[error("Failed to duplicate clone handle")] CloneHandleFailed(#[source] std::io::Error), + #[cfg(target_os = "windows")] + #[error("Missing destination process handle")] + MissingProcessHandle, } #[derive(Debug, Error)] diff --git a/toolkit/crashreporter/crash_helper_common/src/ipc_connector.rs b/toolkit/crashreporter/crash_helper_common/src/ipc_connector.rs @@ -15,7 +15,7 @@ pub enum IPCEvent { *****************************************************************************/ #[cfg(target_os = "windows")] -pub use windows::{AncillaryData, IPCConnector, INVALID_ANCILLARY_DATA}; +pub use windows::{AncillaryData, IPCConnector, RawAncillaryData, INVALID_ANCILLARY_DATA}; #[cfg(target_os = "windows")] pub(crate) mod windows; @@ -25,7 +25,7 @@ pub(crate) mod windows; *****************************************************************************/ #[cfg(any(target_os = "android", target_os = "linux", target_os = "macos"))] -pub use unix::{AncillaryData, IPCConnector, INVALID_ANCILLARY_DATA}; +pub use unix::{AncillaryData, IPCConnector, RawAncillaryData, INVALID_ANCILLARY_DATA}; #[cfg(any(target_os = "android", target_os = "linux", target_os = "macos"))] pub(crate) mod unix; diff --git a/toolkit/crashreporter/crash_helper_common/src/ipc_connector/unix.rs b/toolkit/crashreporter/crash_helper_common/src/ipc_connector/unix.rs @@ -6,7 +6,7 @@ use crate::platform::linux::{set_socket_cloexec, set_socket_default_flags}; #[cfg(target_os = "macos")] use crate::platform::macos::{set_socket_cloexec, set_socket_default_flags}; -use crate::{ignore_eintr, ProcessHandle, IO_TIMEOUT}; +use crate::{ignore_eintr, IntoRawAncillaryData, ProcessHandle, IO_TIMEOUT}; use nix::{ cmsg_space, @@ -26,10 +26,17 @@ use crate::{ messages::{self, Message}, }; -pub type AncillaryData = RawFd; +pub type RawAncillaryData = RawFd; +pub type AncillaryData = OwnedFd; + +impl IntoRawAncillaryData for AncillaryData { + fn into_raw(self) -> RawAncillaryData { + self.into_raw_fd() + } +} // This must match `kInvalidHandle` in `mfbt/UniquePtrExt.h` -pub const INVALID_ANCILLARY_DATA: AncillaryData = -1; +pub const INVALID_ANCILLARY_DATA: RawAncillaryData = -1; pub struct IPCConnector { socket: OwnedFd, @@ -39,7 +46,7 @@ impl IPCConnector { /// Create a new connector from an already connected socket. The /// `FD_CLOEXEC` flag will be set on the underlying socket and thus it /// will not be possible to inerhit this connector in a child process. - pub fn from_fd(socket: OwnedFd) -> Result<IPCConnector, IPCError> { + pub(crate) fn from_fd(socket: OwnedFd) -> Result<IPCConnector, IPCError> { let connector = IPCConnector::from_fd_inheritable(socket)?; set_socket_cloexec(connector.socket.as_fd()).map_err(IPCError::System)?; Ok(connector) @@ -48,15 +55,29 @@ impl IPCConnector { /// Create a new connector from an already connected socket. The /// `FD_CLOEXEC` flag will not be set on the underlying socket and thus it /// will be possible to inherit this connector in a child process. - pub fn from_fd_inheritable(socket: OwnedFd) -> Result<IPCConnector, IPCError> { + pub(crate) fn from_fd_inheritable(socket: OwnedFd) -> Result<IPCConnector, IPCError> { set_socket_default_flags(socket.as_fd()).map_err(IPCError::System)?; Ok(IPCConnector { socket }) } - pub fn from_ancillary(ancillary_data: AncillaryData) -> Result<IPCConnector, IPCError> { - IPCConnector::from_fd(unsafe { OwnedFd::from_raw_fd(ancillary_data) }) + pub fn from_ancillary(socket: AncillaryData) -> Result<IPCConnector, IPCError> { + IPCConnector::from_fd(socket) } + /// Create a connector from a raw file descriptor. + /// + /// # Safety + /// + /// The `ancillary_data` argument must be an open file descriptor + /// representing a connected Unix socket. + pub unsafe fn from_raw_ancillary( + ancillary_data: RawAncillaryData, + ) -> Result<IPCConnector, IPCError> { + IPCConnector::from_fd(OwnedFd::from_raw_fd(ancillary_data)) + } + + pub fn set_process(&mut self, _process: ProcessHandle) {} + /// Serialize this connector into a string that can be passed on the /// command-line to a child process. This only works for newly /// created connectors because they are explicitly created as inheritable. @@ -73,24 +94,12 @@ impl IPCConnector { Ok(IPCConnector { socket }) } - fn raw_fd(&self) -> RawFd { - self.socket.as_raw_fd() + pub fn into_ancillary(self) -> AncillaryData { + self.socket } - pub fn into_ancillary( - self, - _dst_process: &Option<ProcessHandle>, - ) -> Result<AncillaryData, IPCError> { - Ok(self.socket.into_raw_fd()) - } - - /// Like into_ancillary, but the IPCConnector retains ownership of the file descriptor (so be - /// sure to use the result during the lifetime of the IPCConnector). - pub fn as_ancillary( - &self, - _dst_process: &Option<ProcessHandle>, - ) -> Result<AncillaryData, IPCError> { - Ok(self.raw_fd()) + pub fn into_raw_ancillary(self) -> RawAncillaryData { + self.socket.into_raw() } pub fn as_raw_ref(&self) -> BorrowedFd<'_> { @@ -110,10 +119,14 @@ impl IPCConnector { } } - pub fn send_message(&self, message: &dyn Message) -> Result<(), IPCError> { + pub fn send_message<T>(&self, message: T) -> Result<(), IPCError> + where + T: Message, + { self.send(&message.header(), None) .map_err(IPCError::TransmissionFailure)?; - self.send(&message.payload(), message.ancillary_payload()) + let (payload, ancillary_data) = message.into_payload(); + self.send(&payload, ancillary_data) .map_err(IPCError::TransmissionFailure) } @@ -131,13 +144,13 @@ impl IPCConnector { T::decode(&data, None).map_err(IPCError::from) } - fn send_nonblock(&self, buff: &[u8], fd: Option<AncillaryData>) -> Result<(), Errno> { + fn send_nonblock(&self, buff: &[u8], fd: &Option<AncillaryData>) -> Result<(), Errno> { let iov = [IoSlice::new(buff)]; - let scm_fds: Vec<i32> = fd.map_or(vec![], |fd| vec![fd]); + let scm_fds: Vec<i32> = fd.iter().map(|fd| fd.as_raw_fd()).collect(); let scm = ControlMessage::ScmRights(&scm_fds); let res = ignore_eintr!(sendmsg::<()>( - self.raw_fd(), + self.socket.as_raw_fd(), &iov, &[scm], MsgFlags::empty(), @@ -159,13 +172,13 @@ impl IPCConnector { } fn send(&self, buff: &[u8], fd: Option<AncillaryData>) -> Result<(), Errno> { - let res = self.send_nonblock(buff, fd); + let res = self.send_nonblock(buff, &fd); match res { Err(_code @ Errno::EAGAIN) => { // If the socket was not ready to send data wait for it to // become unblocked then retry sending just once. self.poll(PollFlags::POLLOUT)?; - self.send_nonblock(buff, fd) + self.send_nonblock(buff, &fd) } _ => res, } @@ -187,7 +200,7 @@ impl IPCConnector { let mut iov = [IoSliceMut::new(&mut buff)]; let res = ignore_eintr!(recvmsg::<()>( - self.raw_fd(), + self.socket.as_raw_fd(), &mut iov, Some(&mut cmsg_buffer), MsgFlags::empty(), @@ -203,7 +216,7 @@ impl IPCConnector { let res = match res { #[cfg(target_os = "macos")] Err(_code @ Errno::ENOMEM) => ignore_eintr!(recvmsg::<()>( - self.raw_fd(), + self.socket.as_raw_fd(), &mut iov, Some(&mut cmsg_buffer), MsgFlags::empty(), @@ -214,7 +227,7 @@ impl IPCConnector { let fd = if let Some(cmsg) = res.cmsgs()?.next() { if let ControlMessageOwned::ScmRights(fds) = cmsg { - fds.first().copied() + fds.first().map(|&fd| unsafe { OwnedFd::from_raw_fd(fd) }) } else { return Err(Errno::EBADMSG); } diff --git a/toolkit/crashreporter/crash_helper_common/src/ipc_connector/windows.rs b/toolkit/crashreporter/crash_helper_common/src/ipc_connector/windows.rs @@ -6,11 +6,12 @@ use crate::{ errors::IPCError, messages::{self, Message}, platform::windows::{create_manual_reset_event, get_last_error, OverlappedOperation}, - ProcessHandle, IO_TIMEOUT, + IntoRawAncillaryData, IO_TIMEOUT, }; use std::{ - ffi::{c_void, CStr, OsString}, + ffi::{CStr, OsString}, + io::Error, os::windows::io::{AsRawHandle, FromRawHandle, IntoRawHandle, OwnedHandle, RawHandle}, ptr::null_mut, str::FromStr, @@ -33,17 +34,24 @@ use windows_sys::Win32::{ }, }; -pub type AncillaryData = HANDLE; +pub type AncillaryData = OwnedHandle; +pub type RawAncillaryData = HANDLE; + +impl IntoRawAncillaryData for AncillaryData { + fn into_raw(self) -> RawAncillaryData { + self.into_raw_handle() as HANDLE + } +} // This must match `kInvalidHandle` in `mfbt/UniquePtrExt.h` -pub const INVALID_ANCILLARY_DATA: AncillaryData = 0; +pub const INVALID_ANCILLARY_DATA: RawAncillaryData = 0; const HANDLE_SIZE: usize = size_of::<HANDLE>(); // We encode handles at the beginning of every transmitted message. This // function extracts the handle (if present) and returns it together with // the rest of the buffer. -fn extract_buffer_and_handle(buffer: Vec<u8>) -> Result<(Vec<u8>, Option<HANDLE>), IPCError> { +fn extract_buffer_and_handle(buffer: Vec<u8>) -> Result<(Vec<u8>, Option<OwnedHandle>), IPCError> { let handle_bytes = &buffer[0..HANDLE_SIZE]; let data = &buffer[HANDLE_SIZE..]; let handle_bytes: Result<[u8; HANDLE_SIZE], _> = handle_bytes.try_into(); @@ -52,35 +60,50 @@ fn extract_buffer_and_handle(buffer: Vec<u8>) -> Result<(Vec<u8>, Option<HANDLE> }; let handle = match HANDLE::from_ne_bytes(handle_bytes) { INVALID_ANCILLARY_DATA => None, - handle => Some(handle), + handle => Some(unsafe { OwnedHandle::from_raw_handle(handle as RawHandle) }), }; Ok((data.to_vec(), handle)) } pub struct IPCConnector { + /// A connected pipe handle handle: OwnedHandle, + /// A handle to an event which will be used for overlapped I/O on the pipe event: OwnedHandle, + /// Stores the only pending operation we might have on the pipe overlapped: Option<OverlappedOperation>, + /// The process at the other end of the pipe, this is needed to send + /// ancillary data and a send operation will fail if not set. + process: Option<OwnedHandle>, } impl IPCConnector { - pub fn new(handle: OwnedHandle) -> Result<IPCConnector, IPCError> { + pub fn from_ancillary(handle: OwnedHandle) -> Result<IPCConnector, IPCError> { let event = create_manual_reset_event()?; Ok(IPCConnector { handle, event, overlapped: None, + process: None, }) } - pub fn from_ancillary(ancillary_data: AncillaryData) -> Result<IPCConnector, IPCError> { - IPCConnector::new(unsafe { OwnedHandle::from_raw_handle(ancillary_data as RawHandle) }) + /// Create a connector from a raw handle. + /// + /// # Safety + /// + /// The `ancillary_data` argument must be a valid HANDLE representing the + /// endpoint of a named pipe. + pub unsafe fn from_raw_ancillary( + ancillary_data: RawAncillaryData, + ) -> Result<IPCConnector, IPCError> { + IPCConnector::from_ancillary(OwnedHandle::from_raw_handle(ancillary_data as RawHandle)) } - pub fn as_raw(&self) -> HANDLE { - self.handle.as_raw_handle() as HANDLE + pub fn set_process(&mut self, process: OwnedHandle) { + self.process = Some(process); } pub fn event_raw_handle(&self) -> HANDLE { @@ -161,9 +184,8 @@ impl IPCConnector { return Err(IPCError::System(unsafe { GetLastError() })); } - // SAFETY: The raw pipe handle is guaranteed to be open at this point - let handle = unsafe { OwnedHandle::from_raw_handle(pipe as RawHandle) }; - IPCConnector::new(handle) + // SAFETY: We've verified above that the pipe handle is valid + unsafe { IPCConnector::from_raw_ancillary(pipe) } } /// Serialize this connector into a string that can be passed on the @@ -178,78 +200,28 @@ impl IPCConnector { pub fn deserialize(string: &CStr) -> Result<IPCConnector, IPCError> { let string = string.to_str().map_err(|_e| IPCError::ParseError)?; let handle = usize::from_str(string).map_err(|_e| IPCError::ParseError)?; - let handle = handle as *mut c_void; // SAFETY: This is a handle we passed in ourselves. - let handle = unsafe { OwnedHandle::from_raw_handle(handle) }; - IPCConnector::new(handle) + unsafe { IPCConnector::from_raw_ancillary(handle as HANDLE) } } - pub fn into_ancillary( - self, - dst_process: &Option<ProcessHandle>, - ) -> Result<AncillaryData, IPCError> { - let mut dst_handle: HANDLE = INVALID_ANCILLARY_DATA; - - if let Some(dst_process) = dst_process.as_ref() { - let res = unsafe { - DuplicateHandle( - GetCurrentProcess(), - self.handle.into_raw_handle() as HANDLE, - dst_process.as_raw_handle() as HANDLE, - &mut dst_handle, - /* dwDesiredAccess */ 0, - /* bInheritHandle */ FALSE, - DUPLICATE_CLOSE_SOURCE | DUPLICATE_SAME_ACCESS, - ) - }; - - if res > 0 { - Ok(dst_handle) - } else { - Err(IPCError::System(get_last_error())) - } - } else { - Ok(self.handle.into_raw_handle() as HANDLE) - } + pub fn into_ancillary(self) -> AncillaryData { + self.handle } - /// Like into_ancillary, but the IPCConnector retains ownership of the file descriptor (so be - /// sure to use the result during the lifetime of the IPCConnector). - pub fn as_ancillary( - &self, - dst_process: &Option<ProcessHandle>, - ) -> Result<AncillaryData, IPCError> { - let mut dst_handle: HANDLE = INVALID_ANCILLARY_DATA; - - if let Some(dst_process) = dst_process.as_ref() { - let res = unsafe { - DuplicateHandle( - GetCurrentProcess(), - self.handle.as_raw_handle() as HANDLE, - dst_process.as_raw_handle() as HANDLE, - &mut dst_handle, - /* dwDesiredAccess */ 0, - /* bInheritHandle */ FALSE, - DUPLICATE_SAME_ACCESS, - ) - }; - - if res > 0 { - Ok(dst_handle) - } else { - Err(IPCError::System(get_last_error())) - } - } else { - Ok(self.handle.as_raw_handle() as HANDLE) - } + pub fn into_raw_ancillary(self) -> RawAncillaryData { + self.handle.into_raw() } - pub fn send_message(&self, message: &dyn Message) -> Result<(), IPCError> { + pub fn send_message<T>(&self, message: T) -> Result<(), IPCError> + where + T: Message, + { // Send the message header self.send(&message.header(), None)?; // Send the message payload - self.send(&message.payload(), message.ancillary_payload())?; + let (payload, ancillary_data) = message.into_payload(); + self.send(&payload, ancillary_data)?; Ok(()) } @@ -298,9 +270,15 @@ impl IPCConnector { messages::Header::decode(data.as_ref()).map_err(IPCError::BadMessage) } - pub fn send(&self, buff: &[u8], handle: Option<AncillaryData>) -> Result<(), IPCError> { + fn send(&self, buff: &[u8], handle: Option<AncillaryData>) -> Result<(), IPCError> { + let handle = if let Some(handle) = handle { + self.clone_handle(handle)? + } else { + INVALID_ANCILLARY_DATA + }; + let mut buffer = Vec::<u8>::with_capacity(HANDLE_SIZE + buff.len()); - buffer.extend(handle.unwrap_or(INVALID_ANCILLARY_DATA).to_ne_bytes()); + buffer.extend(handle.to_ne_bytes()); buffer.extend(buff); let overlapped = OverlappedOperation::sched_send( @@ -324,6 +302,36 @@ impl IPCConnector { let buffer = overlapped.collect_recv(/* wait */ true)?; extract_buffer_and_handle(buffer) } + + /// Clone a handle in the destination process, this is required to + /// transfer handles over this connector. Note that this consumes the + /// incoming handle because we want it to be closed after it's been cloned + /// over to the other process. + fn clone_handle(&self, handle: OwnedHandle) -> Result<HANDLE, IPCError> { + let Some(dst_process) = self.process.as_ref() else { + return Err(IPCError::MissingProcessHandle); + }; + let mut dst_handle: HANDLE = INVALID_ANCILLARY_DATA; + let res = unsafe { + DuplicateHandle( + GetCurrentProcess(), + handle.into_raw_handle() as HANDLE, + dst_process.as_raw_handle() as HANDLE, + &mut dst_handle, + /* dwDesiredAccess */ 0, + /* bInheritHandle */ FALSE, + DUPLICATE_CLOSE_SOURCE | DUPLICATE_SAME_ACCESS, + ) + }; + + if res == 0 { + return Err(IPCError::CloneHandleFailed(Error::from_raw_os_error( + get_last_error() as i32, + ))); + } + + Ok(dst_handle) + } } // SAFETY: The connector can be transferred across threads in spite of the raw diff --git a/toolkit/crashreporter/crash_helper_common/src/ipc_listener/windows.rs b/toolkit/crashreporter/crash_helper_common/src/ipc_listener/windows.rs @@ -9,7 +9,7 @@ use crate::{ }; use std::{ - ffi::{c_void, CStr, CString, OsString}, + ffi::{CStr, CString, OsString}, os::windows::io::{AsRawHandle, FromRawHandle, OwnedHandle, RawHandle}, ptr::null_mut, str::FromStr, @@ -34,7 +34,7 @@ pub struct IPCListener { } impl IPCListener { - pub fn new(server_addr: CString) -> Result<IPCListener, IPCError> { + pub(crate) fn new(server_addr: CString) -> Result<IPCListener, IPCError> { let pipe = create_named_pipe(&server_addr, /* first_instance */ true)?; let event = create_manual_reset_event()?; @@ -50,11 +50,11 @@ impl IPCListener { self.event.as_raw_handle() as HANDLE } - pub fn address(&self) -> &CStr { + pub(crate) fn address(&self) -> &CStr { &self.server_addr } - pub fn listen(&mut self) -> Result<(), IPCError> { + pub(crate) fn listen(&mut self) -> Result<(), IPCError> { self.overlapped = Some(OverlappedOperation::listen( self.handle .try_clone() @@ -77,7 +77,7 @@ impl IPCListener { // for the next iteration. self.listen()?; - IPCConnector::new(connected_pipe) + IPCConnector::from_ancillary(connected_pipe) } /// Serialize this listener into a string that can be passed on the @@ -94,9 +94,8 @@ impl IPCListener { let server_addr = server_addr(pid); let string = string.to_str().map_err(|_e| IPCError::ParseError)?; let handle = usize::from_str(string).map_err(|_e| IPCError::ParseError)?; - let handle = handle as *mut c_void; // SAFETY: This is a handle we passed in ourselves. - let handle = unsafe { OwnedHandle::from_raw_handle(handle) }; + let handle = unsafe { OwnedHandle::from_raw_handle(handle as RawHandle) }; let event = create_manual_reset_event()?; let mut listener = IPCListener { diff --git a/toolkit/crashreporter/crash_helper_common/src/lib.rs b/toolkit/crashreporter/crash_helper_common/src/lib.rs @@ -18,7 +18,9 @@ use errors::MessageError; // Re-export the platform-specific types and functions pub use crate::breakpad::{BreakpadChar, BreakpadData, BreakpadRawData, Pid}; pub use crate::ipc_channel::{IPCChannel, IPCClientChannel}; -pub use crate::ipc_connector::{AncillaryData, IPCConnector, IPCEvent, INVALID_ANCILLARY_DATA}; +pub use crate::ipc_connector::{ + AncillaryData, IPCConnector, IPCEvent, RawAncillaryData, INVALID_ANCILLARY_DATA, +}; pub use crate::ipc_listener::IPCListener; pub use crate::platform::ProcessHandle; @@ -58,4 +60,9 @@ pub trait BreakpadString { unsafe fn from_raw(ptr: *mut BreakpadChar) -> OsString; } +/// Owned handle or file descriptor conversion to their respective raw versions +pub trait IntoRawAncillaryData { + fn into_raw(self) -> RawAncillaryData; +} + pub const IO_TIMEOUT: u16 = 2 * 1000; diff --git a/toolkit/crashreporter/crash_helper_common/src/messages.rs b/toolkit/crashreporter/crash_helper_common/src/messages.rs @@ -57,8 +57,7 @@ pub trait Message { where Self: Sized; fn header(&self) -> Vec<u8>; - fn payload(&self) -> Vec<u8>; - fn ancillary_payload(&self) -> Option<AncillaryData>; + fn into_payload(self) -> (Vec<u8>, Option<AncillaryData>); fn decode(data: &[u8], ancillary_data: Option<AncillaryData>) -> Result<Self, MessageError> where Self: Sized; @@ -126,16 +125,12 @@ impl Message for SetCrashReportPath { .encode() } - fn payload(&self) -> Vec<u8> { + fn into_payload(self) -> (Vec<u8>, Option<AncillaryData>) { let mut payload = Vec::with_capacity(self.payload_size()); let path = self.path.serialize(); payload.extend(path.len().to_ne_bytes()); payload.extend(self.path.serialize()); - payload - } - - fn ancillary_payload(&self) -> Option<AncillaryData> { - None + (payload, None) } fn decode( @@ -184,12 +179,8 @@ impl Message for TransferMinidump { .encode() } - fn payload(&self) -> Vec<u8> { - self.pid.to_ne_bytes().to_vec() - } - - fn ancillary_payload(&self) -> Option<AncillaryData> { - None + fn into_payload(self) -> (Vec<u8>, Option<AncillaryData>) { + (self.pid.to_ne_bytes().to_vec(), None) } fn decode( @@ -245,7 +236,7 @@ impl Message for TransferMinidumpReply { .encode() } - fn payload(&self) -> Vec<u8> { + fn into_payload(self) -> (Vec<u8>, Option<AncillaryData>) { let path_bytes = self.path.serialize(); let mut buffer = Vec::with_capacity(self.payload_size()); buffer.extend(path_bytes.len().to_ne_bytes()); @@ -262,11 +253,7 @@ impl Message for TransferMinidumpReply { .as_ref() .map_or(Vec::new(), |error| Vec::from(error.as_bytes())), ); - buffer - } - - fn ancillary_payload(&self) -> Option<AncillaryData> { - None + (buffer, None) } fn decode( @@ -348,7 +335,7 @@ impl Message for WindowsErrorReportingMinidump { .encode() } - fn payload(&self) -> Vec<u8> { + fn into_payload(self) -> (Vec<u8>, Option<AncillaryData>) { let mut buffer = Vec::<u8>::with_capacity(self.payload_size()); buffer.extend(self.pid.to_ne_bytes()); buffer.extend(self.tid.to_ne_bytes()); @@ -360,11 +347,7 @@ impl Message for WindowsErrorReportingMinidump { } let bytes: [u8; size_of::<CONTEXT>()] = unsafe { std::mem::transmute(self.context) }; buffer.extend(bytes); - buffer - } - - fn ancillary_payload(&self) -> Option<AncillaryData> { - None + (buffer, None) } fn decode( @@ -452,12 +435,8 @@ impl Message for WindowsErrorReportingMinidumpReply { .encode() } - fn payload(&self) -> Vec<u8> { - Vec::<u8>::new() - } - - fn ancillary_payload(&self) -> Option<AncillaryData> { - None + fn into_payload(self) -> (Vec<u8>, Option<AncillaryData>) { + (Vec::<u8>::new(), None) } fn decode( @@ -507,7 +486,7 @@ impl Message for RegisterAuxvInfo { .encode() } - fn payload(&self) -> Vec<u8> { + fn into_payload(self) -> (Vec<u8>, Option<AncillaryData>) { let mut payload = Vec::with_capacity(self.payload_size()); payload.extend(self.pid.to_ne_bytes()); payload.extend(self.auxv_info.program_header_count.to_ne_bytes()); @@ -515,11 +494,7 @@ impl Message for RegisterAuxvInfo { payload.extend(self.auxv_info.linux_gate_address.to_ne_bytes()); payload.extend(self.auxv_info.entry_address.to_ne_bytes()); debug_assert!(self.payload_size() == payload.len()); - payload - } - - fn ancillary_payload(&self) -> Option<AncillaryData> { - None + (payload, None) } fn decode( @@ -598,15 +573,11 @@ impl Message for UnregisterAuxvInfo { .encode() } - fn payload(&self) -> Vec<u8> { + fn into_payload(self) -> (Vec<u8>, Option<AncillaryData>) { let mut payload = Vec::with_capacity(self.payload_size()); payload.extend(self.pid.to_ne_bytes()); debug_assert!(self.payload_size() == payload.len()); - payload - } - - fn ancillary_payload(&self) -> Option<AncillaryData> { - None + (payload, None) } fn decode( @@ -635,7 +606,9 @@ pub struct RegisterChildProcess { impl RegisterChildProcess { pub fn new(ipc_endpoint: AncillaryData) -> RegisterChildProcess { - RegisterChildProcess { ipc_endpoint } + RegisterChildProcess { + ipc_endpoint, + } } fn payload_size(&self) -> usize { @@ -656,12 +629,8 @@ impl Message for RegisterChildProcess { .encode() } - fn payload(&self) -> Vec<u8> { - Vec::<u8>::new() - } - - fn ancillary_payload(&self) -> Option<AncillaryData> { - Some(self.ipc_endpoint) + fn into_payload(self) -> (Vec<u8>, Option<AncillaryData>) { + (Vec::<u8>::new(), Some(self.ipc_endpoint)) } fn decode( @@ -705,12 +674,8 @@ impl Message for ChildProcessRegistered { .encode() } - fn payload(&self) -> Vec<u8> { - self.crash_helper_pid.to_ne_bytes().to_vec() - } - - fn ancillary_payload(&self) -> Option<AncillaryData> { - None + fn into_payload(self) -> (Vec<u8>, Option<AncillaryData>) { + (self.crash_helper_pid.to_ne_bytes().to_vec(), None) } fn decode( diff --git a/toolkit/crashreporter/crash_helper_server/src/crash_generation.rs b/toolkit/crashreporter/crash_helper_server/src/crash_generation.rs @@ -78,7 +78,6 @@ enum MinidumpOrigin { pub(crate) enum MessageResult { None, - Reply(Box<dyn Message>), Connection(IPCConnector), } @@ -111,6 +110,7 @@ impl CrashGenerator { // reply that will be sent back to the parent. pub(crate) fn parent_message( &mut self, + client: &IPCConnector, kind: messages::Kind, data: &[u8], ancillary_data: Option<AncillaryData>, @@ -123,9 +123,8 @@ impl CrashGenerator { } messages::Kind::TransferMinidump => { let message = messages::TransferMinidump::decode(data, ancillary_data)?; - Ok(MessageResult::Reply(Box::new( - self.transfer_minidump(message.pid), - ))) + client.send_message(self.transfer_minidump(message.pid))?; + Ok(MessageResult::None) } messages::Kind::GenerateMinidump => { todo!("Implement all messages"); @@ -150,7 +149,7 @@ impl CrashGenerator { let message = messages::RegisterChildProcess::decode(data, ancillary_data)?; let connector = IPCConnector::from_ancillary(message.ipc_endpoint)?; connector - .send_message(&messages::ChildProcessRegistered::new(process::id() as Pid))?; + .send_message(messages::ChildProcessRegistered::new(process::id() as Pid))?; Ok(MessageResult::Connection(connector)) } kind => { @@ -174,6 +173,7 @@ impl CrashGenerator { // reply that will be sent back. pub(crate) fn external_message( &mut self, + #[allow(unused_variables)] runtime: &IPCConnector, kind: messages::Kind, #[allow(unused_variables)] data: &[u8], #[allow(unused_variables)] ancillary_data: Option<AncillaryData>, @@ -184,9 +184,8 @@ impl CrashGenerator { let message = messages::WindowsErrorReportingMinidump::decode(data, ancillary_data)?; let _ = self.generate_wer_minidump(message); - Ok(MessageResult::Reply(Box::new( - messages::WindowsErrorReportingMinidumpReply::new(), - ))) + runtime.send_message(messages::WindowsErrorReportingMinidumpReply::new())?; + Ok(MessageResult::None) } kind => { bail!("Unexpected message {kind:?} from external process"); diff --git a/toolkit/crashreporter/crash_helper_server/src/ipc_server.rs b/toolkit/crashreporter/crash_helper_server/src/ipc_server.rs @@ -100,14 +100,17 @@ impl IPCServer { let connector = &mut connection.connector; let (data, ancillary_data) = connector.recv(header.size)?; - let reply = match connection.endpoint { - IPCEndpoint::Parent => generator.parent_message(header.kind, &data, ancillary_data), + let connection = match connection.endpoint { + IPCEndpoint::Parent => { + generator.parent_message(connector, header.kind, &data, ancillary_data) + } IPCEndpoint::Child => generator.child_message(header.kind, &data, ancillary_data), - IPCEndpoint::External => generator.external_message(header.kind, &data, ancillary_data), + IPCEndpoint::External => { + generator.external_message(connector, header.kind, &data, ancillary_data) + } }?; - match reply { - MessageResult::Reply(reply) => connector.send_message(reply.as_ref())?, + match connection { MessageResult::Connection(connector) => { self.connections.push(IPCConnection { connector, diff --git a/toolkit/crashreporter/crash_helper_server/src/lib.rs b/toolkit/crashreporter/crash_helper_server/src/lib.rs @@ -13,13 +13,13 @@ mod phc; #[cfg(not(target_os = "android"))] use crash_helper_common::Pid; +#[cfg(target_os = "android")] +use crash_helper_common::RawAncillaryData; use crash_helper_common::{BreakpadData, BreakpadRawData, IPCConnector, IPCListener}; use std::ffi::{c_char, CStr, OsString}; use crash_generation::CrashGenerator; use ipc_server::{IPCServer, IPCServerState}; -#[cfg(target_os = "android")] -use std::os::fd::{FromRawFd, OwnedFd, RawFd}; /// Runs the crash generator process logic, this includes the IPC used by /// processes to signal that they crashed, the IPC used to retrieve crash @@ -92,7 +92,7 @@ pub unsafe extern "C" fn crash_generator_logic_desktop( pub unsafe extern "C" fn crash_generator_logic_android( breakpad_data: BreakpadRawData, minidump_path: *const c_char, - pipe: RawFd, + pipe: RawAncillaryData, ) { logging::init(); @@ -110,8 +110,9 @@ pub unsafe extern "C" fn crash_generator_logic_android( .unwrap(); let listener = IPCListener::new(0).unwrap(); - let pipe = unsafe { OwnedFd::from_raw_fd(pipe) }; - let connector = IPCConnector::from_fd(pipe) + // SAFETY: The `pipe` file descriptor passed in from the caller is + // guaranteed to be valid. + let connector = unsafe { IPCConnector::from_raw_ancillary(pipe) } .map_err(|error| { log::error!("Could not use the pipe (error: {error})"); }) diff --git a/toolkit/crashreporter/nsExceptionHandler.cpp b/toolkit/crashreporter/nsExceptionHandler.cpp @@ -3340,7 +3340,8 @@ CrashPipeType GetChildNotificationPipe() { UniqueFileHandle RegisterChildIPCChannel() { if (gCrashHelperClient) { - AncillaryData ipc_endpoint = register_child_ipc_channel(gCrashHelperClient); + RawAncillaryData ipc_endpoint = + register_child_ipc_channel(gCrashHelperClient); return UniqueFileHandle{ipc_endpoint}; }