From a2e0149356e6f1aa7afb7e2635bf1fc614541b72 Mon Sep 17 00:00:00 2001 From: Gabriele Svelto Date: Fri, 19 Sep 2025 02:54:04 +0000 Subject: [PATCH] Bug 1975979 - Remove the crash helper listener socket on Linux and macOS a=dmeehan Original Revision: https://phabricator.services.mozilla.com/D262410 Differential Revision: https://phabricator.services.mozilla.com/D263340 --- .../gecko/crashhelper/ICrashHelper.aidl | 2 +- .../gecko/crashhelper/CrashHelper.java | 35 ++------- .../org/mozilla/geckoview/GeckoRuntime.java | 3 +- .../crash_helper/crashhelper.cpp | 27 +++++-- .../crash_helper/crashhelper_android.cpp | 33 +------- .../crash_helper_client/src/platform/unix.rs | 10 +-- .../src/platform/windows.rs | 8 +- .../src/ipc_connector/unix.rs | 44 ++--------- .../src/ipc_listener/unix.rs | 77 +++---------------- .../crash_helper_common/src/lib.rs | 5 +- .../crash_helper_common/src/platform.rs | 4 +- .../crash_helper_common/src/platform/linux.rs | 23 +----- .../crash_helper_common/src/platform/macos.rs | 23 +----- .../crash_helper_server/src/ipc_server.rs | 1 + .../src/ipc_server/unix.rs | 56 +++++--------- .../crash_helper_server/src/lib.rs | 20 ++--- 16 files changed, 92 insertions(+), 279 deletions(-) diff --git a/mobile/android/geckoview/src/main/aidl/org/mozilla/gecko/crashhelper/ICrashHelper.aidl b/mobile/android/geckoview/src/main/aidl/org/mozilla/gecko/crashhelper/ICrashHelper.aidl index f1f150b97e07..dad36632294a 100644 --- a/mobile/android/geckoview/src/main/aidl/org/mozilla/gecko/crashhelper/ICrashHelper.aidl +++ b/mobile/android/geckoview/src/main/aidl/org/mozilla/gecko/crashhelper/ICrashHelper.aidl @@ -5,5 +5,5 @@ package org.mozilla.gecko.crashhelper; interface ICrashHelper { - boolean start(in int mainProcessPid, in ParcelFileDescriptor breakpadFd, in String minidumpPath, in ParcelFileDescriptor listenFd, in ParcelFileDescriptor serverFd); + boolean start(in ParcelFileDescriptor breakpadFd, in String minidumpPath, in ParcelFileDescriptor serverFd); } 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 index 16726c64db9c..76184b90688e 100644 --- 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,7 +13,6 @@ 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; @@ -43,10 +42,8 @@ public final class CrashHelper extends Service { private static class CrashHelperBinder extends ICrashHelper.Stub { @Override public boolean start( - final int clientPid, final ParcelFileDescriptor breakpadFd, final String minidumpPath, - final ParcelFileDescriptor listenFd, final ParcelFileDescriptor serverFd) { // Launch the crash helper code, this will spin out a thread which will // handle the IPC with Firefox' other processes. When running junit tests @@ -56,8 +53,7 @@ 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( - clientPid, breakpadFd.detachFd(), minidumpPath, listenFd.detachFd(), serverFd.detachFd()); + CrashHelper.crash_generator(breakpadFd.detachFd(), minidumpPath, serverFd.detachFd()); return false; } @@ -71,17 +67,14 @@ public final class CrashHelper extends Service { public static ServiceConnection createConnection( final ParcelFileDescriptor breakpadFd, final String minidumpPath, - final ParcelFileDescriptor listenFd, final ParcelFileDescriptor serverFd) { class CrashHelperConnection implements ServiceConnection { public CrashHelperConnection( final ParcelFileDescriptor breakpadFd, final String minidumpPath, - final ParcelFileDescriptor listenFd, final ParcelFileDescriptor serverFd) { mBreakpadFd = breakpadFd; mMinidumpPath = minidumpPath; - mListenFd = listenFd; mServerFd = serverFd; } @@ -89,7 +82,7 @@ public final class CrashHelper extends Service { public void onServiceConnected(final ComponentName name, final IBinder service) { final ICrashHelper helper = ICrashHelper.Stub.asInterface(service); try { - helper.start(Process.myPid(), mBreakpadFd, mMinidumpPath, mListenFd, mServerFd); + helper.start(mBreakpadFd, mMinidumpPath, mServerFd); } catch (final DeadObjectException e) { // The crash helper process died before we could start it, presumably // because of an out-of-memory condition. We don't attempt to restart @@ -107,24 +100,21 @@ public final class CrashHelper extends Service { ParcelFileDescriptor mBreakpadFd; String mMinidumpPath; - ParcelFileDescriptor mListenFd; ParcelFileDescriptor mServerFd; } - return new CrashHelperConnection(breakpadFd, minidumpPath, listenFd, serverFd); + return new CrashHelperConnection(breakpadFd, minidumpPath, serverFd); } public static final class Pipes { public final ParcelFileDescriptor mBreakpadClient; public final ParcelFileDescriptor mBreakpadServer; - public final ParcelFileDescriptor mListener; public final ParcelFileDescriptor mClient; public final ParcelFileDescriptor mServer; public Pipes( final FileDescriptor breakpadClientFd, final FileDescriptor breakpadServerFd, - final FileDescriptor listenerFd, final FileDescriptor clientFd, final FileDescriptor serverFd) throws IOException { @@ -133,10 +123,6 @@ public final class CrashHelper extends Service { if (!CrashHelper.set_breakpad_opts(mBreakpadServer.getFd())) { throw new IOException("Could not set the proper options on the Breakpad socket"); } - mListener = ParcelFileDescriptor.dup(listenerFd); - if (!CrashHelper.bind_and_listen(mListener.getFd())) { - throw new IOException("Could not listen on incoming connections"); - } mClient = ParcelFileDescriptor.dup(clientFd); mServer = ParcelFileDescriptor.dup(serverFd); } @@ -151,8 +137,8 @@ public final class CrashHelper extends Service { public static Pipes createCrashHelperPipes(final Context context) { try { // We can't set the required socket options for the Breakpad server socket - // or our own listener from here, so we delegate those parts to native - // functions in crashhelper_android.cpp. + // so we delegate those parts to native functions in + // crashhelper_android.cpp. GeckoLoader.doLoadLibrary(null, "crashhelper"); final FileDescriptor breakpad_client_fd = new FileDescriptor(); @@ -163,14 +149,11 @@ public final class CrashHelper extends Service { 0, breakpad_client_fd, breakpad_server_fd); - final FileDescriptor listener_fd = - Os.socket(OsConstants.AF_UNIX, OsConstants.SOCK_SEQPACKET, 0); final FileDescriptor client_fd = new FileDescriptor(); final FileDescriptor server_fd = new FileDescriptor(); Os.socketpair(OsConstants.AF_UNIX, OsConstants.SOCK_SEQPACKET, 0, client_fd, server_fd); - final Pipes pipes = - new Pipes(breakpad_client_fd, breakpad_server_fd, listener_fd, client_fd, server_fd); + final Pipes pipes = new Pipes(breakpad_client_fd, breakpad_server_fd, client_fd, server_fd); // Manually close all the file descriptors we created. // ParcelFileDescriptor instances in the Pipes object will close their @@ -178,7 +161,6 @@ public final class CrashHelper extends Service { // not, leaving us the job to clean them up. Os.close(breakpad_client_fd); Os.close(breakpad_server_fd); - Os.close(listener_fd); Os.close(client_fd); Os.close(server_fd); return pipes; @@ -192,10 +174,7 @@ 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 clientPid, int breakpadFd, String minidumpPath, int listenFd, int serverFd); + protected static native void crash_generator(int breakpadFd, String minidumpPath, int serverFd); protected static native boolean set_breakpad_opts(int breakpadFd); - - protected static native boolean bind_and_listen(int listenFd); } diff --git a/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntime.java b/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntime.java index a582fa17324f..3efb03ab001c 100644 --- a/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntime.java +++ b/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntime.java @@ -459,8 +459,7 @@ public final class GeckoRuntime implements Parcelable { final File minidumps = new File(context.getFilesDir(), "minidumps"); context.bindService( i, - CrashHelper.createConnection( - pipes.mBreakpadServer, minidumps.getPath(), pipes.mListener, pipes.mServer), + CrashHelper.createConnection(pipes.mBreakpadServer, minidumps.getPath(), pipes.mServer), Context.BIND_AUTO_CREATE); } catch (final ClassNotFoundException e) { Log.w(LOGTAG, "Couldn't find the crash helper class"); diff --git a/toolkit/crashreporter/crash_helper/crashhelper.cpp b/toolkit/crashreporter/crash_helper/crashhelper.cpp index 88f635a9b65f..f02fdcf80921 100644 --- a/toolkit/crashreporter/crash_helper/crashhelper.cpp +++ b/toolkit/crashreporter/crash_helper/crashhelper.cpp @@ -49,16 +49,31 @@ static void free_breakpad_data(BreakpadRawData aData) { #endif } +#define GET_CLIENT_PID_ARG(arguments) ((arguments)[1]) +#define GET_BREAKPAD_DATA_ARG(arguments) ((arguments)[2]) +#define GET_MINIDUMP_PATH_ARG(arguments) ((arguments)[3]) +#define GET_CONNECTOR_ARG(arguments) ((arguments)[4]) +#ifdef XP_WIN +# define GET_LISTENER_ARG(arguments) ((arguments)[5]) +# define ARG_NUM (6) +#else +static char sDummy[1] = ""; +# define GET_LISTENER_ARG(arguments) (sDummy) +# define ARG_NUM (5) +#endif // XP_WIN + int main(int argc, char* argv[]) { - if (argc < 6) { + if (argc < ARG_NUM) { exit(EXIT_FAILURE); } - Pid client_pid = static_cast(parse_int_or_exit(argv[1])); - BreakpadRawData breakpad_data = parse_breakpad_data(argv[2]); - char* minidump_path = argv[3]; - char* listener = argv[4]; - char* connector = argv[5]; + Pid client_pid = + static_cast(parse_int_or_exit(GET_CLIENT_PID_ARG(argv))); + BreakpadRawData breakpad_data = + parse_breakpad_data(GET_BREAKPAD_DATA_ARG(argv)); + char* minidump_path = GET_MINIDUMP_PATH_ARG(argv); + char* connector = GET_CONNECTOR_ARG(argv); + char* listener = GET_LISTENER_ARG(argv); int res = crash_generator_logic_desktop(client_pid, breakpad_data, minidump_path, listener, connector); diff --git a/toolkit/crashreporter/crash_helper/crashhelper_android.cpp b/toolkit/crashreporter/crash_helper/crashhelper_android.cpp index b9e66a5c5bc7..2fc5eb5eff77 100644 --- a/toolkit/crashreporter/crash_helper/crashhelper_android.cpp +++ b/toolkit/crashreporter/crash_helper/crashhelper_android.cpp @@ -30,36 +30,10 @@ Java_org_mozilla_gecko_crashhelper_CrashHelper_set_1breakpad_1opts( return true; } -extern "C" JNIEXPORT jboolean JNICALL -Java_org_mozilla_gecko_crashhelper_CrashHelper_bind_1and_1listen( - JNIEnv* jenv, jclass, jint listen_fd) { - struct sockaddr_un addr = { - .sun_family = AF_UNIX, - .sun_path = {}, - }; - - // The address' path deliberately starts with a null byte to inform the - // kernel that this is an abstract address and not an actual file path. - snprintf(addr.sun_path + 1, sizeof(addr.sun_path) - 2, - "gecko-crash-helper-pipe.%d", getpid()); - - int res = bind(listen_fd, (const struct sockaddr*)&addr, sizeof(addr)); - if (res < 0) { - return false; - } - - res = listen(listen_fd, 1); - if (res < 0) { - return false; - } - - return true; -} - extern "C" JNIEXPORT void JNICALL Java_org_mozilla_gecko_crashhelper_CrashHelper_crash_1generator( - JNIEnv* jenv, jclass, jint client_pid, jint breakpad_fd, - jstring minidump_path, jint listen_fd, jint server_fd) { + JNIEnv* jenv, jclass, 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 // pass it along to Breakpad. @@ -79,7 +53,6 @@ Java_org_mozilla_gecko_crashhelper_CrashHelper_crash_1generator( const char* minidump_path_str = jenv->GetStringUTFChars(minidump_path, nullptr); - crash_generator_logic_android(client_pid, breakpad_fd, minidump_path_str, - listen_fd, server_fd); + crash_generator_logic_android(breakpad_fd, minidump_path_str, server_fd); jenv->ReleaseStringUTFChars(minidump_path, minidump_path_str); } diff --git a/toolkit/crashreporter/crash_helper_client/src/platform/unix.rs b/toolkit/crashreporter/crash_helper_client/src/platform/unix.rs index 990e5ae4f993..f0d20ce493d3 100644 --- a/toolkit/crashreporter/crash_helper_client/src/platform/unix.rs +++ b/toolkit/crashreporter/crash_helper_client/src/platform/unix.rs @@ -3,9 +3,7 @@ * You can obtain one at http://mozilla.org/MPL/2.0/. */ use anyhow::Result; -use crash_helper_common::{ - ignore_eintr, BreakpadChar, BreakpadData, IPCChannel, IPCConnector, IPCListener, -}; +use crash_helper_common::{ignore_eintr, BreakpadChar, BreakpadData, IPCChannel, IPCConnector}; use nix::{ spawn::{posix_spawn, PosixSpawnAttr, PosixSpawnFileActions}, sys::wait::waitpid, @@ -25,12 +23,11 @@ impl CrashHelperClient { minidump_path: *const BreakpadChar, ) -> Result { let channel = IPCChannel::new()?; - let (listener, server_endpoint, client_endpoint) = channel.deconstruct(); + let (_listener, server_endpoint, client_endpoint) = channel.deconstruct(); CrashHelperClient::spawn_crash_helper( program, breakpad_data, minidump_path, - listener, server_endpoint, )?; @@ -45,7 +42,6 @@ impl CrashHelperClient { program: *const BreakpadChar, breakpad_data: BreakpadData, minidump_path: *const BreakpadChar, - listener: IPCListener, endpoint: IPCConnector, ) -> Result<()> { let parent_pid = getpid().to_string(); @@ -54,7 +50,6 @@ impl CrashHelperClient { let breakpad_data_arg = unsafe { CString::from_vec_unchecked(breakpad_data.to_string().into_bytes()) }; let minidump_path = unsafe { CStr::from_ptr(minidump_path) }; - let listener_arg = listener.serialize(); let endpoint_arg = endpoint.serialize(); let file_actions = PosixSpawnFileActions::init()?; @@ -74,7 +69,6 @@ impl CrashHelperClient { &parent_pid_arg, &breakpad_data_arg, minidump_path, - &listener_arg, &endpoint_arg, ], env.as_slice(), diff --git a/toolkit/crashreporter/crash_helper_client/src/platform/windows.rs b/toolkit/crashreporter/crash_helper_client/src/platform/windows.rs index a7268a1c7d36..36a83d741668 100644 --- a/toolkit/crashreporter/crash_helper_client/src/platform/windows.rs +++ b/toolkit/crashreporter/crash_helper_client/src/platform/windows.rs @@ -44,8 +44,8 @@ impl CrashHelperClient { program, breakpad_data, minidump_path, - listener, server_endpoint, + listener, ) }); @@ -60,8 +60,8 @@ impl CrashHelperClient { program: OsString, breakpad_data: BreakpadData, minidump_path: OsString, - listener: IPCListener, endpoint: IPCConnector, + listener: IPCListener, ) -> Result { // SAFETY: `GetCurrentProcessId()` takes no arguments and should always work let pid = OsString::from(unsafe { GetCurrentProcessId() }.to_string()); @@ -74,9 +74,9 @@ impl CrashHelperClient { cmd_line.push(" "); cmd_line.push(escape_cmd_line_arg(&minidump_path)); cmd_line.push(" "); - cmd_line.push(escape_cmd_line_arg(&listener.serialize())); - cmd_line.push(" "); cmd_line.push(escape_cmd_line_arg(&endpoint.serialize())); + cmd_line.push(" "); + cmd_line.push(escape_cmd_line_arg(&listener.serialize())); cmd_line.push("\0"); let mut cmd_line: Vec = cmd_line.encode_wide().collect(); diff --git a/toolkit/crashreporter/crash_helper_common/src/ipc_connector/unix.rs b/toolkit/crashreporter/crash_helper_common/src/ipc_connector/unix.rs index 438240f507b0..c5bc0d628882 100644 --- a/toolkit/crashreporter/crash_helper_common/src/ipc_connector/unix.rs +++ b/toolkit/crashreporter/crash_helper_common/src/ipc_connector/unix.rs @@ -4,19 +4,19 @@ #[cfg(any(target_os = "android", target_os = "linux"))] use crate::platform::linux::{ - server_addr, set_socket_cloexec, set_socket_default_flags, unix_socket, + set_socket_cloexec, set_socket_default_flags, }; #[cfg(target_os = "macos")] use crate::platform::macos::{ - server_addr, set_socket_cloexec, set_socket_default_flags, unix_socket, + set_socket_cloexec, set_socket_default_flags, }; -use crate::{ignore_eintr, Pid, ProcessHandle, IO_TIMEOUT}; +use crate::{ignore_eintr, ProcessHandle, IO_TIMEOUT}; use nix::{ cmsg_space, errno::Errno, poll::{poll, PollFd, PollFlags, PollTimeout}, - sys::socket::{connect, recvmsg, sendmsg, ControlMessage, ControlMessageOwned, MsgFlags}, + sys::socket::{recvmsg, sendmsg, ControlMessage, ControlMessageOwned, MsgFlags}, }; use std::{ ffi::{CStr, CString}, @@ -61,40 +61,6 @@ impl IPCConnector { IPCConnector::from_fd(unsafe { OwnedFd::from_raw_fd(ancillary_data) }) } - /// Create a new connector by connecting it to the process specified by - /// `pid`. 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 connect(pid: Pid) -> Result { - let socket = unix_socket().map_err(IPCError::ConnectionFailure)?; - set_socket_default_flags(socket.as_fd()).map_err(IPCError::ConnectionFailure)?; - set_socket_cloexec(socket.as_fd()).map_err(IPCError::ConnectionFailure)?; - - let server_addr = server_addr(pid).map_err(IPCError::ConnectionFailure)?; - - loop { - let timeout = PollTimeout::from(IO_TIMEOUT); - let res = ignore_eintr!(poll( - &mut [PollFd::new(socket.as_fd(), PollFlags::POLLOUT)], - timeout - )); - match res { - Err(e) => return Err(IPCError::ConnectionFailure(e)), - Ok(_res @ 0) => return Err(IPCError::ConnectionFailure(Errno::ETIMEDOUT)), - Ok(_) => {} - } - - let res = ignore_eintr!(connect(socket.as_raw_fd(), &server_addr)); - match res { - Ok(_) => break, - Err(_e @ Errno::EAGAIN) => continue, // Retry, the helper might not be ready yet - Err(e) => return Err(IPCError::ConnectionFailure(e)), - } - } - - Ok(IPCConnector { socket }) - } - /// 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. @@ -131,7 +97,7 @@ impl IPCConnector { Ok(self.raw_fd()) } - pub fn as_raw_ref(&self) -> BorrowedFd { + pub fn as_raw_ref(&self) -> BorrowedFd<'_> { self.socket.as_fd() } diff --git a/toolkit/crashreporter/crash_helper_common/src/ipc_listener/unix.rs b/toolkit/crashreporter/crash_helper_common/src/ipc_listener/unix.rs index 3f7bbaac1001..e02fa63035c6 100644 --- a/toolkit/crashreporter/crash_helper_common/src/ipc_listener/unix.rs +++ b/toolkit/crashreporter/crash_helper_common/src/ipc_listener/unix.rs @@ -2,77 +2,24 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ -#[cfg(any(target_os = "android", target_os = "linux"))] -use crate::platform::linux::{ - server_addr, set_socket_cloexec, set_socket_default_flags, unix_socket, -}; -#[cfg(target_os = "macos")] -use crate::platform::macos::{ - server_addr, set_socket_cloexec, set_socket_default_flags, unix_socket, -}; -use crate::{errors::IPCError, IPCConnector, Pid}; +use std::ffi::CStr; -use nix::sys::socket::{accept, bind, listen, Backlog}; -use std::{ - ffi::{CStr, CString}, - os::fd::{AsFd, AsRawFd, BorrowedFd, FromRawFd, OwnedFd, RawFd}, - str::FromStr, -}; +use crate::{errors::IPCError, Pid}; -pub struct IPCListener { - socket: OwnedFd, -} +pub struct IPCListener {} impl IPCListener { - /// Create a new listener with an address based on `pid`. The underlying - /// socket will not have the `FD_CLOEXEC` flag set and thus can be - /// inherited by child processes. - pub fn new(pid: Pid) -> Result { - let socket = unix_socket().map_err(IPCError::System)?; - set_socket_default_flags(socket.as_fd()).map_err(IPCError::System)?; - - let server_addr = server_addr(pid).map_err(IPCError::System)?; - bind(socket.as_fd().as_raw_fd(), &server_addr).map_err(IPCError::System)?; - listen(&socket, Backlog::new(1).unwrap()).map_err(IPCError::ListenFailed)?; - - Ok(IPCListener { socket }) - } - - /// Create a new listener using an already prepared socket. The listener - /// must have been bound to the appropriate address and should already be - /// listening on incoming connections. This will set the `FD_CLOEXEC` flag - /// on the underlying socket and thus will make this litener not inheritable - /// by child processes. - pub fn from_fd(_pid: Pid, socket: OwnedFd) -> Result { - set_socket_cloexec(socket.as_fd()).map_err(IPCError::System)?; - - Ok(IPCListener { socket }) - } - - /// Serialize this listener into a string that can be passed on the - /// command-line to a child process. This only works for newly - /// created listeners because they are explicitly created as inheritable. - pub fn serialize(&self) -> CString { - CString::new(self.socket.as_raw_fd().to_string()).unwrap() + /// Create a new dummy listener. This is not used on Linux and macOS but + /// we keep the type around so that the shared logic is the same as for + /// Windows where this type is used. + pub fn new(_pid: Pid) -> Result { + Ok(IPCListener {}) } /// Deserialize a listener from an argument passed on the command-line. - /// The resulting listener is ready to accept new connections. - pub fn deserialize(string: &CStr, _pid: Pid) -> Result { - let string = string.to_str().map_err(|_e| IPCError::ParseError)?; - let fd = RawFd::from_str(string).map_err(|_e| IPCError::ParseError)?; - // SAFETY: This is a file descriptor we passed in ourselves. - let socket = unsafe { OwnedFd::from_raw_fd(fd) }; - Ok(IPCListener { socket }) - } - - pub fn accept(&self) -> Result { - let socket = accept(self.socket.as_fd().as_raw_fd()).map_err(IPCError::AcceptFailed)?; - // SAFETY: `socket` is guaranteed to be valid at this point. - IPCConnector::from_fd(unsafe { OwnedFd::from_raw_fd(socket) }) - } - - pub fn as_raw_ref(&self) -> BorrowedFd { - self.socket.as_fd() + /// This produces a dummy listener and is only kept to provide shared logic + /// with Windows. + pub fn deserialize(_string: &CStr, _pid: Pid) -> Result { + Ok(IPCListener {}) } } diff --git a/toolkit/crashreporter/crash_helper_common/src/lib.rs b/toolkit/crashreporter/crash_helper_common/src/lib.rs index 5fe5a4a7bd1f..8b08068695b7 100644 --- a/toolkit/crashreporter/crash_helper_common/src/lib.rs +++ b/toolkit/crashreporter/crash_helper_common/src/lib.rs @@ -20,7 +20,10 @@ 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_listener::IPCListener; -pub use crate::platform::{server_addr, ProcessHandle}; +pub use crate::platform::ProcessHandle; + +#[cfg(target_os = "windows")] +pub use crate::platform::server_addr; /// OsString extensions to convert from/to C strings. The strings will be /// regular nul-terminated byte strings on most platforms but will use wide diff --git a/toolkit/crashreporter/crash_helper_common/src/platform.rs b/toolkit/crashreporter/crash_helper_common/src/platform.rs index 27ad3dbf5261..9a6ec1ce469c 100644 --- a/toolkit/crashreporter/crash_helper_common/src/platform.rs +++ b/toolkit/crashreporter/crash_helper_common/src/platform.rs @@ -9,13 +9,13 @@ pub use windows::{server_addr, ProcessHandle}; pub(crate) mod windows; #[cfg(any(target_os = "android", target_os = "linux"))] -pub use linux::{server_addr, ProcessHandle}; +pub use linux::ProcessHandle; #[cfg(any(target_os = "android", target_os = "linux"))] pub(crate) mod linux; #[cfg(target_os = "macos")] -pub use macos::{server_addr, ProcessHandle}; +pub use macos::ProcessHandle; #[cfg(target_os = "macos")] pub(crate) mod macos; diff --git a/toolkit/crashreporter/crash_helper_common/src/platform/linux.rs b/toolkit/crashreporter/crash_helper_common/src/platform/linux.rs index 1d5171223f2b..2b5201acf0de 100644 --- a/toolkit/crashreporter/crash_helper_common/src/platform/linux.rs +++ b/toolkit/crashreporter/crash_helper_common/src/platform/linux.rs @@ -2,33 +2,21 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ -use crate::Pid; - use nix::{ fcntl::{ fcntl, FcntlArg::{F_GETFL, F_SETFD, F_SETFL}, FdFlag, OFlag, }, - sys::socket::{socket, socketpair, AddressFamily, SockFlag, SockType, UnixAddr}, + sys::socket::{socketpair, AddressFamily, SockFlag, SockType}, Result, }; use std::{ - env, os::fd::{BorrowedFd, OwnedFd}, }; pub type ProcessHandle = (); -pub(crate) fn unix_socket() -> Result { - socket( - AddressFamily::Unix, - SockType::SeqPacket, - SockFlag::empty(), - None, - ) -} - pub(crate) fn unix_socketpair() -> Result<(OwnedFd, OwnedFd)> { socketpair( AddressFamily::Unix, @@ -47,12 +35,3 @@ pub(crate) fn set_socket_default_flags(socket: BorrowedFd) -> Result<()> { pub(crate) fn set_socket_cloexec(socket: BorrowedFd) -> Result<()> { fcntl(socket, F_SETFD(FdFlag::FD_CLOEXEC)).map(|_res| ()) } - -pub fn server_addr(pid: Pid) -> Result { - let server_name = if let Ok(snap_instance_name) = env::var("SNAP_INSTANCE_NAME") { - format!("snap.{snap_instance_name:}.gecko-crash-helper-pipe.{pid:}") - } else { - format!("gecko-crash-helper-pipe.{pid:}") - }; - UnixAddr::new_abstract(server_name.as_bytes()) -} diff --git a/toolkit/crashreporter/crash_helper_common/src/platform/macos.rs b/toolkit/crashreporter/crash_helper_common/src/platform/macos.rs index 4589876e070f..f79b82bec421 100644 --- a/toolkit/crashreporter/crash_helper_common/src/platform/macos.rs +++ b/toolkit/crashreporter/crash_helper_common/src/platform/macos.rs @@ -2,8 +2,6 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ -use crate::Pid; - use nix::{ errno::Errno, fcntl::{ @@ -12,27 +10,16 @@ use nix::{ FdFlag, OFlag, }, libc::{setsockopt, SOL_SOCKET, SO_NOSIGPIPE}, - sys::socket::{socket, socketpair, AddressFamily, SockFlag, SockType, UnixAddr}, + sys::socket::{socketpair, AddressFamily, SockFlag, SockType}, Result, }; use std::{ mem::size_of, os::fd::{AsRawFd, BorrowedFd, OwnedFd}, - path::PathBuf, - str::FromStr, }; pub type ProcessHandle = (); -pub(crate) fn unix_socket() -> Result { - socket( - AddressFamily::Unix, - SockType::Stream, - SockFlag::empty(), - None, - ) -} - pub(crate) fn unix_socketpair() -> Result<(OwnedFd, OwnedFd)> { socketpair( AddressFamily::Unix, @@ -70,11 +57,3 @@ pub(crate) fn set_socket_default_flags(socket: BorrowedFd) -> Result<()> { pub(crate) fn set_socket_cloexec(socket: BorrowedFd) -> Result<()> { fcntl(socket, F_SETFD(FdFlag::FD_CLOEXEC)).map(|_res| ()) } - -pub fn server_addr(pid: Pid) -> Result { - // macOS doesn't seem to support abstract paths as addresses for Unix - // protocol sockets, so this needs to be the path of an actual file. - let server_name = format!("/tmp/gecko-crash-helper-pipe.{pid:}"); - let server_path = PathBuf::from_str(&server_name).unwrap(); - UnixAddr::new(&server_path) -} diff --git a/toolkit/crashreporter/crash_helper_server/src/ipc_server.rs b/toolkit/crashreporter/crash_helper_server/src/ipc_server.rs index 701191c25a45..1f2274560f5c 100644 --- a/toolkit/crashreporter/crash_helper_server/src/ipc_server.rs +++ b/toolkit/crashreporter/crash_helper_server/src/ipc_server.rs @@ -32,6 +32,7 @@ struct IPCConnection { } pub(crate) struct IPCServer { + #[cfg_attr(unix, allow(dead_code))] listener: IPCListener, connections: Vec, } diff --git a/toolkit/crashreporter/crash_helper_server/src/ipc_server/unix.rs b/toolkit/crashreporter/crash_helper_server/src/ipc_server/unix.rs index 612472d239e0..613d0ba3e4bf 100644 --- a/toolkit/crashreporter/crash_helper_server/src/ipc_server/unix.rs +++ b/toolkit/crashreporter/crash_helper_server/src/ipc_server/unix.rs @@ -3,17 +3,13 @@ * You can obtain one at http://mozilla.org/MPL/2.0/. */ use crash_helper_common::{errors::IPCError, ignore_eintr, IPCEvent}; -use nix::{ - errno::Errno, - poll::{poll, PollFd, PollFlags, PollTimeout}, -}; +use nix::poll::{poll, PollFd, PollFlags, PollTimeout}; use super::IPCServer; impl IPCServer { pub fn wait_for_events(&mut self) -> Result, IPCError> { - let mut pollfds = Vec::with_capacity(1 + self.connections.len()); - pollfds.push(PollFd::new(self.listener.as_raw_ref(), PollFlags::POLLIN)); + let mut pollfds = Vec::with_capacity(self.connections.len()); pollfds.extend( self.connections.iter().map(|connection| { PollFd::new(connection.connector.as_raw_ref(), PollFlags::POLLIN) @@ -31,39 +27,27 @@ impl IPCServer { let revents = pollfd.revents().unwrap(); if revents.contains(PollFlags::POLLHUP) { - if index > 0 { - events.push(IPCEvent::Disconnect(index - 1)); - // If a process was disconnected then skip all further - // processing of the socket. This wouldn't matter normally, - // but on macOS calling recvmsg() on a hung-up socket seems - // to trigger a kernel panic, one we've already encountered - // in the past. Doing things this way avoids the panic - // while having no real downsides. - continue; - } else { - // This should never happen, unless the listener socket was - // not set up properly or a failure happened during setup. - return Err(IPCError::System(Errno::EFAULT)); - } + events.push(IPCEvent::Disconnect(index)); + // If a process was disconnected then skip all further + // processing of the socket. This wouldn't matter normally, + // but on macOS calling recvmsg() on a hung-up socket seems + // to trigger a kernel panic, one we've already encountered + // in the past. Doing things this way avoids the panic + // while having no real downsides. + continue; } if revents.contains(PollFlags::POLLIN) { - if index == 0 { - if let Ok(connector) = self.listener.accept() { - events.push(IPCEvent::Connect(connector)); - } - } else { - // SAFETY: The index is guaranteed to be >0 and within - // the bounds of the connections array. - let connection = unsafe { self.connections.get_unchecked(index - 1) }; - let header = connection.connector.recv_header(); - if let Ok(header) = header { - // Note that if we encounter a failure we don't propagate - // it, when the socket gets disconnected we'll get a - // POLLHUP event anyway so deal with disconnections there - // instead of here. - events.push(IPCEvent::Header(index - 1, header)); - } + // SAFETY: The index is guaranteed to be >0 and within + // the bounds of the connections array. + let connection = unsafe { self.connections.get_unchecked(index) }; + let header = connection.connector.recv_header(); + if let Ok(header) = header { + // Note that if we encounter a failure we don't propagate + // it, when the socket gets disconnected we'll get a + // POLLHUP event anyway so deal with disconnections there + // instead of here. + events.push(IPCEvent::Header(index, header)); } } diff --git a/toolkit/crashreporter/crash_helper_server/src/lib.rs b/toolkit/crashreporter/crash_helper_server/src/lib.rs index b3062d069675..db44a4871335 100644 --- a/toolkit/crashreporter/crash_helper_server/src/lib.rs +++ b/toolkit/crashreporter/crash_helper_server/src/lib.rs @@ -11,7 +11,9 @@ mod ipc_server; mod logging; mod phc; -use crash_helper_common::{BreakpadData, BreakpadRawData, IPCConnector, IPCListener, Pid}; +#[cfg(not(target_os = "android"))] +use crash_helper_common::Pid; +use crash_helper_common::{BreakpadData, BreakpadRawData, IPCConnector, IPCListener}; use std::ffi::{c_char, CStr, OsString}; use crash_generation::CrashGenerator; @@ -82,17 +84,14 @@ pub unsafe extern "C" fn crash_generator_logic_desktop( /// /// # Safety /// -/// `minidump_data` must point to valid, nul-terminated C strings. `listener` -/// and `server_pipe` must be valid file descriptors and `breakpad_data` must -/// also be a valid file descriptor compatible with Breakpad's crash generation -/// server. +/// `minidump_data` must point to valid, nul-terminated C strings. `server_pipe` +/// must be a valid file descriptor and `breakpad_data` must also be a valid +/// file descriptor compatible with Breakpad's crash generation server. #[cfg(target_os = "android")] #[no_mangle] pub unsafe extern "C" fn crash_generator_logic_android( - client_pid: Pid, breakpad_data: BreakpadRawData, minidump_path: *const c_char, - listener: RawFd, pipe: RawFd, ) { logging::init(); @@ -110,12 +109,7 @@ pub unsafe extern "C" fn crash_generator_logic_android( }) .unwrap(); - let listener = unsafe { OwnedFd::from_raw_fd(listener) }; - let listener = IPCListener::from_fd(client_pid, listener) - .map_err(|error| { - log::error!("Could not use the listener (error: {error})"); - }) - .unwrap(); + let listener = IPCListener::new(0).unwrap(); let pipe = unsafe { OwnedFd::from_raw_fd(pipe) }; let connector = IPCConnector::from_fd(pipe) .map_err(|error| {