XWayland: Use a fork helper process to launch Xwayland (#250)

* xwayland: Add LaunchHelper

Calling fork() in a multi-threaded process is a bad idea. I am getting
crashes at exit() and "man fork" says that only async-signal-safe
functions can be called after forking in a multi-threaded process.
exit() is not async-signal safe. Thus, forking needs to happen before
any threads are created.

This commit adds a LaunchHelper to the public API. This is a handle to a
forked child process. So far this does not do much, but the intention is
to use this later instead of fork()ing directly.

Signed-off-by: Uli Schlachter <psychon@znc.in>

* xwayland: Move the fork dance to LaunchHelper

No functional changes intended. This only moves the code over and does
not actually use the LaunchHelper for anything.

Signed-off-by: Uli Schlachter <psychon@znc.in>

* Move checking the launch status to LaunchHelper

This should again have no functional changes, but the error log output
might change slightly, since an IOError instead of a NixError is
returned.

Signed-off-by: Uli Schlachter <psychon@znc.in>

* xwayland: Set $DISPLAY earlier

This way, the WM already gets the correct value of $DISPLAY.

Signed-off-by: Uli Schlachter <psychon@znc.in>

* xwayland: Use the launch helper process

FD passing is now used to give the display number and the sockets to the
launch helper process. That process then forks of the actual process
that spawns Xwayland. Once Xwayland started, this is reported back with
another write on the pipe.

Signed-off-by: Uli Schlachter <psychon@znc.in>

* Add to the comment explaining Xwayland startup

Signed-off-by: Uli Schlachter <psychon@znc.in>

* Please rustfmt

Signed-off-by: Uli Schlachter <psychon@znc.in>

* Make clippy happy

Signed-off-by: Uli Schlachter <psychon@znc.in>

src/xwayland/launch_helper.rs

@@ -0,0 +1,225 @@
+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,7 +13,9 @@
 //!
 //! 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

@@ -23,6 +23,11 @@
  *
  * cf https://github.com/swaywm/wlroots/blob/master/xwayland/xwayland.c
  *
+ * Since fork is not safe to call in a multi-threaded process, a process is
+ * forked of early (LaunchHelper). Via a shared FD, a command to actually launch
+ * Xwayland can be sent to that process. The process then does the fork and
+ * reports back when Xwayland successfully started (=SIGUSR1 was received) with
+ * another write on the pipe.
  */
 use std::{
     any::Any,
@@ -30,25 +35,26 @@ use std::{
     env,
     ffi::CString,
     os::unix::{
-        io::{AsRawFd, IntoRawFd},
+        io::{AsRawFd, IntoRawFd, RawFd},
         net::UnixStream,
     },
     rc::Rc,
     sync::Arc,
 };
 
-use calloop::{generic::Generic, Interest, LoopHandle, Mode, Source};
+use calloop::{
-
+    generic::{Fd, Generic},
-use nix::{
+    Interest, LoopHandle, Mode, Source,
-    errno::Errno,
-    sys::signal,
-    unistd::{fork, ForkResult, Pid},
-    Error as NixError, Result as NixResult,
 };
 
+use nix::Result as NixResult;
+
 use wayland_server::{Client, Display, Filter};
 
-use super::x11_sockets::{prepare_x11_sockets, X11Lock};
+use super::{
+    x11_sockets::{prepare_x11_sockets, X11Lock},
+    LaunchHelper,
+};
 
 /// The XWayland handle
 pub struct XWayland<WM: XWindowManager> {
@@ -81,6 +87,7 @@ 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>>,
@@ -95,7 +102,7 @@ impl<WM: XWindowManager + 'static> XWayland<WM> {
             source_maker: Box::new(move |inner, fd| {
                 handle
                     .insert_source(
-                        Generic::new(fd, Interest::Readable, Mode::Level),
+                        Generic::new(Fd(fd), Interest::Readable, Mode::Level),
                         move |evt, _, _| {
                             debug_assert!(evt.readable);
                             xwayland_ready(&inner);
@@ -106,6 +113,7 @@ impl<WM: XWindowManager + 'static> XWayland<WM> {
             }),
             wayland_display: display,
             instance: None,
+            helper,
             log: log.new(o!("smithay_module" => "XWayland")),
         }));
         launch(&inner, data)?;
@@ -122,14 +130,12 @@ impl<WM: XWindowManager> Drop for XWayland<WM> {
 struct XWaylandInstance {
     display_lock: X11Lock,
     wayland_client: Client,
-    startup_handler: Option<Source<Generic<UnixStream>>>,
+    startup_handler: Option<Source<Generic<Fd>>>,
     wm_fd: Option<UnixStream>,
     started_at: ::std::time::Instant,
-    child_pid: Option<Pid>,
 }
 
-type SourceMaker<WM> =
+type SourceMaker<WM> = dyn FnMut(Rc<RefCell<Inner<WM>>>, RawFd) -> Result<Source<Generic<Fd>>, ()>;
-    dyn FnMut(Rc<RefCell<Inner<WM>>>, UnixStream) -> Result<Source<Generic<UnixStream>>, ()>;
 
 // Inner implementation of the XWayland manager
 struct Inner<WM: XWindowManager> {
@@ -137,7 +143,8 @@ struct Inner<WM: XWindowManager> {
     source_maker: Box<SourceMaker<WM>>,
     wayland_display: Rc<RefCell<Display>>,
     instance: Option<XWaylandInstance>,
-    kill_source: Box<dyn Fn(Source<Generic<UnixStream>>)>,
+    kill_source: Box<dyn Fn(Source<Generic<Fd>>)>,
+    helper: LaunchHelper,
     log: ::slog::Logger,
 }
 
@@ -155,7 +162,6 @@ fn launch<WM: XWindowManager + 'static, T: Any>(
 
     info!(guard.log, "Starting XWayland");
 
-    let (status_child, status_me) = UnixStream::pair().map_err(|_| ())?;
     let (x_wm_x11, x_wm_me) = UnixStream::pair().map_err(|_| ())?;
     let (wl_x11, wl_me) = UnixStream::pair().map_err(|_| ())?;
 
@@ -179,58 +185,16 @@ fn launch<WM: XWindowManager + 'static, T: Any>(
     }));
 
     // all is ready, we can do the fork dance
-    let child_pid = match unsafe { fork() } {
+    match guard.helper.launch(lock.display(), wl_x11, x_wm_x11, &x_fds) {
-        Ok(ForkResult::Parent { child }) => {
+        Ok(()) => {}
-            // we are the main smithay process
-            child
-        }
-        Ok(ForkResult::Child) => {
-            // we are the first child
-            let mut set = signal::SigSet::empty();
-            set.add(signal::Signal::SIGUSR1);
-            set.add(signal::Signal::SIGCHLD);
-            // we can't handle errors here anyway
-            let _ = signal::sigprocmask(signal::SigmaskHow::SIG_BLOCK, Some(&set), None);
-            match unsafe { fork() } {
-                Ok(ForkResult::Parent { child }) => {
-                    // When we exit(), we will close() this which wakes up the main process.
-                    let _status_child = status_child;
-                    // we are still the first child
-                    let sig = set.wait();
-                    // Parent will wait for us and know from out
-                    // exit status if XWayland launch was a success or not =)
-                    if let Ok(signal::Signal::SIGCHLD) = sig {
-                        // XWayland has exited before being ready
-                        let _ = ::nix::sys::wait::waitpid(child, None);
-                        unsafe { ::nix::libc::exit(1) };
-                    }
-                    unsafe { ::nix::libc::exit(0) };
-                }
-                Ok(ForkResult::Child) => {
-                    // we are the second child, we exec xwayland
-                    match exec_xwayland(lock.display(), wl_x11, x_wm_x11, &x_fds) {
-                        Ok(x) => match x {},
-                        Err(e) => {
-                            // well, what can we do ?
-                            error!(guard.log, "exec XWayland failed"; "err" => format!("{:?}", e));
-                            unsafe { ::nix::libc::exit(1) };
-                        }
-                    }
-                }
-                Err(e) => {
-                    // well, what can we do ?
-                    error!(guard.log, "XWayland second fork failed"; "err" => format!("{:?}", e));
-                    unsafe { ::nix::libc::exit(1) };
-                }
-            }
-        }
         Err(e) => {
-            error!(guard.log, "XWayland first fork failed"; "err" => format!("{:?}", e));
+            error!(guard.log, "Could not initiate launch of Xwayland"; "err" => format!("{:?}", e));
             return Err(());
         }
-    };
+    }
 
-    let startup_handler = (&mut *guard.source_maker)(inner.clone(), status_me)?;
+    let status_fd = guard.helper.status_fd();
+    let startup_handler = (&mut *guard.source_maker)(inner.clone(), status_fd)?;
 
     guard.instance = Some(XWaylandInstance {
         display_lock: lock,
@@ -238,7 +202,6 @@ fn launch<WM: XWindowManager + 'static, T: Any>(
         startup_handler: Some(startup_handler),
         wm_fd: Some(x_wm_me),
         started_at: creation_time,
-        child_pid: Some(child_pid),
     });
 
     Ok(())
@@ -290,46 +253,30 @@ fn client_destroy<WM: XWindowManager + 'static, T: Any>(map: &::wayland_server::
 }
 
 fn xwayland_ready<WM: XWindowManager>(inner: &Rc<RefCell<Inner<WM>>>) {
-    use nix::sys::wait;
     let mut guard = inner.borrow_mut();
     let inner = &mut *guard;
     // instance should never be None at this point
     let instance = inner.instance.as_mut().unwrap();
     let wm = &mut inner.wm;
-    // neither the pid
-    let pid = instance.child_pid.unwrap();
 
-    // find out if the launch was a success by waiting on the intermediate child
+    let success = match inner.helper.was_launch_succesful() {
-    let success: bool;
+        Ok(s) => s,
-    loop {
+        Err(e) => {
-        match wait::waitpid(pid, None) {
+            error!(inner.log, "Checking launch status failed"; "err" => format!("{:?}", e));
-            Ok(wait::WaitStatus::Exited(_, 0)) => {
+            false
-                // XWayland was correctly started :)
-                success = true;
-                break;
-            }
-            Err(NixError::Sys(Errno::EINTR)) => {
-                // interupted, retry
-                continue;
-            }
-            _ => {
-                // something went wrong :(
-                success = false;
-                break;
-            }
         }
-    }
+    };
 
     if success {
+        // setup the environemnt
+        ::std::env::set_var("DISPLAY", format!(":{}", instance.display_lock.display()));
+
         // signal the WM
         info!(inner.log, "XWayland is ready, signaling the WM.");
         wm.xwayland_ready(
             instance.wm_fd.take().unwrap(), // This is a bug if None
             instance.wayland_client.clone(),
         );
-
-        // setup the environemnt
-        ::std::env::set_var("DISPLAY", format!(":{}", instance.display_lock.display()));
     } else {
         error!(
             inner.log,
@@ -343,12 +290,12 @@ fn xwayland_ready<WM: XWindowManager>(inner: &Rc<RefCell<Inner<WM>>>) {
     }
 }
 
-enum Void {}
+pub(crate) enum Void {}
 
 /// Exec XWayland with given sockets on given display
 ///
 /// If this returns, that means that something failed
-fn exec_xwayland(
+pub(crate) fn exec_xwayland(
     display: u32,
     wayland_socket: UnixStream,
     wm_socket: UnixStream,