commit 0437106b4fa1f47c8562971c72128a755327bb68 parent 4d46c93414e59d8dc40bf62d76138cb3a11714c1 Author: Gabriele Svelto <gsvelto@mozilla.com> Date: Wed, 17 Dec 2025 10:26:00 +0000 Bug 2002785 - Associate Firefox processes' PIDs with their connection in the crash helper r=afranchuk,geckoview-reviewers,tcampbell Differential Revision: https://phabricator.services.mozilla.com/D275946 Diffstat:
13 files changed, 281 insertions(+), 117 deletions(-)
diff --git a/mobile/android/geckoview/src/main/java/org/mozilla/gecko/crashhelper/CrashHelper.java b/mobile/android/geckoview/src/main/java/org/mozilla/gecko/crashhelper/CrashHelper.java @@ -13,6 +13,7 @@ import android.os.Binder; import android.os.DeadObjectException; import android.os.IBinder; import android.os.ParcelFileDescriptor; +import android.os.Process; import android.os.RemoteException; import android.system.ErrnoException; import android.system.Os; @@ -53,7 +54,8 @@ public final class CrashHelper extends Service { // "steal" the crash report of another main process. We should add // additional separation within the crash generation code to prevent this // from happening even though it's very unlikely. - CrashHelper.crash_generator(breakpadFd.detachFd(), minidumpPath, serverFd.detachFd()); + CrashHelper.crash_generator( + Process.myPid(), breakpadFd.detachFd(), minidumpPath, serverFd.detachFd()); return false; } @@ -174,7 +176,8 @@ public final class CrashHelper extends Service { // `stopWithTask` flag set in the manifest, so the service manager will // tear it down for us. - protected static native void crash_generator(int breakpadFd, String minidumpPath, int serverFd); + protected static native void crash_generator( + int clientPid, int breakpadFd, String minidumpPath, int serverFd); protected static native boolean set_breakpad_opts(int breakpadFd); } diff --git a/toolkit/crashreporter/crash_helper/crashhelper_android.cpp b/toolkit/crashreporter/crash_helper/crashhelper_android.cpp @@ -32,7 +32,7 @@ Java_org_mozilla_gecko_crashhelper_CrashHelper_set_1breakpad_1opts( extern "C" JNIEXPORT void JNICALL Java_org_mozilla_gecko_crashhelper_CrashHelper_crash_1generator( - JNIEnv* jenv, jclass, jint breakpad_fd, jstring minidump_path, + JNIEnv* jenv, jclass, jint pid, jint breakpad_fd, jstring minidump_path, jint server_fd) { // The breakpad server socket needs to be put in non-blocking mode, we do it // here as the Rust code that picks it up won't touch it anymore and just @@ -53,6 +53,6 @@ Java_org_mozilla_gecko_crashhelper_CrashHelper_crash_1generator( const char* minidump_path_str = jenv->GetStringUTFChars(minidump_path, nullptr); - crash_generator_logic_android(breakpad_fd, minidump_path_str, server_fd); + crash_generator_logic_android(pid, breakpad_fd, minidump_path_str, server_fd); jenv->ReleaseStringUTFChars(minidump_path, minidump_path_str); } diff --git a/toolkit/crashreporter/crash_helper_client/src/lib.rs b/toolkit/crashreporter/crash_helper_client/src/lib.rs @@ -15,6 +15,7 @@ use std::os::fd::RawFd; use std::{ ffi::{c_char, CString, OsString}, hint::spin_loop, + process, ptr::null_mut, sync::{ atomic::{AtomicBool, Ordering}, @@ -388,15 +389,18 @@ pub unsafe extern "C" fn crash_helper_rendezvous(client_endpoint: RawAncillaryDa }; let join_handle = thread::spawn(move || { - if let Ok(message) = connector.recv_reply::<messages::ChildProcessRegistered>() { - CrashHelperClient::prepare_for_minidump(message.crash_helper_pid); - assert!( - CHILD_IPC_ENDPOINT - .set(Box::new(connector.into_raw_ancillary())) - .is_ok(), - "The crash_helper_rendezvous() function must only be called once" - ); - return; + if let Ok(message) = connector.recv_reply::<messages::ChildProcessRendezVous>() { + let res = CrashHelperClient::prepare_for_minidump(message.crash_helper_pid); + let message = messages::ChildProcessRendezVousReply::new(res, process::id() as Pid); + if let Ok(_) = connector.send_message(message) { + assert!( + CHILD_IPC_ENDPOINT + .set(Box::new(connector.into_raw_ancillary())) + .is_ok(), + "The crash_helper_rendezvous() function must only be called once" + ); + return; + } } RENDEZVOUS_FAILED.store(true, Ordering::Relaxed); diff --git a/toolkit/crashreporter/crash_helper_client/src/platform/android.rs b/toolkit/crashreporter/crash_helper_client/src/platform/android.rs @@ -20,7 +20,8 @@ impl CrashHelperClient { }) } - pub(crate) fn prepare_for_minidump(_crash_helper_pid: Pid) { + pub(crate) fn prepare_for_minidump(_crash_helper_pid: Pid) -> bool { // On Android this is currently a no-op + true } } diff --git a/toolkit/crashreporter/crash_helper_client/src/platform/linux.rs b/toolkit/crashreporter/crash_helper_client/src/platform/linux.rs @@ -8,10 +8,10 @@ use nix::libc::{prctl, PR_SET_PTRACER}; use crate::CrashHelperClient; impl CrashHelperClient { - pub(crate) fn prepare_for_minidump(crash_helper_pid: Pid) { + pub(crate) fn prepare_for_minidump(crash_helper_pid: Pid) -> bool { unsafe { - // TODO: Log a warning in case we fail to set the ptracer - let _ = prctl(PR_SET_PTRACER, crash_helper_pid); + let res = prctl(PR_SET_PTRACER, crash_helper_pid); + res >= 0 } } } diff --git a/toolkit/crashreporter/crash_helper_client/src/platform/unix.rs b/toolkit/crashreporter/crash_helper_client/src/platform/unix.rs @@ -81,7 +81,8 @@ impl CrashHelperClient { } #[cfg(not(target_os = "linux"))] - pub(crate) fn prepare_for_minidump(_pid: crash_helper_common::Pid) { + pub(crate) fn prepare_for_minidump(_pid: crash_helper_common::Pid) -> bool { // This is a no-op on platforms that don't need it + true } } diff --git a/toolkit/crashreporter/crash_helper_client/src/platform/windows.rs b/toolkit/crashreporter/crash_helper_client/src/platform/windows.rs @@ -115,8 +115,9 @@ impl CrashHelperClient { Ok(unsafe { OwnedHandle::from_raw_handle(pi.hProcess as RawHandle) }) } - pub(crate) fn prepare_for_minidump(_crash_helper_pid: Pid) { + pub(crate) fn prepare_for_minidump(_crash_helper_pid: Pid) -> bool { // On Windows this is currently a no-op + true } } diff --git a/toolkit/crashreporter/crash_helper_common/src/ipc_connector/unix.rs b/toolkit/crashreporter/crash_helper_common/src/ipc_connector/unix.rs @@ -131,9 +131,15 @@ impl IPCConnector { where T: Message, { + let expected_payload_len = message.payload_size(); + let expected_ancillary_data = message.has_ancillary_data(); self.send(&message.header(), None) .map_err(IPCError::TransmissionFailure)?; let (payload, ancillary_data) = message.into_payload(); + assert!( + payload.len() == expected_payload_len + ); + assert!(ancillary_data.is_some() == expected_ancillary_data); self.send(&payload, ancillary_data) .map_err(IPCError::TransmissionFailure) } diff --git a/toolkit/crashreporter/crash_helper_common/src/ipc_connector/windows.rs b/toolkit/crashreporter/crash_helper_common/src/ipc_connector/windows.rs @@ -231,8 +231,12 @@ impl IPCConnector { where T: Message, { + let expected_payload_len = message.payload_size(); + let expected_ancillary_data = message.has_ancillary_data(); let header = message.header(); let (payload, ancillary_data) = message.into_payload(); + assert!(payload.len() == expected_payload_len); + assert!(ancillary_data.is_some() == expected_ancillary_data); // Send the message header OverlappedOperation::send(&self.handle, self.event.as_handle(), header)?; diff --git a/toolkit/crashreporter/crash_helper_common/src/messages.rs b/toolkit/crashreporter/crash_helper_common/src/messages.rs @@ -33,6 +33,8 @@ pub enum MessageError { MissingNul(#[from] FromBytesWithNulError), #[error("Truncated message")] Truncated, + #[error("Unexpected ancillary data")] + UnexpectedAncillaryData, } #[repr(u8)] @@ -66,16 +68,18 @@ pub enum Kind { /// to talk to the crash helper. This is sent from the main process to the /// crash helper. RegisterChildProcess = 10, - /// Reply sent by the crash helper to a newly registered child process. - /// Note that this won't be sent back to the main process, it's sent from - /// the crash helper to the newly launched child process. - ChildProcessRegistered = 11, + /// Message sent by the crash helper to rendez-vous with a newly-createdù + /// child process. + ChildProcessRendezVous = 11, + /// Reply to a `ChildProcessRendezVous` message sent by a child after + /// preparing itself for being dumped. + ChildProcessRendezVousReply = 12, } pub trait Message { - fn kind() -> Kind - where - Self: Sized; + fn kind() -> Kind; + fn payload_size(&self) -> usize; + fn has_ancillary_data(&self) -> bool; fn header(&self) -> Vec<u8>; fn into_payload(self) -> (Vec<u8>, Option<AncillaryData>); fn decode(data: &[u8], ancillary_data: Option<AncillaryData>) -> Result<Self, MessageError> @@ -125,16 +129,20 @@ impl SetCrashReportPath { pub fn new(path: OsString) -> SetCrashReportPath { SetCrashReportPath { path } } +} + +impl Message for SetCrashReportPath { + fn kind() -> Kind { + Kind::SetCrashReportPath + } fn payload_size(&self) -> usize { let path_len = self.path.serialize().len(); size_of::<usize>() + path_len } -} -impl Message for SetCrashReportPath { - fn kind() -> Kind { - Kind::SetCrashReportPath + fn has_ancillary_data(&self) -> bool { + false } fn header(&self) -> Vec<u8> { @@ -157,10 +165,9 @@ impl Message for SetCrashReportPath { data: &[u8], ancillary_data: Option<AncillaryData>, ) -> Result<SetCrashReportPath, MessageError> { - debug_assert!( - ancillary_data.is_none(), - "SetCrashReportPath messages cannot carry ancillary data" - ); + if ancillary_data.is_some() { + return Err(MessageError::UnexpectedAncillaryData); + } let path_len_bytes: [u8; size_of::<usize>()] = data[0..size_of::<usize>()].try_into()?; let path_len = usize::from_ne_bytes(path_len_bytes); @@ -191,10 +198,18 @@ impl Message for TransferMinidump { Kind::TransferMinidump } + fn payload_size(&self) -> usize { + size_of::<Pid>() + } + + fn has_ancillary_data(&self) -> bool { + false + } + fn header(&self) -> Vec<u8> { Header { kind: Self::kind(), - size: size_of::<Pid>(), + size: self.payload_size(), } .encode() } @@ -207,10 +222,10 @@ impl Message for TransferMinidump { data: &[u8], ancillary_data: Option<AncillaryData>, ) -> Result<TransferMinidump, MessageError> { - debug_assert!( - ancillary_data.is_none(), - "TransferMinidump messages cannot carry ancillary data" - ); + if ancillary_data.is_some() { + return Err(MessageError::UnexpectedAncillaryData); + } + let bytes: [u8; size_of::<Pid>()] = data[0..size_of::<Pid>()].try_into()?; let pid = Pid::from_ne_bytes(bytes); @@ -230,6 +245,12 @@ impl TransferMinidumpReply { pub fn new(path: OsString, error: Option<CString>) -> TransferMinidumpReply { TransferMinidumpReply { path, error } } +} + +impl Message for TransferMinidumpReply { + fn kind() -> Kind { + Kind::TransferMinidumpReply + } fn payload_size(&self) -> usize { let path_len = self.path.serialize().len(); @@ -241,11 +262,9 @@ impl TransferMinidumpReply { .as_ref() .map_or(0, |error| error.as_bytes().len()) } -} -impl Message for TransferMinidumpReply { - fn kind() -> Kind { - Kind::TransferMinidumpReply + fn has_ancillary_data(&self) -> bool { + false } fn header(&self) -> Vec<u8> { @@ -280,10 +299,10 @@ impl Message for TransferMinidumpReply { data: &[u8], ancillary_data: Option<AncillaryData>, ) -> Result<TransferMinidumpReply, MessageError> { - debug_assert!( - ancillary_data.is_none(), - "TransferMinidumpReply messages cannot carry ancillary data" - ); + if ancillary_data.is_some() { + return Err(MessageError::UnexpectedAncillaryData); + } + let path_len_bytes: [u8; size_of::<usize>()] = data[0..size_of::<usize>()].try_into()?; let path_len = usize::from_ne_bytes(path_len_bytes); let offset = size_of::<usize>(); @@ -332,6 +351,13 @@ impl WindowsErrorReportingMinidump { context, } } +} + +#[cfg(target_os = "windows")] +impl Message for WindowsErrorReportingMinidump { + fn kind() -> Kind { + Kind::WindowsErrorReporting + } fn payload_size(&self) -> usize { (size_of::<Pid>() * 2) @@ -339,12 +365,9 @@ impl WindowsErrorReportingMinidump { + (size_of::<EXCEPTION_RECORD>() * self.exception_records.len()) + size_of::<CONTEXT>() } -} -#[cfg(target_os = "windows")] -impl Message for WindowsErrorReportingMinidump { - fn kind() -> Kind { - Kind::WindowsErrorReporting + fn has_ancillary_data(&self) -> bool { + false } fn header(&self) -> Vec<u8> { @@ -374,10 +397,10 @@ impl Message for WindowsErrorReportingMinidump { data: &[u8], ancillary_data: Option<AncillaryData>, ) -> Result<WindowsErrorReportingMinidump, MessageError> { - debug_assert!( - ancillary_data.is_none(), - "WindowsErrorReportingMinidump messages cannot carry ancillary data" - ); + if ancillary_data.is_some() { + return Err(MessageError::UnexpectedAncillaryData); + } + let bytes: [u8; size_of::<Pid>()] = data[0..size_of::<Pid>()].try_into()?; let pid = Pid::from_ne_bytes(bytes); let offset = size_of::<Pid>(); @@ -435,10 +458,6 @@ impl WindowsErrorReportingMinidumpReply { pub fn new() -> WindowsErrorReportingMinidumpReply { WindowsErrorReportingMinidumpReply {} } - - fn payload_size(&self) -> usize { - 0 - } } #[cfg(target_os = "windows")] @@ -447,6 +466,14 @@ impl Message for WindowsErrorReportingMinidumpReply { Kind::WindowsErrorReportingReply } + fn payload_size(&self) -> usize { + 0 + } + + fn has_ancillary_data(&self) -> bool { + false + } + fn header(&self) -> Vec<u8> { Header { kind: Self::kind(), @@ -463,7 +490,11 @@ impl Message for WindowsErrorReportingMinidumpReply { data: &[u8], ancillary_data: Option<AncillaryData>, ) -> Result<WindowsErrorReportingMinidumpReply, MessageError> { - if ancillary_data.is_some() || !data.is_empty() { + if ancillary_data.is_some() { + return Err(MessageError::UnexpectedAncillaryData); + } + + if !data.is_empty() { return Err(MessageError::InvalidData); } @@ -484,18 +515,22 @@ impl RegisterAuxvInfo { pub fn new(pid: Pid, auxv_info: DirectAuxvDumpInfo) -> RegisterAuxvInfo { RegisterAuxvInfo { pid, auxv_info } } +} + +#[cfg(any(target_os = "android", target_os = "linux"))] +impl Message for RegisterAuxvInfo { + fn kind() -> Kind { + Kind::RegisterAuxvInfo + } fn payload_size(&self) -> usize { // A bit hacky but we'll change this when we make // serialization/deserialization later. size_of::<Pid>() + (size_of::<AuxvType>() * 4) } -} -#[cfg(any(target_os = "android", target_os = "linux"))] -impl Message for RegisterAuxvInfo { - fn kind() -> Kind { - Kind::RegisterAuxvInfo + fn has_ancillary_data(&self) -> bool { + false } fn header(&self) -> Vec<u8> { @@ -513,7 +548,6 @@ impl Message for RegisterAuxvInfo { payload.extend(self.auxv_info.program_header_address.to_ne_bytes()); 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) } @@ -521,10 +555,9 @@ impl Message for RegisterAuxvInfo { data: &[u8], ancillary_data: Option<AncillaryData>, ) -> Result<RegisterAuxvInfo, MessageError> { - debug_assert!( - ancillary_data.is_none(), - "RegisterAuxvInfo messages cannot carry ancillary data" - ); + if ancillary_data.is_some() { + return Err(MessageError::UnexpectedAncillaryData); + } let bytes: [u8; size_of::<Pid>()] = data[0..size_of::<Pid>()].try_into()?; let pid = Pid::from_ne_bytes(bytes); @@ -573,10 +606,6 @@ impl UnregisterAuxvInfo { pub fn new(pid: Pid) -> UnregisterAuxvInfo { UnregisterAuxvInfo { pid } } - - fn payload_size(&self) -> usize { - size_of::<Pid>() - } } #[cfg(any(target_os = "android", target_os = "linux"))] @@ -585,6 +614,14 @@ impl Message for UnregisterAuxvInfo { Kind::UnregisterAuxvInfo } + fn payload_size(&self) -> usize { + size_of::<Pid>() + } + + fn has_ancillary_data(&self) -> bool { + false + } + fn header(&self) -> Vec<u8> { Header { kind: Self::kind(), @@ -596,7 +633,6 @@ impl Message for UnregisterAuxvInfo { 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, None) } @@ -604,10 +640,9 @@ impl Message for UnregisterAuxvInfo { data: &[u8], ancillary_data: Option<AncillaryData>, ) -> Result<UnregisterAuxvInfo, MessageError> { - debug_assert!( - ancillary_data.is_none(), - "UnregisterAuxvInfo messages cannot carry ancillary data" - ); + if ancillary_data.is_some() { + return Err(MessageError::UnexpectedAncillaryData); + } let bytes: [u8; size_of::<Pid>()] = data[0..size_of::<Pid>()].try_into()?; let pid = Pid::from_ne_bytes(bytes); @@ -626,8 +661,7 @@ impl Message for UnregisterAuxvInfo { * 10.15 implementation of Unix sockets which would sometimes fail to deliver * a message that would only contain control data and no buffer. See bug * 1989686 for more information. This dummy payload can be removed once bug - * 2002791 is implemented. - */ + * 2002791 is implemented. */ pub struct RegisterChildProcess { pub ipc_endpoint: AncillaryData, @@ -637,10 +671,6 @@ impl RegisterChildProcess { pub fn new(ipc_endpoint: AncillaryData) -> RegisterChildProcess { RegisterChildProcess { ipc_endpoint } } - - fn payload_size(&self) -> usize { - 1 - } } impl Message for RegisterChildProcess { @@ -648,6 +678,14 @@ impl Message for RegisterChildProcess { Kind::RegisterChildProcess } + fn payload_size(&self) -> usize { + 1 // HACK, see the comment above + } + + fn has_ancillary_data(&self) -> bool { + true + } + fn header(&self) -> Vec<u8> { Header { kind: Self::kind(), @@ -672,31 +710,40 @@ impl Message for RegisterChildProcess { } } -/* Reply sent from the crash helper process to a newly registered child. It - * contains platform-dependent information which is required for the child - * process to prepare itself for crash generation. */ +/* Message sent from the crash helper process to a newly registered child + * process. The child will prepare itself for being dumped by the crash helper + * after receiving this message, and then reply to inform the crash helper + * that it is now possible to dump it. */ -pub struct ChildProcessRegistered { +pub struct ChildProcessRendezVous { pub crash_helper_pid: Pid, } -impl ChildProcessRegistered { - pub fn new(pid: Pid) -> ChildProcessRegistered { - ChildProcessRegistered { +impl ChildProcessRendezVous { + pub fn new(pid: Pid) -> ChildProcessRendezVous { + ChildProcessRendezVous { crash_helper_pid: pid, } } } -impl Message for ChildProcessRegistered { +impl Message for ChildProcessRendezVous { fn kind() -> Kind { - Kind::ChildProcessRegistered + Kind::ChildProcessRendezVous + } + + fn payload_size(&self) -> usize { + size_of::<Pid>() + } + + fn has_ancillary_data(&self) -> bool { + false } fn header(&self) -> Vec<u8> { Header { kind: Self::kind(), - size: size_of::<Pid>(), + size: self.payload_size(), } .encode() } @@ -708,16 +755,88 @@ impl Message for ChildProcessRegistered { fn decode( data: &[u8], ancillary_data: Option<AncillaryData>, - ) -> Result<ChildProcessRegistered, MessageError> { - debug_assert!( - ancillary_data.is_none(), - "ChildProcessRegistered messages cannot carry ancillary data" - ); + ) -> Result<ChildProcessRendezVous, MessageError> { + if ancillary_data.is_some() { + return Err(MessageError::UnexpectedAncillaryData); + } + let bytes: [u8; size_of::<Pid>()] = data[0..size_of::<Pid>()].try_into()?; let pid = Pid::from_ne_bytes(bytes); - Ok(ChildProcessRegistered { + Ok(ChildProcessRendezVous { crash_helper_pid: pid, }) } } + +/* Reply sent by a child process to the crash helper process after receiving + * a ChildProcessRendezVous message. The message contains information on + * whether the child process managed to set itself up for being dumped, its + * PID plus platform-specific information that might be needed by the parent to + * dump it. */ + +pub struct ChildProcessRendezVousReply { + pub dumpable: bool, + pub child_pid: Pid, +} + +impl ChildProcessRendezVousReply { + pub fn new(dumpable: bool, child_pid: Pid) -> ChildProcessRendezVousReply { + ChildProcessRendezVousReply { + dumpable, + child_pid, + } + } +} + +impl Message for ChildProcessRendezVousReply { + fn kind() -> Kind { + Kind::ChildProcessRendezVousReply + } + + fn payload_size(&self) -> usize { + size_of::<u8>() + size_of::<Pid>() + } + + fn has_ancillary_data(&self) -> bool { + false + } + + fn header(&self) -> Vec<u8> { + Header { + kind: Self::kind(), + size: self.payload_size(), + } + .encode() + } + + fn into_payload(self) -> (Vec<u8>, Option<AncillaryData>) { + let mut payload = Vec::with_capacity(self.payload_size()); + payload.push(self.dumpable.into()); + payload.extend(self.child_pid.to_ne_bytes()); + debug_assert!(self.payload_size() == payload.len()); + (payload, None) + } + + fn decode( + data: &[u8], + ancillary_data: Option<AncillaryData>, + ) -> Result<ChildProcessRendezVousReply, MessageError> { + if ancillary_data.is_some() { + return Err(MessageError::UnexpectedAncillaryData); + } + + let dumpable_bytes: [u8; size_of::<u8>()] = data[0..size_of::<u8>()].try_into()?; + let dumpable = if dumpable_bytes[0] == 0 { false } else { true }; + let offset = size_of::<u8>(); + + let child_pid_bytes: [u8; size_of::<Pid>()] = + data[offset..offset + size_of::<Pid>()].try_into()?; + let child_pid = Pid::from_ne_bytes(child_pid_bytes); + + Ok(ChildProcessRendezVousReply { + dumpable, + child_pid, + }) + } +} diff --git a/toolkit/crashreporter/crash_helper_server/src/crash_generation/windows.rs b/toolkit/crashreporter/crash_helper_server/src/crash_generation/windows.rs @@ -32,7 +32,7 @@ use windows_sys::Win32::{ }; impl CrashGenerator { - pub(super) fn generate_wer_minidump( + pub(crate) fn generate_wer_minidump( &self, message: messages::WindowsErrorReportingMinidump, ) -> Result<(), ()> { diff --git a/toolkit/crashreporter/crash_helper_server/src/ipc_server.rs b/toolkit/crashreporter/crash_helper_server/src/ipc_server.rs @@ -19,16 +19,24 @@ pub enum IPCServerState { #[derive(PartialEq)] enum IPCEndpoint { - Parent, // A connection to the parent process - - Child, // A connection to the child process + /// A connection to the parent process + Parent, + /// A connection to the child process + Child, #[allow(dead_code)] - External, // A connection to an external process + /// A connection to an external process + External, } struct IPCConnection { + /// The platform-specific connector used for this connection connector: Rc<IPCConnector>, + /// The type of process on the other side of this connection endpoint: IPCEndpoint, + #[allow(dead_code)] + /// The pid of the Firefox process this is connected to. This is `None` + /// when this is a connection to an external process. + pid: Option<Pid>, } pub(crate) struct IPCServer { @@ -40,7 +48,11 @@ pub(crate) struct IPCServer { } impl IPCServer { - pub(crate) fn new(listener: IPCListener, connector: IPCConnector) -> Result<IPCServer> { + pub(crate) fn new( + client_pid: Pid, + listener: IPCListener, + connector: IPCConnector, + ) -> Result<IPCServer> { let connector = Rc::new(connector); let mut queue = IPCQueue::new(listener)?; queue.add_connector(&connector)?; @@ -51,6 +63,7 @@ impl IPCServer { IPCConnection { connector, endpoint: IPCEndpoint::Parent, + pid: Some(client_pid), }, ); @@ -68,6 +81,7 @@ impl IPCServer { IPCConnection { connector, endpoint: IPCEndpoint::External, + pid: None, }, ); } @@ -76,7 +90,7 @@ impl IPCServer { self.handle_message(key, &header, payload, ancillary_data, generator) { log::error!( - "Error {error} when handling a message of kind {:?}", + "Error {error:#} when handling a message of kind {:?}", header.kind ); } @@ -128,9 +142,14 @@ impl IPCServer { messages::Kind::RegisterChildProcess => { let message = messages::RegisterChildProcess::decode(&data, ancillary_data)?; let connector = IPCConnector::from_ancillary(message.ipc_endpoint)?; - connector.send_message(messages::ChildProcessRegistered::new( + connector.send_message(messages::ChildProcessRendezVous::new( process::id() as Pid ))?; + let reply = connector.recv_reply::<messages::ChildProcessRendezVousReply>()?; + + if !reply.dumpable { + bail!("Child process {} is not dumpable", reply.child_pid); + } let connector = Rc::new(connector); self.queue.add_connector(&connector)?; @@ -139,6 +158,7 @@ impl IPCServer { IPCConnection { connector, endpoint: IPCEndpoint::Child, + pid: Some(reply.child_pid), }, ); } @@ -163,8 +183,14 @@ impl IPCServer { #[cfg(target_os = "windows")] messages::Kind::WindowsErrorReporting => { let message = - messages::WindowsErrorReportingMinidump::decode(data, ancillary_data)?; - generator.generate_wer_minidump(message); + messages::WindowsErrorReportingMinidump::decode(&data, ancillary_data)?; + let res = generator.generate_wer_minidump(message); + match res { + Ok(_) => {} + Err(error) => log::error!( + "Could not generate a minidump requested via WER, error: {error:?}" + ), + } connector.send_message(messages::WindowsErrorReportingMinidumpReply::new())?; } kind => { diff --git a/toolkit/crashreporter/crash_helper_server/src/lib.rs b/toolkit/crashreporter/crash_helper_server/src/lib.rs @@ -11,11 +11,9 @@ mod ipc_server; mod logging; 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 crash_helper_common::{BreakpadData, BreakpadRawData, IPCConnector, IPCListener, Pid}; use std::{ ffi::{c_char, CStr, OsString}, fmt::Display, @@ -71,7 +69,7 @@ pub unsafe extern "C" fn crash_generator_logic_desktop( ); let ipc_server = unwrap_with_message( - IPCServer::new(listener, connector), + IPCServer::new(client_pid, listener, connector), "Could not create the IPC server", ); @@ -92,6 +90,7 @@ pub unsafe extern "C" fn crash_generator_logic_desktop( #[cfg(target_os = "android")] #[no_mangle] pub unsafe extern "C" fn crash_generator_logic_android( + pid: Pid, breakpad_data: BreakpadRawData, minidump_path: *const c_char, pipe: RawAncillaryData, @@ -121,7 +120,7 @@ pub unsafe extern "C" fn crash_generator_logic_android( "Could not use the pipe", ); let ipc_server = unwrap_with_message( - IPCServer::new(listener, connector), + IPCServer::new(pid, listener, connector), "Could not create the IPC server", ); main_loop(ipc_server, crash_generator)