commit b9908103c43dfa6f421a08de195185cbb68eaed7 parent 936f7e47dce92f20fe3a250c9aaae63bf0f82281 Author: Narcis Beleuzu <nbeleuzu@mozilla.com> Date: Mon, 6 Oct 2025 21:13:43 +0300 Revert "Bug 1982016: apply code formatting via Lando" for causing reftest crahses This reverts commit 227ca58dd353c9fdb1ae356a2502b4865e8ce847. This reverts commit b7d7df929f6954d8cf5a0638b227393d974e99ab. This reverts commit c2e5b3d2d5a697fc1fe0ed9af30bd89f3a9679d9. Diffstat:
16 files changed, 207 insertions(+), 203 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, IntoRawAncillaryData, - ProcessHandle, RawAncillaryData, INVALID_ANCILLARY_DATA, + AncillaryData, BreakpadString, IPCClientChannel, IPCConnector, ProcessHandle, + INVALID_ANCILLARY_DATA, }; #[cfg(any(target_os = "android", target_os = "linux"))] use minidump_writer::minidump_writer::{AuxvType, DirectAuxvDumpInfo}; @@ -34,12 +34,13 @@ 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(()) } @@ -56,36 +57,48 @@ impl CrashHelperClient { bail!("The crash helper process failed to launch"); }; - self.connector.set_process(process_handle); + self.helper_process = Some(process_handle); } - let Ok(ancillary_data) = server_endpoint.into_ancillary() else { + 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)?; + 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(client_endpoint.into_ancillary().unwrap()) + Ok(ancillary_data) } #[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")] @@ -219,10 +232,10 @@ pub unsafe extern "C" fn set_crash_report_path( #[no_mangle] pub unsafe extern "C" fn register_child_ipc_channel( client: *mut CrashHelperClient, -) -> RawAncillaryData { +) -> AncillaryData { let client = client.as_mut().unwrap(); if let Ok(client_endpoint) = client.register_child_process() { - client_endpoint.into_raw() + client_endpoint } else { INVALID_ANCILLARY_DATA } @@ -298,7 +311,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>()); } } @@ -376,7 +389,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<RawAncillaryData>> = OnceLock::new(); +static CHILD_IPC_ENDPOINT: OnceLock<Box<AncillaryData>> = OnceLock::new(); static RENDEZVOUS_FAILED: AtomicBool = AtomicBool::new(false); /// Let a client rendez-vous with the crash helper process. This step ensures @@ -389,8 +402,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: RawAncillaryData) { - let Ok(connector) = IPCConnector::from_raw_ancillary(client_endpoint) else { +pub unsafe extern "C" fn crash_helper_rendezvous(client_endpoint: AncillaryData) { + let Ok(connector) = IPCConnector::from_ancillary(client_endpoint) else { RENDEZVOUS_FAILED.store(true, Ordering::Relaxed); return; }; @@ -400,7 +413,7 @@ pub unsafe extern "C" fn crash_helper_rendezvous(client_endpoint: RawAncillaryDa CrashHelperClient::prepare_for_minidump(message.crash_helper_pid); assert!( CHILD_IPC_ENDPOINT - .set(Box::new(connector.into_raw_ancillary().unwrap())) + .set(Box::new(connector.into_ancillary(&None).unwrap())) .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 @@ -17,6 +17,7 @@ impl CrashHelperClient { 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,6 +34,7 @@ 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,6 +52,7 @@ impl CrashHelperClient { Ok(CrashHelperClient { connector: client_endpoint, spawner_thread: Some(spawner_thread), + helper_process: None, }) } diff --git a/toolkit/crashreporter/crash_helper_common/src/breakpad/macos.rs b/toolkit/crashreporter/crash_helper_common/src/breakpad/macos.rs @@ -23,11 +23,6 @@ pub struct BreakpadData { } impl BreakpadData { - /// Creates a new BreakpadData object from a C string - /// - /// # Safety - /// - /// The `raw` argument must point to a valid NUL-terminated C string pub unsafe fn new(raw: BreakpadRawData) -> BreakpadData { BreakpadData { data: <OsString as BreakpadString>::from_ptr(raw), diff --git a/toolkit/crashreporter/crash_helper_common/src/errors.rs b/toolkit/crashreporter/crash_helper_common/src/errors.rs @@ -40,9 +40,6 @@ 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, RawAncillaryData, INVALID_ANCILLARY_DATA}; +pub use windows::{AncillaryData, IPCConnector, 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, RawAncillaryData, INVALID_ANCILLARY_DATA}; +pub use unix::{AncillaryData, IPCConnector, 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, IntoRawAncillaryData, ProcessHandle, IO_TIMEOUT}; +use crate::{ignore_eintr, ProcessHandle, IO_TIMEOUT}; use nix::{ cmsg_space, @@ -26,17 +26,10 @@ use crate::{ messages::{self, Message}, }; -pub type RawAncillaryData = RawFd; -pub type AncillaryData = OwnedFd; - -impl IntoRawAncillaryData for AncillaryData { - fn into_raw(self) -> RawAncillaryData { - self.into_raw_fd() - } -} +pub type AncillaryData = RawFd; // This must match `kInvalidHandle` in `mfbt/UniquePtrExt.h` -pub const INVALID_ANCILLARY_DATA: RawAncillaryData = -1; +pub const INVALID_ANCILLARY_DATA: AncillaryData = -1; pub struct IPCConnector { socket: OwnedFd, @@ -60,24 +53,10 @@ impl IPCConnector { Ok(IPCConnector { socket }) } - pub fn from_ancillary(socket: AncillaryData) -> Result<IPCConnector, IPCError> { - IPCConnector::from_fd(socket) + pub fn from_ancillary(ancillary_data: AncillaryData) -> Result<IPCConnector, IPCError> { + IPCConnector::from_fd(unsafe { OwnedFd::from_raw_fd(ancillary_data) }) } - /// 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. @@ -98,12 +77,20 @@ impl IPCConnector { self.socket.as_raw_fd() } - pub fn into_ancillary(self) -> Result<AncillaryData, IPCError> { - Ok(self.socket) + pub fn into_ancillary( + self, + _dst_process: &Option<ProcessHandle>, + ) -> Result<AncillaryData, IPCError> { + Ok(self.socket.into_raw_fd()) } - pub fn into_raw_ancillary(self) -> Result<RawAncillaryData, 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 as_raw_ref(&self) -> BorrowedFd<'_> { @@ -123,14 +110,10 @@ impl IPCConnector { } } - pub fn send_message<T>(&self, message: T) -> Result<(), IPCError> - where - T: Message, - { + pub fn send_message(&self, message: &dyn Message) -> Result<(), IPCError> { self.send(&message.header(), None) .map_err(IPCError::TransmissionFailure)?; - let (payload, ancillary_data) = message.into_payload(); - self.send(&payload, ancillary_data) + self.send(&message.payload(), message.ancillary_payload()) .map_err(IPCError::TransmissionFailure) } @@ -148,9 +131,9 @@ 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.iter().map(|fd| fd.as_raw_fd()).collect(); + let scm_fds: Vec<i32> = fd.map_or(vec![], |fd| vec![fd]); let scm = ControlMessage::ScmRights(&scm_fds); let res = ignore_eintr!(sendmsg::<()>( @@ -176,13 +159,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, } @@ -231,7 +214,7 @@ impl IPCConnector { let fd = if let Some(cmsg) = res.cmsgs()?.next() { if let ControlMessageOwned::ScmRights(fds) = cmsg { - fds.first().map(|&fd| unsafe { OwnedFd::from_raw_fd(fd) }) + fds.first().copied() } 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,12 +6,11 @@ use crate::{ errors::IPCError, messages::{self, Message}, platform::windows::{create_manual_reset_event, get_last_error, OverlappedOperation}, - IntoRawAncillaryData, IO_TIMEOUT, + ProcessHandle, IO_TIMEOUT, }; use std::{ - ffi::{CStr, OsString}, - io::Error, + ffi::{c_void, CStr, OsString}, os::windows::io::{AsRawHandle, FromRawHandle, IntoRawHandle, OwnedHandle, RawHandle}, ptr::null_mut, str::FromStr, @@ -34,24 +33,17 @@ use windows_sys::Win32::{ }, }; -pub type AncillaryData = OwnedHandle; -pub type RawAncillaryData = HANDLE; - -impl IntoRawAncillaryData for AncillaryData { - fn into_raw(self) -> RawAncillaryData { - self.into_raw_handle() as HANDLE - } -} +pub type AncillaryData = HANDLE; // This must match `kInvalidHandle` in `mfbt/UniquePtrExt.h` -pub const INVALID_ANCILLARY_DATA: RawAncillaryData = 0; +pub const INVALID_ANCILLARY_DATA: AncillaryData = 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<OwnedHandle>), IPCError> { +fn extract_buffer_and_handle(buffer: Vec<u8>) -> Result<(Vec<u8>, Option<HANDLE>), IPCError> { let handle_bytes = &buffer[0..HANDLE_SIZE]; let data = &buffer[HANDLE_SIZE..]; let handle_bytes: Result<[u8; HANDLE_SIZE], _> = handle_bytes.try_into(); @@ -60,50 +52,31 @@ fn extract_buffer_and_handle(buffer: Vec<u8>) -> Result<(Vec<u8>, Option<OwnedHa }; let handle = match HANDLE::from_ne_bytes(handle_bytes) { INVALID_ANCILLARY_DATA => None, - handle => Some(unsafe { OwnedHandle::from_raw_handle(handle as RawHandle) }), + handle => Some(handle), }; 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 from_ancillary(handle: OwnedHandle) -> Result<IPCConnector, IPCError> { + pub fn new(handle: OwnedHandle) -> Result<IPCConnector, IPCError> { let event = create_manual_reset_event()?; Ok(IPCConnector { handle, event, overlapped: None, - process: None, }) } - /// 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 set_process(&mut self, process: OwnedHandle) { - self.process = Some(process); + pub fn from_ancillary(ancillary_data: AncillaryData) -> Result<IPCConnector, IPCError> { + IPCConnector::new(unsafe { OwnedHandle::from_raw_handle(ancillary_data as RawHandle) }) } pub fn as_raw(&self) -> HANDLE { @@ -188,8 +161,9 @@ impl IPCConnector { return Err(IPCError::System(unsafe { GetLastError() })); } - // SAFETY: We've verified above that the pipe handle is valid - unsafe { IPCConnector::from_raw_ancillary(pipe) } + // 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) } /// Serialize this connector into a string that can be passed on the @@ -204,28 +178,78 @@ 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. - unsafe { IPCConnector::from_raw_ancillary(handle as HANDLE) } + let handle = unsafe { OwnedHandle::from_raw_handle(handle) }; + IPCConnector::new(handle) } - pub fn into_ancillary(self) -> Result<AncillaryData, IPCError> { - Ok(self.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_raw_ancillary(self) -> Result<RawAncillaryData, IPCError> { - Ok(self.handle.into_raw()) + /// 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 send_message<T>(&self, message: T) -> Result<(), IPCError> - where - T: Message, - { + pub fn send_message(&self, message: &dyn Message) -> Result<(), IPCError> { // Send the message header self.send(&message.header(), None)?; // Send the message payload - let (payload, ancillary_data) = message.into_payload(); - self.send(&payload, ancillary_data)?; + self.send(&message.payload(), message.ancillary_payload())?; Ok(()) } @@ -275,14 +299,8 @@ impl IPCConnector { } pub 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.to_ne_bytes()); + buffer.extend(handle.unwrap_or(INVALID_ANCILLARY_DATA).to_ne_bytes()); buffer.extend(buff); let overlapped = OverlappedOperation::sched_send( @@ -306,36 +324,6 @@ 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.as_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 @@ -77,7 +77,7 @@ impl IPCListener { // for the next iteration. self.listen()?; - IPCConnector::from_ancillary(connected_pipe) + IPCConnector::new(connected_pipe) } /// Serialize this listener into a string that can be passed on the diff --git a/toolkit/crashreporter/crash_helper_common/src/lib.rs b/toolkit/crashreporter/crash_helper_common/src/lib.rs @@ -18,9 +18,7 @@ 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, RawAncillaryData, INVALID_ANCILLARY_DATA, -}; +pub use crate::ipc_connector::{AncillaryData, IPCConnector, IPCEvent, INVALID_ANCILLARY_DATA}; pub use crate::ipc_listener::IPCListener; pub use crate::platform::ProcessHandle; @@ -60,9 +58,4 @@ 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,7 +57,8 @@ pub trait Message { where Self: Sized; fn header(&self) -> Vec<u8>; - fn into_payload(self) -> (Vec<u8>, Option<AncillaryData>); + fn payload(&self) -> Vec<u8>; + fn ancillary_payload(&self) -> Option<AncillaryData>; fn decode(data: &[u8], ancillary_data: Option<AncillaryData>) -> Result<Self, MessageError> where Self: Sized; @@ -125,12 +126,16 @@ impl Message for SetCrashReportPath { .encode() } - fn into_payload(self) -> (Vec<u8>, Option<AncillaryData>) { + fn payload(&self) -> Vec<u8> { 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, None) + payload + } + + fn ancillary_payload(&self) -> Option<AncillaryData> { + None } fn decode( @@ -179,8 +184,12 @@ impl Message for TransferMinidump { .encode() } - fn into_payload(self) -> (Vec<u8>, Option<AncillaryData>) { - (self.pid.to_ne_bytes().to_vec(), None) + fn payload(&self) -> Vec<u8> { + self.pid.to_ne_bytes().to_vec() + } + + fn ancillary_payload(&self) -> Option<AncillaryData> { + None } fn decode( @@ -236,7 +245,7 @@ impl Message for TransferMinidumpReply { .encode() } - fn into_payload(self) -> (Vec<u8>, Option<AncillaryData>) { + fn payload(&self) -> Vec<u8> { let path_bytes = self.path.serialize(); let mut buffer = Vec::with_capacity(self.payload_size()); buffer.extend(path_bytes.len().to_ne_bytes()); @@ -253,7 +262,11 @@ impl Message for TransferMinidumpReply { .as_ref() .map_or(Vec::new(), |error| Vec::from(error.as_bytes())), ); - (buffer, None) + buffer + } + + fn ancillary_payload(&self) -> Option<AncillaryData> { + None } fn decode( @@ -335,7 +348,7 @@ impl Message for WindowsErrorReportingMinidump { .encode() } - fn into_payload(self) -> (Vec<u8>, Option<AncillaryData>) { + fn payload(&self) -> Vec<u8> { let mut buffer = Vec::<u8>::with_capacity(self.payload_size()); buffer.extend(self.pid.to_ne_bytes()); buffer.extend(self.tid.to_ne_bytes()); @@ -347,7 +360,11 @@ impl Message for WindowsErrorReportingMinidump { } let bytes: [u8; size_of::<CONTEXT>()] = unsafe { std::mem::transmute(self.context) }; buffer.extend(bytes); - (buffer, None) + buffer + } + + fn ancillary_payload(&self) -> Option<AncillaryData> { + None } fn decode( @@ -435,8 +452,12 @@ impl Message for WindowsErrorReportingMinidumpReply { .encode() } - fn into_payload(self) -> (Vec<u8>, Option<AncillaryData>) { - (Vec::<u8>::new(), None) + fn payload(&self) -> Vec<u8> { + Vec::<u8>::new() + } + + fn ancillary_payload(&self) -> Option<AncillaryData> { + None } fn decode( @@ -486,7 +507,7 @@ impl Message for RegisterAuxvInfo { .encode() } - fn into_payload(self) -> (Vec<u8>, Option<AncillaryData>) { + fn payload(&self) -> Vec<u8> { 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()); @@ -494,7 +515,11 @@ 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, None) + payload + } + + fn ancillary_payload(&self) -> Option<AncillaryData> { + None } fn decode( @@ -573,11 +598,15 @@ impl Message for UnregisterAuxvInfo { .encode() } - fn into_payload(self) -> (Vec<u8>, Option<AncillaryData>) { + fn payload(&self) -> Vec<u8> { let mut payload = Vec::with_capacity(self.payload_size()); payload.extend(self.pid.to_ne_bytes()); debug_assert!(self.payload_size() == payload.len()); - (payload, None) + payload + } + + fn ancillary_payload(&self) -> Option<AncillaryData> { + None } fn decode( @@ -606,9 +635,7 @@ pub struct RegisterChildProcess { impl RegisterChildProcess { pub fn new(ipc_endpoint: AncillaryData) -> RegisterChildProcess { - RegisterChildProcess { - ipc_endpoint, - } + RegisterChildProcess { ipc_endpoint } } fn payload_size(&self) -> usize { @@ -629,8 +656,12 @@ impl Message for RegisterChildProcess { .encode() } - fn into_payload(self) -> (Vec<u8>, Option<AncillaryData>) { - (Vec::<u8>::new(), Some(self.ipc_endpoint)) + fn payload(&self) -> Vec<u8> { + Vec::<u8>::new() + } + + fn ancillary_payload(&self) -> Option<AncillaryData> { + Some(self.ipc_endpoint) } fn decode( @@ -674,8 +705,12 @@ impl Message for ChildProcessRegistered { .encode() } - fn into_payload(self) -> (Vec<u8>, Option<AncillaryData>) { - (self.crash_helper_pid.to_ne_bytes().to_vec(), None) + fn payload(&self) -> Vec<u8> { + self.crash_helper_pid.to_ne_bytes().to_vec() + } + + fn ancillary_payload(&self) -> Option<AncillaryData> { + 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,6 +78,7 @@ enum MinidumpOrigin { pub(crate) enum MessageResult { None, + Reply(Box<dyn Message>), Connection(IPCConnector), } @@ -110,7 +111,6 @@ 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,8 +123,9 @@ impl CrashGenerator { } messages::Kind::TransferMinidump => { let message = messages::TransferMinidump::decode(data, ancillary_data)?; - client.send_message(self.transfer_minidump(message.pid))?; - Ok(MessageResult::None) + Ok(MessageResult::Reply(Box::new( + self.transfer_minidump(message.pid), + ))) } messages::Kind::GenerateMinidump => { todo!("Implement all messages"); @@ -149,7 +150,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 => { @@ -173,7 +174,6 @@ 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,8 +184,9 @@ impl CrashGenerator { let message = messages::WindowsErrorReportingMinidump::decode(data, ancillary_data)?; let _ = self.generate_wer_minidump(message); - runtime.send_message(messages::WindowsErrorReportingMinidumpReply::new())?; - Ok(MessageResult::None) + Ok(MessageResult::Reply(Box::new( + messages::WindowsErrorReportingMinidumpReply::new(), + ))) } 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,17 +100,14 @@ impl IPCServer { let connector = &mut connection.connector; let (data, ancillary_data) = connector.recv(header.size)?; - let connection = match connection.endpoint { - IPCEndpoint::Parent => { - generator.parent_message(connector, header.kind, &data, ancillary_data) - } + let reply = match connection.endpoint { + IPCEndpoint::Parent => generator.parent_message(header.kind, &data, ancillary_data), IPCEndpoint::Child => generator.child_message(header.kind, &data, ancillary_data), - IPCEndpoint::External => { - generator.external_message(connector, header.kind, &data, ancillary_data) - } + IPCEndpoint::External => generator.external_message(header.kind, &data, ancillary_data), }?; - match connection { + match reply { + MessageResult::Reply(reply) => connector.send_message(reply.as_ref())?, MessageResult::Connection(connector) => { self.connections.push(IPCConnection { connector, diff --git a/toolkit/crashreporter/nsExceptionHandler.cpp b/toolkit/crashreporter/nsExceptionHandler.cpp @@ -3340,8 +3340,7 @@ CrashPipeType GetChildNotificationPipe() { UniqueFileHandle RegisterChildIPCChannel() { if (gCrashHelperClient) { - RawAncillaryData ipc_endpoint = - register_child_ipc_channel(gCrashHelperClient); + AncillaryData ipc_endpoint = register_child_ipc_channel(gCrashHelperClient); return UniqueFileHandle{ipc_endpoint}; } diff --git a/toolkit/crashreporter/process_reader/src/platform/linux.rs b/toolkit/crashreporter/process_reader/src/platform/linux.rs @@ -66,7 +66,7 @@ impl ProcessReader { .filter_map(Result::ok) .find_map(|(name, address)| { let name = name?; - if name == module_name { + if &name == module_name { return Some(address); } // Check whether the SO_NAME matches the module name.