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

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);
 }

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(
+      CrashHelper.crash_generator(breakpadFd.detachFd(), minidumpPath, serverFd.detachFd());
-          clientPid, breakpadFd.detachFd(), minidumpPath, listenFd.detachFd(), 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
+      // so we delegate those parts to native functions in
-      // functions in crashhelper_android.cpp.
+      // 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 =
+      final Pipes pipes = new Pipes(breakpad_client_fd, breakpad_server_fd, client_fd, server_fd);
-          new Pipes(breakpad_client_fd, breakpad_server_fd, listener_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(
+  protected static native void crash_generator(int breakpadFd, String minidumpPath, int serverFd);
-      int clientPid, int breakpadFd, String minidumpPath, int listenFd, int serverFd);
 
   protected static native boolean set_breakpad_opts(int breakpadFd);
-
-  protected static native boolean bind_and_listen(int listenFd);
 }

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(
+          CrashHelper.createConnection(pipes.mBreakpadServer, minidumps.getPath(), pipes.mServer),
-              pipes.mBreakpadServer, minidumps.getPath(), pipes.mListener, pipes.mServer),
           Context.BIND_AUTO_CREATE);
     } catch (final ClassNotFoundException e) {
       Log.w(LOGTAG, "Couldn't find the crash helper class");

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<Pid>(parse_int_or_exit(argv[1]));
+  Pid client_pid =
-  BreakpadRawData breakpad_data = parse_breakpad_data(argv[2]);
+      static_cast<Pid>(parse_int_or_exit(GET_CLIENT_PID_ARG(argv)));
-  char* minidump_path = argv[3];
+  BreakpadRawData breakpad_data =
-  char* listener = argv[4];
+      parse_breakpad_data(GET_BREAKPAD_DATA_ARG(argv));
-  char* connector = argv[5];
+  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);

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,
+    JNIEnv* jenv, jclass, jint breakpad_fd, jstring minidump_path,
-    jstring minidump_path, jint listen_fd, jint server_fd) {
+    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,
+  crash_generator_logic_android(breakpad_fd, minidump_path_str, server_fd);
-                                listen_fd, server_fd);
   jenv->ReleaseStringUTFChars(minidump_path, minidump_path_str);
 }

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::{
+use crash_helper_common::{ignore_eintr, BreakpadChar, BreakpadData, IPCChannel, IPCConnector};
-    ignore_eintr, BreakpadChar, BreakpadData, IPCChannel, IPCConnector, IPCListener,
-};
 use nix::{
     spawn::{posix_spawn, PosixSpawnAttr, PosixSpawnFileActions},
     sys::wait::waitpid,
@@ -25,12 +23,11 @@ impl CrashHelperClient {
         minidump_path: *const BreakpadChar,
     ) -> Result<CrashHelperClient> {
         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(),

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<OwnedHandle> {
         // 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<u16> = cmd_line.encode_wide().collect();
 

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<IPCConnector, IPCError> {
-        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()
     }
 

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 std::ffi::CStr;
-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 nix::sys::socket::{accept, bind, listen, Backlog};
+use crate::{errors::IPCError, Pid};
-use std::{
-    ffi::{CStr, CString},
-    os::fd::{AsFd, AsRawFd, BorrowedFd, FromRawFd, OwnedFd, RawFd},
-    str::FromStr,
-};
 
-pub struct IPCListener {
+pub struct IPCListener {}
-    socket: OwnedFd,
-}
 
 impl IPCListener {
-    /// Create a new listener with an address based on `pid`. The underlying
+    /// Create a new dummy listener. This is not used on Linux and macOS but
-    /// socket will not have the `FD_CLOEXEC` flag set and thus can be
+    /// we keep the type around so that the shared logic is the same as for
-    /// inherited by child processes.
+    /// Windows where this type is used.
-    pub fn new(pid: Pid) -> Result<IPCListener, IPCError> {
+    pub fn new(_pid: Pid) -> Result<IPCListener, IPCError> {
-        let socket = unix_socket().map_err(IPCError::System)?;
+        Ok(IPCListener {})
-        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<IPCListener, IPCError> {
-        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()
     }
 
     /// Deserialize a listener from an argument passed on the command-line.
-    /// The resulting listener is ready to accept new connections.
+    /// This produces a dummy listener and is only kept to provide shared logic
-    pub fn deserialize(string: &CStr, _pid: Pid) -> Result<IPCListener, IPCError> {
+    /// with Windows.
-        let string = string.to_str().map_err(|_e| IPCError::ParseError)?;
+    pub fn deserialize(_string: &CStr, _pid: Pid) -> Result<IPCListener, IPCError> {
-        let fd = RawFd::from_str(string).map_err(|_e| IPCError::ParseError)?;
+        Ok(IPCListener {})
-        // 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<IPCConnector, IPCError> {
-        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()
     }
 }

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

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;

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<OwnedFd> {
-    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<UnixAddr> {
-    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())
-}

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<OwnedFd> {
-    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<UnixAddr> {
-    // 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)
-}

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<IPCConnection>,
 }

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::{
+use nix::poll::{poll, PollFd, PollFlags, PollTimeout};
-    errno::Errno,
-    poll::{poll, PollFd, PollFlags, PollTimeout},
-};
 
 use super::IPCServer;
 
 impl IPCServer {
     pub fn wait_for_events(&mut self) -> Result<Vec<IPCEvent>, IPCError> {
-        let mut pollfds = Vec::with_capacity(1 + self.connections.len());
+        let mut pollfds = Vec::with_capacity(self.connections.len());
-        pollfds.push(PollFd::new(self.listener.as_raw_ref(), PollFlags::POLLIN));
         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));
-                    events.push(IPCEvent::Disconnect(index - 1));
+                // If a process was disconnected then skip all further
-                    // If a process was disconnected then skip all further
+                // processing of the socket. This wouldn't matter normally,
-                    // processing of the socket. This wouldn't matter normally,
+                // but on macOS calling recvmsg() on a hung-up socket seems
-                    // but on macOS calling recvmsg() on a hung-up socket seems
+                // to trigger a kernel panic, one we've already encountered
-                    // to trigger a kernel panic, one we've already encountered
+                // in the past. Doing things this way avoids the panic
-                    // in the past. Doing things this way avoids the panic
+                // while having no real downsides.
-                    // while having no real downsides.
+                continue;
-                    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));
-                }
             }
 
             if revents.contains(PollFlags::POLLIN) {
-                if index == 0 {
+                // SAFETY: The index is guaranteed to be >0 and within
-                    if let Ok(connector) = self.listener.accept() {
+                // the bounds of the connections array.
-                        events.push(IPCEvent::Connect(connector));
+                let connection = unsafe { self.connections.get_unchecked(index) };
-                    }
+                let header = connection.connector.recv_header();
-                } else {
+                if let Ok(header) = header {
-                    // SAFETY: The index is guaranteed to be >0 and within
+                    // Note that if we encounter a failure we don't propagate
-                    // the bounds of the connections array.
+                    // it, when the socket gets disconnected we'll get a
-                    let connection = unsafe { self.connections.get_unchecked(index - 1) };
+                    // POLLHUP event anyway so deal with disconnections there
-                    let header = connection.connector.recv_header();
+                    // instead of here.
-                    if let Ok(header) = header {
+                    events.push(IPCEvent::Header(index, 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));
-                    }
                 }
             }
 

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`
+/// `minidump_data` must point to valid, nul-terminated C strings. `server_pipe`
-/// and `server_pipe` must be valid file descriptors and `breakpad_data` must
+/// must be a valid file descriptor and `breakpad_data` must also be a valid
-/// also be a valid file descriptor compatible with Breakpad's crash generation
+/// file descriptor compatible with Breakpad's crash generation server.
-/// 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::new(0).unwrap();
-    let listener = IPCListener::from_fd(client_pid, listener)
-        .map_err(|error| {
-            log::error!("Could not use the listener (error: {error})");
-        })
-        .unwrap();
     let pipe = unsafe { OwnedFd::from_raw_fd(pipe) };
     let connector = IPCConnector::from_fd(pipe)
         .map_err(|error| {