Use sh to handle SIGUSR1 and replace fork+exec with spawn

src/xwayland/launch_helper.rs

@@ -1,225 +0,0 @@
-use std::{
-    io::{Error as IOError, Read, Result as IOResult, Write},
-    os::unix::{
-        io::{AsRawFd, FromRawFd, RawFd},
-        net::UnixStream,
-    },
-};
-
-use nix::{
-    errno::Errno,
-    libc::exit,
-    sys::{signal, socket, uio},
-    unistd::{fork, ForkResult},
-    Error as NixError, Result as NixResult,
-};
-
-use super::xserver::exec_xwayland;
-
-const MAX_FDS: usize = 10;
-
-/// A handle to a `fork()`'d child process that is used for starting XWayland
-pub struct LaunchHelper(UnixStream);
-
-impl LaunchHelper {
-    /// Start a new launch helper process.
-    ///
-    /// # Safety
-    ///
-    /// This function calls [`nix::unistd::fork`] to create a new process. This function is unsafe
-    /// because `fork` is unsafe. Most importantly: Calling `fork` in a multi-threaded process has
-    /// lots of limitations. Thus, you must call this function before any threads are created.
-    pub unsafe fn fork() -> IOResult<Self> {
-        let (child, me) = UnixStream::pair()?;
-        match fork().map_err(nix_error_to_io)? {
-            ForkResult::Child => {
-                drop(me);
-                match do_child(child) {
-                    Ok(()) => exit(0),
-                    Err(e) => {
-                        eprintln!("Error in smithay fork child: {:?}", e);
-                        exit(1);
-                    }
-                }
-            }
-            ForkResult::Parent { child: _ } => Ok(Self(me)),
-        }
-    }
-
-    /// Get access to an fd that becomes readable when a launch finished. Call
-    /// `was_launch_succesful()` once this happens.
-    pub(crate) fn status_fd(&self) -> RawFd {
-        self.0.as_raw_fd()
-    }
-
-    /// Check the status of a previous launch.
-    pub(crate) fn was_launch_succesful(&self) -> IOResult<bool> {
-        // This reads the one byte that is written at the end of do_child()
-        let mut buffer = [0];
-        let len = (&mut &self.0).read(&mut buffer)?;
-        Ok(len > 0 && buffer[0] != 0)
-    }
-
-    /// Fork a child and call exec_xwayland() in it.
-    pub(crate) fn launch(
-        &self,
-        display: u32,
-        wayland_socket: UnixStream,
-        wm_socket: UnixStream,
-        listen_sockets: &[UnixStream],
-    ) -> IOResult<()> {
-        let buffer = display.to_ne_bytes();
-        let mut fds = vec![wayland_socket.as_raw_fd(), wm_socket.as_raw_fd()];
-        fds.extend(listen_sockets.iter().map(|s| s.as_raw_fd()));
-        assert!(fds.len() <= MAX_FDS);
-        send_with_fds(self.0.as_raw_fd(), &buffer, &fds)
-    }
-}
-
-fn do_child(mut stream: UnixStream) -> IOResult<()> {
-    use nix::sys::wait;
-
-    let mut display = [0; 4];
-    let mut fds = [0; MAX_FDS];
-
-    // Block signals. SIGUSR1 being blocked is inherited by Xwayland and makes it signal its parent
-    let mut set = signal::SigSet::empty();
-    set.add(signal::Signal::SIGUSR1);
-    set.add(signal::Signal::SIGCHLD);
-    signal::sigprocmask(signal::SigmaskHow::SIG_BLOCK, Some(&set), None).map_err(nix_error_to_io)?;
-
-    loop {
-        while wait::waitpid(None, Some(wait::WaitPidFlag::WNOHANG)).is_ok() {
-            // We just want to reap the zombies
-        }
-
-        // Receive a new command: u32 display number and associated FDs
-        let (bytes, num_fds) = receive_fds(stream.as_raw_fd(), &mut display, &mut fds)?;
-        if bytes == 0 {
-            // End of file => our parent exited => we should do the same
-            break Ok(());
-        }
-
-        assert_eq!(bytes, 4);
-        let display = u32::from_ne_bytes(display);
-
-        // Wrap the FDs so that they are later closed.
-        assert!(num_fds >= 2);
-        let wayland_socket = unsafe { UnixStream::from_raw_fd(fds[0]) };
-        let wm_socket = unsafe { UnixStream::from_raw_fd(fds[1]) };
-        let listen_sockets = fds[2..num_fds]
-            .iter()
-            .map(|fd| unsafe { UnixStream::from_raw_fd(*fd) })
-            .collect::<Vec<_>>();
-
-        // Fork Xwayland and report back the result
-        let success = match fork_xwayland(display, wayland_socket, wm_socket, &listen_sockets) {
-            Ok(true) => 1,
-            Ok(false) => {
-                eprintln!("Xwayland failed to start");
-                0
-            }
-            Err(e) => {
-                eprintln!("Failed to fork Xwayland: {:?}", e);
-                0
-            }
-        };
-        stream.write_all(&[success])?;
-    }
-}
-
-/// fork() a child process and execute XWayland via exec_xwayland()
-fn fork_xwayland(
-    display: u32,
-    wayland_socket: UnixStream,
-    wm_socket: UnixStream,
-    listen_sockets: &[UnixStream],
-) -> NixResult<bool> {
-    match unsafe { fork()? } {
-        ForkResult::Parent { child: _ } => {
-            // Wait for the child process to exit or send SIGUSR1
-            let mut set = signal::SigSet::empty();
-            set.add(signal::Signal::SIGUSR1);
-            set.add(signal::Signal::SIGCHLD);
-            match set.wait()? {
-                signal::Signal::SIGUSR1 => Ok(true),
-                _ => Ok(false),
-            }
-        }
-        ForkResult::Child => {
-            match exec_xwayland(display, wayland_socket, wm_socket, listen_sockets) {
-                Ok(x) => match x {},
-                Err(e) => {
-                    // Well, what can we do? Our parent will get SIGCHLD when we exit.
-                    eprintln!("exec Xwayland failed: {:?}", e);
-                    unsafe { exit(1) };
-                }
-            }
-        }
-    }
-}
-
-/// Wrapper around `sendmsg()` for FD-passing
-fn send_with_fds(fd: RawFd, bytes: &[u8], fds: &[RawFd]) -> IOResult<()> {
-    let iov = [uio::IoVec::from_slice(bytes)];
-    loop {
-        let result = if !fds.is_empty() {
-            let cmsgs = [socket::ControlMessage::ScmRights(fds)];
-            socket::sendmsg(fd.as_raw_fd(), &iov, &cmsgs, socket::MsgFlags::empty(), None)
-        } else {
-            socket::sendmsg(fd.as_raw_fd(), &iov, &[], socket::MsgFlags::empty(), None)
-        };
-        match result {
-            Ok(len) => {
-                // All data should have been sent. Why would it fail!?
-                assert_eq!(len, bytes.len());
-                return Ok(());
-            }
-            Err(NixError::Sys(Errno::EINTR)) => {
-                // Try again
-            }
-            Err(e) => return Err(nix_error_to_io(e)),
-        }
-    }
-}
-
-/// Wrapper around `recvmsg()` for FD-passing
-fn receive_fds(fd: RawFd, buffer: &mut [u8], fds: &mut [RawFd]) -> IOResult<(usize, usize)> {
-    let mut cmsg = cmsg_space!([RawFd; MAX_FDS]);
-    let iov = [uio::IoVec::from_mut_slice(buffer)];
-
-    let msg = loop {
-        match socket::recvmsg(
-            fd.as_raw_fd(),
-            &iov[..],
-            Some(&mut cmsg),
-            socket::MsgFlags::empty(),
-        ) {
-            Ok(msg) => break msg,
-            Err(NixError::Sys(Errno::EINTR)) => {
-                // Try again
-            }
-            Err(e) => return Err(nix_error_to_io(e)),
-        }
-    };
-
-    let received_fds = msg.cmsgs().flat_map(|cmsg| match cmsg {
-        socket::ControlMessageOwned::ScmRights(s) => s,
-        _ => Vec::new(),
-    });
-    let mut fd_count = 0;
-    for (fd, place) in received_fds.zip(fds.iter_mut()) {
-        fd_count += 1;
-        *place = fd;
-    }
-    Ok((msg.bytes, fd_count))
-}
-
-fn nix_error_to_io(err: NixError) -> IOError {
-    use std::io::ErrorKind;
-    match err {
-        NixError::Sys(errno) => errno.into(),
-        NixError::InvalidPath | NixError::InvalidUtf8 => IOError::new(ErrorKind::InvalidInput, err),
-        NixError::UnsupportedOperation => IOError::new(ErrorKind::Other, err),
-    }
-}

src/xwayland/mod.rs

@@ -13,9 +13,7 @@
 //!
 //! Smithay does not provide any helper for doing that yet, but it is planned.
 
-mod launch_helper;
 mod x11_sockets;
 mod xserver;
 
-pub use self::launch_helper::LaunchHelper;
 pub use self::xserver::{XWayland, XWindowManager};

src/xwayland/xserver.rs

@@ -33,11 +33,13 @@ use std::{
     any::Any,
     cell::RefCell,
     env,
-    ffi::CString,
+    io::{Error as IOError, Read, Result as IOResult},
     os::unix::{
         io::{AsRawFd, IntoRawFd, RawFd},
         net::UnixStream,
+        process::CommandExt,
     },
+    process::{ChildStdout, Command, Stdio},
     rc::Rc,
     sync::Arc,
 };
@@ -47,14 +49,11 @@ use calloop::{
     Interest, LoopHandle, Mode, Source,
 };
 
-use nix::Result as NixResult;
+use nix::Error as NixError;
 
 use wayland_server::{Client, Display, Filter};
 
-use super::{
+use super::x11_sockets::{prepare_x11_sockets, X11Lock};
-    x11_sockets::{prepare_x11_sockets, X11Lock},
-    LaunchHelper,
-};
 
 /// The XWayland handle
 pub struct XWayland<WM: XWindowManager> {
@@ -87,7 +86,6 @@ impl<WM: XWindowManager + 'static> XWayland<WM> {
         display: Rc<RefCell<Display>>,
         data: &mut T,
         logger: L,
-        helper: LaunchHelper,
     ) -> Result<XWayland<WM>, ()>
     where
         L: Into<Option<::slog::Logger>>,
@@ -113,7 +111,6 @@ impl<WM: XWindowManager + 'static> XWayland<WM> {
             }),
             wayland_display: display,
             instance: None,
-            helper,
             log: log.new(o!("smithay_module" => "XWayland")),
         }));
         launch(&inner, data)?;
@@ -133,6 +130,7 @@ struct XWaylandInstance {
     startup_handler: Option<Source<Generic<Fd>>>,
     wm_fd: Option<UnixStream>,
     started_at: ::std::time::Instant,
+    child_stdout: Option<ChildStdout>,
 }
 
 type SourceMaker<WM> = dyn FnMut(Rc<RefCell<Inner<WM>>>, RawFd) -> Result<Source<Generic<Fd>>, ()>;
@@ -144,7 +142,6 @@ struct Inner<WM: XWindowManager> {
     wayland_display: Rc<RefCell<Display>>,
     instance: Option<XWaylandInstance>,
     kill_source: Box<dyn Fn(Source<Generic<Fd>>)>,
-    helper: LaunchHelper,
     log: ::slog::Logger,
 }
 
@@ -185,16 +182,15 @@ fn launch<WM: XWindowManager + 'static, T: Any>(
     }));
 
     // all is ready, we can do the fork dance
-    match guard.helper.launch(lock.display(), wl_x11, x_wm_x11, &x_fds) {
+    let child_stdout = match spawn_xwayland(lock.display(), wl_x11, x_wm_x11, &x_fds) {
-        Ok(()) => {}
+        Ok(child_stdout) => child_stdout,
         Err(e) => {
-            error!(guard.log, "Could not initiate launch of Xwayland"; "err" => format!("{:?}", e));
+            error!(guard.log, "XWayland failed to spawn"; "err" => format!("{:?}", e));
             return Err(());
         }
-    }
+    };
 
-    let status_fd = guard.helper.status_fd();
+    let startup_handler = (&mut *guard.source_maker)(inner.clone(), child_stdout.as_raw_fd())?;
-    let startup_handler = (&mut *guard.source_maker)(inner.clone(), status_fd)?;
 
     guard.instance = Some(XWaylandInstance {
         display_lock: lock,
@@ -202,6 +198,7 @@ fn launch<WM: XWindowManager + 'static, T: Any>(
         startup_handler: Some(startup_handler),
         wm_fd: Some(x_wm_me),
         started_at: creation_time,
+        child_stdout: Some(child_stdout),
     });
 
     Ok(())
@@ -258,9 +255,13 @@ fn xwayland_ready<WM: XWindowManager>(inner: &Rc<RefCell<Inner<WM>>>) {
     // instance should never be None at this point
     let instance = inner.instance.as_mut().unwrap();
     let wm = &mut inner.wm;
+    // neither the child_stdout
+    let child_stdout = instance.child_stdout.as_mut().unwrap();
 
-    let success = match inner.helper.was_launch_succesful() {
+    // This reads the one byte that is written when sh receives SIGUSR1
-        Ok(s) => s,
+    let mut buffer = [0];
+    let success = match child_stdout.read(&mut buffer) {
+        Ok(len) => len > 0 && buffer[0] == b'S',
         Err(e) => {
             error!(inner.log, "Checking launch status failed"; "err" => format!("{:?}", e));
             false
@@ -290,72 +291,79 @@ fn xwayland_ready<WM: XWindowManager>(inner: &Rc<RefCell<Inner<WM>>>) {
     }
 }
 
-pub(crate) enum Void {}
-
 /// Exec XWayland with given sockets on given display
 ///
 /// If this returns, that means that something failed
-pub(crate) fn exec_xwayland(
+fn spawn_xwayland(
     display: u32,
     wayland_socket: UnixStream,
     wm_socket: UnixStream,
     listen_sockets: &[UnixStream],
-) -> NixResult<Void> {
+) -> IOResult<ChildStdout> {
-    // uset the CLOEXEC flag from the sockets we need to pass
+    let mut command = Command::new("sh");
-    // to xwayland
+
-    unset_cloexec(&wayland_socket)?;
+    // We use output stream to communicate because FD is easier to handle than exit code.
-    unset_cloexec(&wm_socket)?;
+    command.stdout(Stdio::piped());
+
+    let mut xwayland_args = format!(":{} -rootless -terminate -wm {}", display, wm_socket.as_raw_fd());
     for socket in listen_sockets {
-        unset_cloexec(socket)?;
+        xwayland_args.push_str(&format!(" -listen {}", socket.as_raw_fd()));
     }
-    // prepare the arguments to XWayland
+    // This command let sh to:
-    let mut args = vec![
+    // * Set up signal handler for USR1
-        CString::new("Xwayland").unwrap(),
+    // * Launch Xwayland with USR1 ignored so Xwayland will signal us when it is ready (also redirect
-        CString::new(format!(":{}", display)).unwrap(),
+    //   Xwayland's STDOUT to STDERR so its output, if any, won't distract us)
-        CString::new("-rootless").unwrap(),
+    // * Print "S" and exit if USR1 is received
-        CString::new("-terminate").unwrap(),
+    command.arg("-c").arg(format!(
-        CString::new("-wm").unwrap(),
+        "trap 'echo S' USR1; (trap '' USR1; exec Xwayland {}) 1>&2 & wait",
-        CString::new(format!("{}", wm_socket.as_raw_fd())).unwrap(),
+        xwayland_args
-    ];
+    ));
-    for socket in listen_sockets {
+
-        args.push(CString::new("-listen").unwrap());
+    // Setup the environment: clear everything except PATH and XDG_RUNTIME_DIR
-        args.push(CString::new(format!("{}", socket.as_raw_fd())).unwrap());
+    command.env_clear();
-    }
+    for (key, value) in env::vars_os() {
-    // setup the environment: clear everything except PATH and XDG_RUNTIME_DIR
-    for (key, _) in env::vars_os() {
         if key.to_str() == Some("PATH") || key.to_str() == Some("XDG_RUNTIME_DIR") {
+            command.env(key, value);
             continue;
         }
-        env::remove_var(key);
     }
-    // the WAYLAND_SOCKET var tells XWayland where to connect as a wayland client
+    command.env("WAYLAND_SOCKET", format!("{}", wayland_socket.as_raw_fd()));
-    env::set_var("WAYLAND_SOCKET", format!("{}", wayland_socket.as_raw_fd()));
 
-    // ignore SIGUSR1, this will make the XWayland server send us this
-    // signal when it is ready apparently
     unsafe {
-        use nix::sys::signal::*;
+        let wayland_socket_fd = wayland_socket.as_raw_fd();
-        sigaction(
+        let wm_socket_fd = wm_socket.as_raw_fd();
-            Signal::SIGUSR1,
+        let socket_fds: Vec<_> = listen_sockets.iter().map(|socket| socket.as_raw_fd()).collect();
-            &SigAction::new(SigHandler::SigIgn, SaFlags::empty(), SigSet::empty()),
+        command.pre_exec(move || {
-        )?;
+            // unset the CLOEXEC flag from the sockets we need to pass
+            // to xwayland
+            unset_cloexec(wayland_socket_fd)?;
+            unset_cloexec(wm_socket_fd)?;
+            for &socket in socket_fds.iter() {
+                unset_cloexec(socket)?;
+            }
+            Ok(())
+        });
     }
 
-    // run it
+    let mut child = command.spawn()?;
-    let ret = ::nix::unistd::execvp(
+    Ok(child.stdout.take().expect("stdout should be piped"))
-        &CString::new("Xwayland").unwrap(),
+}
-        &args.iter().map(AsRef::as_ref).collect::<Vec<_>>(),
+
-    )?;
+fn nix_error_to_io(err: NixError) -> IOError {
-    // small dance to actually return Void
+    use std::io::ErrorKind;
-    match ret {}
+    match err {
+        NixError::Sys(errno) => errno.into(),
+        NixError::InvalidPath | NixError::InvalidUtf8 => IOError::new(ErrorKind::InvalidInput, err),
+        NixError::UnsupportedOperation => IOError::new(ErrorKind::Other, err),
+    }
 }
 
 /// Remove the `O_CLOEXEC` flag from this `Fd`
 ///
 /// This means that the `Fd` will *not* be automatically
 /// closed when we `exec()` into XWayland
-fn unset_cloexec<F: AsRawFd>(fd: &F) -> NixResult<()> {
+fn unset_cloexec(fd: RawFd) -> IOResult<()> {
     use nix::fcntl::{fcntl, FcntlArg, FdFlag};
-    fcntl(fd.as_raw_fd(), FcntlArg::F_SETFD(FdFlag::empty()))?;
+    fcntl(fd, FcntlArg::F_SETFD(FdFlag::empty())).map_err(nix_error_to_io)?;
     Ok(())
 }