From a14a2f6fb06358f83ed26ed8ef03faab3084dd84 Mon Sep 17 00:00:00 2001 From: Aram Drevekenin Date: Mon, 1 Nov 2021 22:01:59 +0100 Subject: [PATCH] fix(unix): forkpty => openpty (#830) * fix(unix): forkpty => openpty * style(fmt): make rustfmt happy --- Cargo.lock | 11 ++ zellij-server/Cargo.toml | 1 + zellij-server/src/os_input_output.rs | 142 +++++++++++-------------- zellij-server/src/pty.rs | 81 +++++++------- zellij-server/src/unit/screen_tests.rs | 9 +- zellij-server/src/unit/tab_tests.rs | 8 +- 6 files changed, 126 insertions(+), 126 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 39d449fe..f092f778 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -351,6 +351,16 @@ dependencies = [ "vec_map", ] +[[package]] +name = "close_fds" +version = "0.3.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3bc416f33de9d59e79e57560f450d21ff8393adcf1cdfc3e6d8fb93d5f88a2ed" +dependencies = [ + "cfg-if 1.0.0", + "libc", +] + [[package]] name = "colored" version = "2.0.0" @@ -2857,6 +2867,7 @@ dependencies = [ "byteorder", "cassowary", "chrono", + "close_fds", "daemonize", "darwin-libproc", "highway", diff --git a/zellij-server/Cargo.toml b/zellij-server/Cargo.toml index 9d7c075e..ea72f2d0 100644 --- a/zellij-server/Cargo.toml +++ b/zellij-server/Cargo.toml @@ -25,6 +25,7 @@ zellij-utils = { path = "../zellij-utils/", version = "0.20.0" } log = "0.4.14" typetag = "0.1.7" chrono = "0.4.19" +close_fds = "0.3.2" [target.'cfg(target_os = "macos")'.dependencies] darwin-libproc = "0.2.0" diff --git a/zellij-server/src/os_input_output.rs b/zellij-server/src/os_input_output.rs index 3ca50ed6..3e19ca5a 100644 --- a/zellij-server/src/os_input_output.rs +++ b/zellij-server/src/os_input_output.rs @@ -1,5 +1,7 @@ use std::collections::HashMap; +use crate::panes::PaneId; + #[cfg(target_os = "macos")] use darwin_libproc; @@ -10,7 +12,7 @@ use std::env; use std::os::unix::io::RawFd; use std::os::unix::process::CommandExt; use std::path::PathBuf; -use std::process::{Child, Command}; +use std::process::{Child, Command, Stdio}; use std::sync::{Arc, Mutex}; use zellij_utils::{async_std, interprocess, libc, nix, signal_hook, zellij_tile}; @@ -18,7 +20,8 @@ use zellij_utils::{async_std, interprocess, libc, nix, signal_hook, zellij_tile} use async_std::fs::File as AsyncFile; use async_std::os::unix::io::FromRawFd; use interprocess::local_socket::LocalSocketStream; -use nix::pty::{forkpty, ForkptyResult, Winsize}; +use nix::fcntl::{fcntl, FcntlArg, FdFlag}; +use nix::pty::{forkpty, openpty, ForkptyResult, OpenptyResult, Winsize}; use nix::sys::signal::{kill, Signal}; use nix::sys::termios; use nix::sys::wait::waitpid; @@ -99,96 +102,63 @@ fn handle_command_exit(mut child: Child) { } } -fn handle_fork_pty( - fork_pty_res: ForkptyResult, +fn handle_openpty( + open_pty_res: OpenptyResult, cmd: RunCommand, - parent_fd: RawFd, - child_fd: RawFd, -) -> (RawFd, ChildId) { - let pid_primary = fork_pty_res.master; - let (pid_secondary, pid_shell) = match fork_pty_res.fork_result { - ForkResult::Parent { child } => { - let pid_shell = read_from_pipe(parent_fd, child_fd); - (child, pid_shell) + quit_cb: Box, +) -> (RawFd, RawFd) { + // primary side of pty and child fd + let pid_primary = open_pty_res.master; + let pid_secondary = open_pty_res.slave; + + let mut child = unsafe { + let command = &mut Command::new(cmd.command); + if let Some(current_dir) = cmd.cwd { + command.current_dir(current_dir); } - ForkResult::Child => { - let child = unsafe { - let command = &mut Command::new(cmd.command); - if let Some(current_dir) = cmd.cwd { - command.current_dir(current_dir); + command + .args(&cmd.args) + .pre_exec(move || -> std::io::Result<()> { + if libc::login_tty(pid_secondary) != 0 { + panic!("failed to set controlling terminal"); } - command - .args(&cmd.args) - .pre_exec(|| -> std::io::Result<()> { - // this is the "unsafe" part, for more details please see: - // https://doc.rust-lang.org/std/os/unix/process/trait.CommandExt.html#notes-and-safety - unistd::setpgid(Pid::from_raw(0), Pid::from_raw(0)) - .expect("failed to create a new process group"); - Ok(()) - }) - .spawn() - .expect("failed to spawn") - }; - unistd::tcsetpgrp(0, Pid::from_raw(child.id() as i32)) - .expect("faled to set child's forceground process group"); - write_to_pipe(child.id(), parent_fd, child_fd); - handle_command_exit(child); - ::std::process::exit(0); - } + close_fds::close_open_fds(3, &[]); + Ok(()) + }) + .spawn() + .expect("failed to spawn") }; - ( - pid_primary, - ChildId { - primary: pid_secondary, - shell: pid_shell.map(|pid| Pid::from_raw(pid as i32)), - }, - ) + let child_id = child.id(); + std::thread::spawn(move || { + child.wait().unwrap(); + handle_command_exit(child); + let _ = nix::unistd::close(pid_primary); + let _ = nix::unistd::close(pid_secondary); + quit_cb(PaneId::Terminal(pid_primary)); + }); + + (pid_primary, child_id as RawFd) } /// Spawns a new terminal from the parent terminal with [`termios`](termios::Termios) /// `orig_termios`. /// -fn handle_terminal(cmd: RunCommand, orig_termios: termios::Termios) -> (RawFd, ChildId) { +fn handle_terminal( + cmd: RunCommand, + orig_termios: termios::Termios, + quit_cb: Box, +) -> (RawFd, RawFd) { // Create a pipe to allow the child the communicate the shell's pid to it's // parent. - let (parent_fd, child_fd) = unistd::pipe().expect("failed to create pipe"); - match forkpty(None, Some(&orig_termios)) { - Ok(fork_pty_res) => handle_fork_pty(fork_pty_res, cmd, parent_fd, child_fd), + match openpty(None, Some(&orig_termios)) { + Ok(open_pty_res) => handle_openpty(open_pty_res, cmd, quit_cb), Err(e) => { - panic!("failed to fork {:?}", e); + panic!("failed to start pty{:?}", e); } } } -/// Write to a pipe given both file descriptors -fn write_to_pipe(data: u32, parent_fd: RawFd, child_fd: RawFd) { - let mut buff = [0; 4]; - BigEndian::write_u32(&mut buff, data); - if unistd::close(parent_fd).is_err() { - return; - } - if unistd::write(child_fd, &buff).is_err() { - return; - } - unistd::close(child_fd).unwrap_or_default(); -} - -/// Read from a pipe given both file descriptors -fn read_from_pipe(parent_fd: RawFd, child_fd: RawFd) -> Option { - let mut buffer = [0; 4]; - if unistd::close(child_fd).is_err() { - return None; - } - if unistd::read(parent_fd, &mut buffer).is_err() { - return None; - } - if unistd::close(parent_fd).is_err() { - return None; - } - Some(u32::from_be_bytes(buffer)) -} - /// If a [`TerminalAction::OpenFile(file)`] is given, the text editor specified by environment variable `EDITOR` /// (or `VISUAL`, if `EDITOR` is not set) will be started in the new terminal, with the given /// file open. @@ -204,7 +174,8 @@ fn read_from_pipe(parent_fd: RawFd, child_fd: RawFd) -> Option { pub fn spawn_terminal( terminal_action: TerminalAction, orig_termios: termios::Termios, -) -> (RawFd, ChildId) { + quit_cb: Box, +) -> (RawFd, RawFd) { let cmd = match terminal_action { TerminalAction::OpenFile(file_to_open) => { if env::var("EDITOR").is_err() && env::var("VISUAL").is_err() { @@ -226,7 +197,7 @@ pub fn spawn_terminal( TerminalAction::RunCommand(command) => command, }; - handle_terminal(cmd, orig_termios) + handle_terminal(cmd, orig_termios, quit_cb) } #[derive(Clone)] @@ -271,7 +242,11 @@ pub trait ServerOsApi: Send + Sync { /// Spawn a new terminal, with a terminal action. The returned tuple contains the master file /// descriptor of the forked psuedo terminal and a [ChildId] struct containing process id's for /// the forked child process. - fn spawn_terminal(&self, terminal_action: TerminalAction) -> (RawFd, ChildId); + fn spawn_terminal( + &self, + terminal_action: TerminalAction, + quit_cb: Box, + ) -> (RawFd, RawFd); /// Read bytes from the standard output of the virtual terminal referred to by `fd`. fn read_from_tty_stdout(&self, fd: RawFd, buf: &mut [u8]) -> Result; /// Creates an `AsyncReader` that can be used to read from `fd` in an async context @@ -304,9 +279,13 @@ impl ServerOsApi for ServerOsInputOutput { set_terminal_size_using_fd(fd, cols, rows); } } - fn spawn_terminal(&self, terminal_action: TerminalAction) -> (RawFd, ChildId) { + fn spawn_terminal( + &self, + terminal_action: TerminalAction, + quit_cb: Box, + ) -> (RawFd, RawFd) { let orig_termios = self.orig_termios.lock().unwrap(); - spawn_terminal(terminal_action, orig_termios.clone()) + spawn_terminal(terminal_action, orig_termios.clone(), quit_cb) } fn read_from_tty_stdout(&self, fd: RawFd, buf: &mut [u8]) -> Result { unistd::read(fd, buf) @@ -324,8 +303,7 @@ impl ServerOsApi for ServerOsInputOutput { Box::new((*self).clone()) } fn kill(&self, pid: Pid) -> Result<(), nix::Error> { - kill(pid, Some(Signal::SIGTERM)).unwrap(); - waitpid(pid, None).unwrap(); + let _ = kill(pid, Some(Signal::SIGTERM)); Ok(()) } fn force_kill(&self, pid: Pid) -> Result<(), nix::Error> { diff --git a/zellij-server/src/pty.rs b/zellij-server/src/pty.rs index 277f572c..93699a32 100644 --- a/zellij-server/src/pty.rs +++ b/zellij-server/src/pty.rs @@ -1,5 +1,5 @@ use crate::{ - os_input_output::{AsyncReader, ChildId, ServerOsApi}, + os_input_output::{AsyncReader, ServerOsApi}, panes::PaneId, screen::ScreenInstruction, thread_bus::{Bus, ThreadSenders}, @@ -17,6 +17,7 @@ use std::{ path::PathBuf, time::{Duration, Instant}, }; +use zellij_utils::nix::unistd::Pid; use zellij_utils::{ async_std, errors::{get_current_ctx, ContextType, PtyContext}, @@ -66,7 +67,7 @@ impl From<&PtyInstruction> for PtyContext { pub(crate) struct Pty { pub active_panes: HashMap, pub bus: Bus, - pub id_to_child_pid: HashMap, + pub id_to_child_pid: HashMap, // pty_primary => child raw fd debug_to_file: bool, task_handles: HashMap>, } @@ -252,15 +253,6 @@ fn stream_terminal_bytes( } } async_send_to_screen(senders.clone(), ScreenInstruction::Render).await; - - // we send ClosePane here so that the screen knows to close this tab if the process - // inside the terminal exited on its own (eg. the user typed "exit" inside a - // bash shell) - async_send_to_screen( - senders, - ScreenInstruction::ClosePane(PaneId::Terminal(pid), None), - ) - .await; } }) } @@ -283,9 +275,14 @@ impl Pty { .and_then(|client_id| self.active_panes.get(&client_id)) .and_then(|pane| match pane { PaneId::Plugin(..) => None, - PaneId::Terminal(id) => self.id_to_child_pid.get(id).and_then(|id| id.shell), + PaneId::Terminal(id) => self.id_to_child_pid.get(id), + }) + .and_then(|id| { + self.bus + .os_input + .as_ref() + .map(|input| input.get_cwd(Pid::from_raw(*id))) }) - .and_then(|id| self.bus.os_input.as_ref().map(|input| input.get_cwd(id))) .flatten(), }) } @@ -302,12 +299,18 @@ impl Pty { terminal_action.unwrap_or_else(|| self.get_default_terminal(None)) } }; - let (pid_primary, child_id): (RawFd, ChildId) = self + let quit_cb = Box::new({ + let senders = self.bus.senders.clone(); + move |pane_id| { + let _ = senders.send_to_screen(ScreenInstruction::ClosePane(pane_id, None)); + } + }); + let (pid_primary, child_fd): (RawFd, RawFd) = self .bus .os_input .as_mut() .unwrap() - .spawn_terminal(terminal_action); + .spawn_terminal(terminal_action, quit_cb); let task_handle = stream_terminal_bytes( pid_primary, self.bus.senders.clone(), @@ -315,7 +318,7 @@ impl Pty { self.debug_to_file, ); self.task_handles.insert(pid_primary, task_handle); - self.id_to_child_pid.insert(pid_primary, child_id); + self.id_to_child_pid.insert(pid_primary, child_fd); pid_primary } pub fn spawn_terminals_for_layout( @@ -329,22 +332,32 @@ impl Pty { let extracted_run_instructions = layout.extract_run_instructions(); let mut new_pane_pids = vec![]; for run_instruction in extracted_run_instructions { + let quit_cb = Box::new({ + let senders = self.bus.senders.clone(); + move |pane_id| { + let _ = senders.send_to_screen(ScreenInstruction::ClosePane(pane_id, None)); + } + }); match run_instruction { Some(Run::Command(command)) => { let cmd = TerminalAction::RunCommand(command); - let (pid_primary, child_id): (RawFd, ChildId) = - self.bus.os_input.as_mut().unwrap().spawn_terminal(cmd); - self.id_to_child_pid.insert(pid_primary, child_id); - new_pane_pids.push(pid_primary); - } - None => { - let (pid_primary, child_id): (RawFd, ChildId) = self + let (pid_primary, child_fd): (RawFd, RawFd) = self .bus .os_input .as_mut() .unwrap() - .spawn_terminal(default_shell.clone()); - self.id_to_child_pid.insert(pid_primary, child_id); + .spawn_terminal(cmd, quit_cb); + self.id_to_child_pid.insert(pid_primary, child_fd); + new_pane_pids.push(pid_primary); + } + None => { + let (pid_primary, child_fd): (RawFd, RawFd) = self + .bus + .os_input + .as_mut() + .unwrap() + .spawn_terminal(default_shell.clone(), quit_cb); + self.id_to_child_pid.insert(pid_primary, child_fd); new_pane_pids.push(pid_primary); } // Investigate moving plugin loading to here. @@ -372,27 +385,15 @@ impl Pty { pub fn close_pane(&mut self, id: PaneId) { match id { PaneId::Terminal(id) => { - let pids = self.id_to_child_pid.remove(&id).unwrap(); - let handle = self.task_handles.remove(&id).unwrap(); + let child_fd = self.id_to_child_pid.remove(&id).unwrap(); + self.task_handles.remove(&id).unwrap(); task::block_on(async { self.bus .os_input .as_mut() .unwrap() - .kill(pids.primary) + .kill(Pid::from_raw(child_fd)) .unwrap(); - let timeout = Duration::from_millis(100); - match async_timeout(timeout, handle.cancel()).await { - Ok(_) => {} - _ => { - self.bus - .os_input - .as_mut() - .unwrap() - .force_kill(pids.primary) - .unwrap(); - } - }; }); } PaneId::Plugin(pid) => drop( diff --git a/zellij-server/src/unit/screen_tests.rs b/zellij-server/src/unit/screen_tests.rs index b4d367e4..f9f63195 100644 --- a/zellij-server/src/unit/screen_tests.rs +++ b/zellij-server/src/unit/screen_tests.rs @@ -1,7 +1,8 @@ use super::{Screen, ScreenInstruction}; +use crate::panes::PaneId; use crate::zellij_tile::data::{ModeInfo, Palette}; use crate::{ - os_input_output::{AsyncReader, ChildId, Pid, ServerOsApi}, + os_input_output::{AsyncReader, Pid, ServerOsApi}, thread_bus::Bus, ClientId, }; @@ -29,7 +30,11 @@ impl ServerOsApi for FakeInputOutput { fn set_terminal_size_using_fd(&self, _fd: RawFd, _cols: u16, _rows: u16) { // noop } - fn spawn_terminal(&self, _file_to_open: TerminalAction) -> (RawFd, ChildId) { + fn spawn_terminal( + &self, + _file_to_open: TerminalAction, + _quit_db: Box, + ) -> (RawFd, RawFd) { unimplemented!() } fn read_from_tty_stdout(&self, _fd: RawFd, _buf: &mut [u8]) -> Result { diff --git a/zellij-server/src/unit/tab_tests.rs b/zellij-server/src/unit/tab_tests.rs index 41e243cd..3d177ab8 100644 --- a/zellij-server/src/unit/tab_tests.rs +++ b/zellij-server/src/unit/tab_tests.rs @@ -1,7 +1,7 @@ use super::Tab; use crate::zellij_tile::data::{ModeInfo, Palette}; use crate::{ - os_input_output::{AsyncReader, ChildId, Pid, ServerOsApi}, + os_input_output::{AsyncReader, Pid, ServerOsApi}, panes::PaneId, thread_bus::ThreadSenders, ClientId, @@ -29,7 +29,11 @@ impl ServerOsApi for FakeInputOutput { fn set_terminal_size_using_fd(&self, _fd: RawFd, _cols: u16, _rows: u16) { // noop } - fn spawn_terminal(&self, _file_to_open: TerminalAction) -> (RawFd, ChildId) { + fn spawn_terminal( + &self, + _file_to_open: TerminalAction, + _quit_cb: Box, + ) -> (RawFd, RawFd) { unimplemented!() } fn read_from_tty_stdout(&self, _fd: RawFd, _buf: &mut [u8]) -> Result {