From 4d012e17a0b37cf8fe39530df0948fafa371b026 Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Tue, 29 Dec 2020 15:41:29 +0100 Subject: [PATCH] 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 * 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 * 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 * xwayland: Set $DISPLAY earlier This way, the WM already gets the correct value of $DISPLAY. Signed-off-by: Uli Schlachter * 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 * Add to the comment explaining Xwayland startup Signed-off-by: Uli Schlachter * Please rustfmt Signed-off-by: Uli Schlachter * Make clippy happy Signed-off-by: Uli Schlachter --- src/xwayland/launch_helper.rs | 225 ++++++++++++++++++++++++++++++++++ src/xwayland/mod.rs | 2 + src/xwayland/xserver.rs | 131 ++++++-------------- 3 files changed, 266 insertions(+), 92 deletions(-) create mode 100644 src/xwayland/launch_helper.rs diff --git a/src/xwayland/launch_helper.rs b/src/xwayland/launch_helper.rs new file mode 100644 index 0000000..6e44364 --- /dev/null +++ b/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 { + 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 { + // 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::>(); + + // 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 { + 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), + } +} diff --git a/src/xwayland/mod.rs b/src/xwayland/mod.rs index 28782c4..1fba973 100644 --- a/src/xwayland/mod.rs +++ b/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}; diff --git a/src/xwayland/xserver.rs b/src/xwayland/xserver.rs index e16b707..4218696 100644 --- a/src/xwayland/xserver.rs +++ b/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 nix::{ - errno::Errno, - sys::signal, - unistd::{fork, ForkResult, Pid}, - Error as NixError, Result as NixResult, +use calloop::{ + generic::{Fd, Generic}, + Interest, LoopHandle, Mode, Source, }; +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 { @@ -81,6 +87,7 @@ impl XWayland { display: Rc>, data: &mut T, logger: L, + helper: LaunchHelper, ) -> Result, ()> where L: Into>, @@ -95,7 +102,7 @@ impl XWayland { 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 XWayland { }), wayland_display: display, instance: None, + helper, log: log.new(o!("smithay_module" => "XWayland")), })); launch(&inner, data)?; @@ -122,14 +130,12 @@ impl Drop for XWayland { struct XWaylandInstance { display_lock: X11Lock, wayland_client: Client, - startup_handler: Option>>, + startup_handler: Option>>, wm_fd: Option, started_at: ::std::time::Instant, - child_pid: Option, } -type SourceMaker = - dyn FnMut(Rc>>, UnixStream) -> Result>, ()>; +type SourceMaker = dyn FnMut(Rc>>, RawFd) -> Result>, ()>; // Inner implementation of the XWayland manager struct Inner { @@ -137,7 +143,8 @@ struct Inner { source_maker: Box>, wayland_display: Rc>, instance: Option, - kill_source: Box>)>, + kill_source: Box>)>, + helper: LaunchHelper, log: ::slog::Logger, } @@ -155,7 +162,6 @@ fn launch( 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( })); // all is ready, we can do the fork dance - let child_pid = match unsafe { fork() } { - Ok(ForkResult::Parent { child }) => { - // 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) }; - } - } - } + match guard.helper.launch(lock.display(), wl_x11, x_wm_x11, &x_fds) { + Ok(()) => {} 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( 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(map: &::wayland_server:: } fn xwayland_ready(inner: &Rc>>) { - 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: bool; - loop { - match wait::waitpid(pid, None) { - Ok(wait::WaitStatus::Exited(_, 0)) => { - // XWayland was correctly started :) - success = true; - break; - } - Err(NixError::Sys(Errno::EINTR)) => { - // interupted, retry - continue; - } - _ => { - // something went wrong :( - success = false; - break; - } + let success = match inner.helper.was_launch_succesful() { + Ok(s) => s, + Err(e) => { + error!(inner.log, "Checking launch status failed"; "err" => format!("{:?}", e)); + false } - } + }; 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(inner: &Rc>>) { } } -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,