diff --git a/crates/nu-command/src/system/run_external.rs b/crates/nu-command/src/system/run_external.rs index f5f2e5b997..07058be9fb 100644 --- a/crates/nu-command/src/system/run_external.rs +++ b/crates/nu-command/src/system/run_external.rs @@ -13,9 +13,7 @@ use nu_protocol::{Category, Example, ListStream, PipelineData, RawStream, Span, use itertools::Itertools; use nu_engine::CallExt; -use nu_system::external_process_setup::{ - prepare_to_foreground, reset_foreground_id, set_foreground, -}; +use nu_system::ForegroundProcess; use pathdiff::diff_paths; use regex::Regex; @@ -125,15 +123,15 @@ impl ExternalCommand { let ctrlc = engine_state.ctrlc.clone(); - let mut process = self.create_process(&input, false, head)?; - prepare_to_foreground(&mut process); + let mut fg_process = ForegroundProcess::new(self.create_process(&input, false, head)?); let child; #[cfg(windows)] { - match process.spawn() { + match fg_process.spawn() { Err(_) => { - let mut process = self.create_process(&input, true, head)?; + let mut fg_process = + ForegroundProcess::new(self.create_process(&input, true, head)?); child = process.spawn(); } Ok(process) => { @@ -144,7 +142,7 @@ impl ExternalCommand { #[cfg(not(windows))] { - child = process.spawn() + child = fg_process.spawn() } match child { @@ -154,7 +152,6 @@ impl ExternalCommand { self.name.span, )), Ok(mut child) => { - set_foreground(&child); if !input.is_nothing() { let mut engine_state = engine_state.clone(); let mut stack = stack.clone(); @@ -163,7 +160,7 @@ impl ExternalCommand { engine_state.config.use_ansi_coloring = false; // if there is a string or a stream, that is sent to the pipe std - if let Some(mut stdin_write) = child.stdin.take() { + if let Some(mut stdin_write) = child.as_mut().stdin.take() { std::thread::spawn(move || { let input = crate::Table::run( &crate::Table, @@ -204,7 +201,7 @@ impl ExternalCommand { // and we create a ListStream that can be consumed if redirect_stderr { - let stderr = child.stderr.take().ok_or_else(|| { + let stderr = child.as_mut().stderr.take().ok_or_else(|| { ShellError::ExternalCommand( "Error taking stderr from external".to_string(), "Redirects need access to stderr of an external command" @@ -243,7 +240,7 @@ impl ExternalCommand { } if redirect_stdout { - let stdout = child.stdout.take().ok_or_else(|| { + let stdout = child.as_mut().stdout.take().ok_or_else(|| { ShellError::ExternalCommand( "Error taking stdout from external".to_string(), "Redirects need access to stdout of an external command" @@ -281,7 +278,7 @@ impl ExternalCommand { } } - match child.wait() { + match child.as_mut().wait() { Err(err) => Err(ShellError::ExternalCommand( "External command exited with error".into(), err.to_string(), @@ -301,7 +298,6 @@ impl ExternalCommand { span: head, }); } - reset_foreground_id(); Ok(()) } } diff --git a/crates/nu-system/src/foreground.rs b/crates/nu-system/src/foreground.rs index 1e9cb8ca3c..ca2aa3cb7e 100644 --- a/crates/nu-system/src/foreground.rs +++ b/crates/nu-system/src/foreground.rs @@ -1,20 +1,76 @@ -// It's a simpler version for fish shell's external process handling. +use std::process::{Child, Command}; + +/// A simple wrapper for `std::process::Command` +/// +/// ## spawn behavior +/// ### Unix +/// When invoke `spawn`, current process will ignore `SIGTTOU`, `SIGTTIN` signal, spawned child process will get it's +/// own process group id, and it's going to foreground(by making stdin belong's to child's process group). +/// +/// When child is to over, `SIGTTOU`, `SIGTTIN` signal will be reset, foreground process is back to callers' process. +/// +/// ### Windows +/// It does nothing special on windows system, `spawn` is the same as [std::process::Command::spawn](std::process::Command::spawn) +pub struct ForegroundProcess { + inner: Command, +} + +/// A simple wrapper for `std::process::Child` +/// +/// It can only be created by `ForegroundProcess::spawn`. +pub struct ForegroundChild { + inner: Child, +} + +impl ForegroundProcess { + pub fn new(cmd: Command) -> Self { + Self { inner: cmd } + } + + pub fn spawn(&mut self) -> std::io::Result { + fg_process_setup::prepare_to_foreground(&mut self.inner); + self.inner.spawn().map(|child| { + fg_process_setup::set_foreground(&child); + ForegroundChild { inner: child } + }) + } +} + +impl AsMut for ForegroundChild { + fn as_mut(&mut self) -> &mut Child { + &mut self.inner + } +} + +impl Drop for ForegroundChild { + fn drop(&mut self) { + // It's ok to use here because we have called `set_foreground` during creation. + unsafe { fg_process_setup::reset_foreground_id() } + } +} + +// It's a simpler version of fish shell's external process handling. // // For more information, please check `child_setup_process` function in fish shell. // https://github.com/fish-shell/fish-shell/blob/3f90efca38079922b4b21707001d7bb9630107eb/src/postfork.cpp#L140 #[cfg(target_family = "unix")] -pub mod external_process_setup { +mod fg_process_setup { use std::os::unix::prelude::CommandExt; pub fn prepare_to_foreground(external_command: &mut std::process::Command) { unsafe { libc::signal(libc::SIGTTOU, libc::SIG_IGN); libc::signal(libc::SIGTTIN, libc::SIG_IGN); + // Safety: + // POSIX only allows async-signal-safe functions to be called. + // And `setpgid` is async-signal-safe function according to: + // https://manpages.ubuntu.com/manpages/bionic/man7/signal-safety.7.html + // So we're ok to invoke `libc::setpgid` inside `pre_exec`. external_command.pre_exec(|| { // make the command startup with new process group. // The process group id must be the same as external commands' pid. // Or else we'll failed to set it as foreground process. - // For more information, refer to `fork_child_for_process` function: + // For more information, check `fork_child_for_process` function: // https://github.com/fish-shell/fish-shell/blob/023042098396aa450d2c4ea1eb9341312de23126/src/exec.cpp#L398 libc::setpgid(0, 0); Ok(()) @@ -22,28 +78,30 @@ pub mod external_process_setup { } } - /// Before call this function, `prepare_to_foreground` function must be called. Or else - /// it'll failed with silence. + // If `prepare_to_foreground` function is not called, the function will fail with silence and do nothing. pub fn set_foreground(process: &std::process::Child) { + // it's ok to use unsafe here + // the implementaion here is just the same as + // https://docs.rs/nix/latest/nix/unistd/fn.tcsetpgrp.html, which is a safe function. unsafe { libc::tcsetpgrp(libc::STDIN_FILENO, process.id() as i32); } } - /// It can only be called when you have called `set_foreground`, or else - /// Something strange will happened. - pub fn reset_foreground_id() { - unsafe { - libc::tcsetpgrp(libc::STDIN_FILENO, libc::getpgrp()); - libc::signal(libc::SIGTTOU, libc::SIG_DFL); - libc::signal(libc::SIGTTIN, libc::SIG_DFL); - } + /// Reset foreground to current process, and reset back `SIGTTOU`, `SIGTTIN` single handler. + /// + /// ## Safety + /// It can only be called when you have called `set_foreground`, or results in undefined behavior. + pub unsafe fn reset_foreground_id() { + libc::tcsetpgrp(libc::STDIN_FILENO, libc::getpgrp()); + libc::signal(libc::SIGTTOU, libc::SIG_DFL); + libc::signal(libc::SIGTTIN, libc::SIG_DFL); } } // TODO: investigate if we can set foreground process through windows system call. #[cfg(target_family = "windows")] -pub mod external_process_setup { +mod fg_process_setup { pub fn prepare_to_foreground(_external_command: &mut std::process::Command) {} diff --git a/crates/nu-system/src/lib.rs b/crates/nu-system/src/lib.rs index 0fc41a54e5..2283a94b78 100644 --- a/crates/nu-system/src/lib.rs +++ b/crates/nu-system/src/lib.rs @@ -6,7 +6,7 @@ mod macos; #[cfg(target_os = "windows")] mod windows; -pub use self::foreground::external_process_setup; +pub use self::foreground::{ForegroundChild, ForegroundProcess}; #[cfg(any(target_os = "android", target_os = "linux"))] pub use self::linux::*; #[cfg(target_os = "macos")]