Mirrors/nushell
2
0
Fork 0
mirror of https://github.com/nushell/nushell synced 2024-12-27 05:23:11 +00:00
Code Issues Projects Releases Packages Wiki Activity

Do not block signals for child processes (#11402)

Browse source 
# Description / User-Facing Changes
Signals are no longer blocked for child processes launched from both
interactive and non-interactive mode. The only exception is that
`SIGTSTP`, `SIGTTIN`, and `SIGTTOU` remain blocked for child processes
launched only from **interactive** mode. This is to help prevent nushell
from getting into an unrecoverable state, since we don't support
background jobs. Anyways, this fully fixes #9026.

# Other Notes
- Needs Rust version `>= 1.66` for a fix in
`std::process::Command::spawn`, but it looks our current Rust version is
way above this.
- Uses `sigaction` instead of `signal`, since the behavior of `signal`
can apparently differ across systems. Also, the `sigaction` man page
says:
> The sigaction() function supersedes the signal() function, and should
be used in preference.

Additionally, using both `sigaction` and `signal` is not recommended.
Since we were already using `sigaction` in some places (and possibly
some of our dependencies as well), this PR replaces all usages of
`signal`.

# Tests
Might want to wait for #11178 for testing.
This commit is contained in:
Ian Manske 2024-01-15 22:08:21 +00:00 committed by GitHub
parent 7071617f18
commit 924986576d
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
7 changed files with 231 additions and 273 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

1
Cargo.lock generated
View file

@ -2637,7 +2637,6 @@ dependencies = [
"rstest",
"serde_json",
"serial_test",
"signal-hook",
"simplelog",
"tempfile",
"time",

1
Cargo.toml
View file

@ -90,7 +90,6 @@ time = "0.3"
[target.'cfg(not(target_os = "windows"))'.dependencies]
# Our dependencies don't use OpenSSL on Windows
openssl = { version = "0.10", features = ["vendored"], optional = true }
signal-hook = { version = "0.3", default-features = false }
[target.'cfg(windows)'.build-dependencies]
winresource = "0.1"

58
crates/nu-command/src/system/run_external.rs
View file

@ -9,7 +9,7 @@ use nu_protocol::{
Category, Example, ListStream, PipelineData, RawStream, ShellError, Signature, Span, Spanned,
SyntaxShape, Type, Value,
};
use nu_system::ForegroundProcess;
use nu_system::ForegroundChild;
use nu_utils::IgnoreCaseExt;
use os_pipe::PipeReader;
use pathdiff::diff_paths;
@ -216,14 +216,14 @@ impl ExternalCommand {
#[allow(unused_mut)]
let (cmd, mut reader) = self.create_process(&input, false, head)?;
let mut fg_process =
ForegroundProcess::new(cmd, engine_state.pipeline_externals_state.clone());
// mut is used in the windows branch only, suppress warning on other platforms
#[allow(unused_mut)]
let mut child;
#[cfg(all(not(unix), not(windows)))] // are there any systems like this?
let child = ForegroundChild::spawn(cmd);
#[cfg(windows)]
{
let child = match ForegroundChild::spawn(cmd) {
Ok(child) => Ok(child),
Err(err) => {
// Running external commands on Windows has 2 points of complication:
// 1. Some common Windows commands are actually built in to cmd.exe, not executables in their own right.
// 2. We need to let users run batch scripts etc. (.bat, .cmd) without typing their extension
@ -232,10 +232,9 @@ impl ExternalCommand {
// fails to be run as a normal executable:
// 1. "shell out" to cmd.exe if the command is a known cmd.exe internal command
// 2. Otherwise, use `which-rs` to look for batch files etc. then run those in cmd.exe
match fg_process.spawn(engine_state.is_interactive) {
Err(err) => {
// set the default value, maybe we'll override it later
child = Err(err);
let mut child = Err(err);
// This has the full list of cmd.exe "internal" commands: https://ss64.com/nt/syntax-internal.html
// I (Reilly) went through the full list and whittled it down to ones that are potentially useful:
@ -250,11 +249,7 @@ impl ExternalCommand {
if looks_like_cmd_internal {
let (cmd, new_reader) = self.create_process(&input, true, head)?;
reader = new_reader;
let mut cmd_process = ForegroundProcess::new(
cmd,
engine_state.pipeline_externals_state.clone(),
);
child = cmd_process.spawn(engine_state.is_interactive);
child = ForegroundChild::spawn(cmd);
} else {
#[cfg(feature = "which-support")]
{
@ -271,9 +266,7 @@ impl ExternalCommand {
which::which_in(&self.name.item, Some(path_with_cwd), cwd)
{
if let Some(file_name) = which_path.file_name() {
if !file_name
.to_string_lossy()
.eq_ignore_case(command_name)
if !file_name.to_string_lossy().eq_ignore_case(command_name)
{
// which-rs found an executable file with a slightly different name
// than the one the user tried. Let's try running it
@ -282,15 +275,10 @@ impl ExternalCommand {
item: file_name.to_string_lossy().to_string(),
span: self.name.span,
};
let (cmd, new_reader) = new_command
.create_process(&input, true, head)?;
let (cmd, new_reader) =
new_command.create_process(&input, true, head)?;
reader = new_reader;
let mut cmd_process = ForegroundProcess::new(
cmd,
engine_state.pipeline_externals_state.clone(),
);
child =
cmd_process.spawn(engine_state.is_interactive);
child = ForegroundChild::spawn(cmd);
}
}
}
@ -298,17 +286,17 @@ impl ExternalCommand {
}
}
}
}
Ok(process) => {
child = Ok(process);
}
}
}
#[cfg(not(windows))]
{
child = fg_process.spawn(engine_state.is_interactive)
child
}
};
#[cfg(unix)]
let child = ForegroundChild::spawn(
cmd,
engine_state.is_interactive,
&engine_state.pipeline_externals_state,
);
match child {
Err(err) => {

191
crates/nu-system/src/foreground.rs
View file

@ -1,66 +1,78 @@
use std::{
io,
process::{Child, Command},
};
#[cfg(unix)]
use std::{
io::IsTerminal,
sync::{
atomic::{AtomicU32, Ordering},
Arc,
},
};
/// A simple wrapper for `std::process::Command`
/// A simple wrapper for [`std::process::Child`]
///
/// ## Spawn behavior
/// ### Unix
/// It can only be created by [`ForegroundChild::spawn`].
///
/// The spawned child process will get its own process group id, and it's going to foreground (by making stdin belong's to child's process group).
/// # Spawn behavior
/// ## Unix
///
/// For interactive shells, the spawned child process will get its own process group id,
/// and it will be put in the foreground (by making stdin belong to the child's process group).
/// On drop, the calling process's group will become the foreground process group once again.
///
/// ### 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,
pipeline_state: Arc<(AtomicU32, AtomicU32)>,
}
/// A simple wrapper for `std::process::Child`
/// For non-interactive mode, processes are spawned normally without any foreground process handling.
///
/// It can only be created by `ForegroundProcess::spawn`.
/// ## Other systems
///
/// It does nothing special on non-unix systems, so `spawn` is the same as [`std::process::Command::spawn`].
pub struct ForegroundChild {
inner: Child,
pipeline_state: Arc<(AtomicU32, AtomicU32)>,
interactive: bool,
#[cfg(unix)]
pipeline_state: Option<Arc<(AtomicU32, AtomicU32)>>,
}
impl ForegroundProcess {
pub fn new(cmd: Command, pipeline_state: Arc<(AtomicU32, AtomicU32)>) -> Self {
Self {
inner: cmd,
pipeline_state,
}
impl ForegroundChild {
#[cfg(not(unix))]
pub fn spawn(mut command: Command) -> io::Result<Self> {
command.spawn().map(|child| Self { inner: child })
}
pub fn spawn(&mut self, interactive: bool) -> std::io::Result<ForegroundChild> {
let (ref pgrp, ref pcnt) = *self.pipeline_state;
#[cfg(unix)]
pub fn spawn(
mut command: Command,
interactive: bool,
pipeline_state: &Arc<(AtomicU32, AtomicU32)>,
) -> io::Result<Self> {
if interactive && io::stdin().is_terminal() {
let (pgrp, pcnt) = pipeline_state.as_ref();
let existing_pgrp = pgrp.load(Ordering::SeqCst);
fg_process_setup::prepare_to_foreground(&mut self.inner, existing_pgrp, interactive);
self.inner
foreground_pgroup::prepare_command(&mut command, existing_pgrp);
command
.spawn()
.map(|child| {
fg_process_setup::set_foreground(&child, existing_pgrp, interactive);
foreground_pgroup::set(&child, existing_pgrp);
let _ = pcnt.fetch_add(1, Ordering::SeqCst);
if existing_pgrp == 0 {
pgrp.store(child.id(), Ordering::SeqCst);
}
ForegroundChild {
Self {
inner: child,
pipeline_state: self.pipeline_state.clone(),
interactive,
pipeline_state: Some(pipeline_state.clone()),
}
})
.map_err(|e| {
fg_process_setup::reset_foreground_id(interactive);
foreground_pgroup::reset();
e
})
} else {
command.spawn().map(|child| Self {
inner: child,
pipeline_state: None,
})
}
}
}
@ -70,122 +82,85 @@ impl AsMut<Child> for ForegroundChild {
}
}
#[cfg(unix)]
impl Drop for ForegroundChild {
fn drop(&mut self) {
let (ref pgrp, ref pcnt) = *self.pipeline_state;
if let Some((pgrp, pcnt)) = self.pipeline_state.as_deref() {
if pcnt.fetch_sub(1, Ordering::SeqCst) == 1 {
pgrp.store(0, Ordering::SeqCst);
fg_process_setup::reset_foreground_id(self.interactive)
foreground_pgroup::reset()
}
}
}
}
// It's a simpler version of fish shell's external process handling.
#[cfg(unix)]
mod fg_process_setup {
mod foreground_pgroup {
use nix::{
sys::signal,
libc,
sys::signal::{sigaction, SaFlags, SigAction, SigHandler, SigSet, Signal},
unistd::{self, Pid},
};
use std::io::IsTerminal;
use std::os::unix::prelude::{CommandExt, RawFd};
use std::{
os::unix::prelude::CommandExt,
process::{Child, Command},
};
// TODO: when raising MSRV past 1.63.0, switch to OwnedFd
struct TtyHandle(RawFd);
impl Drop for TtyHandle {
fn drop(&mut self) {
let _ = unistd::close(self.0);
}
}
pub(super) fn prepare_to_foreground(
external_command: &mut std::process::Command,
existing_pgrp: u32,
interactive: bool,
) {
let tty = TtyHandle(unistd::dup(nix::libc::STDIN_FILENO).expect("dup"));
let interactive = interactive && std::io::stdin().is_terminal();
pub fn prepare_command(external_command: &mut Command, existing_pgrp: u32) {
unsafe {
// Safety:
// POSIX only allows async-signal-safe functions to be called.
// `sigprocmask`, `setpgid` and `tcsetpgrp` are async-signal-safe according to:
// `sigaction` and `getpid` are async-signal-safe according to:
// https://manpages.ubuntu.com/manpages/bionic/man7/signal-safety.7.html
// Also, `set_foreground_pid` is async-signal-safe.
external_command.pre_exec(move || {
// When this callback is run, std::process has already done:
// - pthread_sigmask(SIG_SETMASK) with an empty sigset
// - signal(SIGPIPE, SIG_DFL)
// However, we do need TTOU/TTIN blocked again during this setup.
let mut sigset = signal::SigSet::empty();
sigset.add(signal::Signal::SIGTSTP);
sigset.add(signal::Signal::SIGTTOU);
sigset.add(signal::Signal::SIGTTIN);
sigset.add(signal::Signal::SIGCHLD);
signal::sigprocmask(signal::SigmaskHow::SIG_BLOCK, Some(&sigset), None)
.expect("signal mask");
// When this callback is run, std::process has already:
// - reset SIGPIPE to SIG_DFL
// According to glibc's job control manual:
// https://www.gnu.org/software/libc/manual/html_node/Launching-Jobs.html
// This has to be done *both* in the parent and here in the child due to race conditions.
if interactive {
set_foreground_pid(unistd::getpid(), existing_pgrp, tty.0);
}
set_foreground_pid(Pid::this(), existing_pgrp);
// Now let the child process have all the signals by resetting with SIG_SETMASK.
let mut sigset = signal::SigSet::empty();
sigset.add(signal::Signal::SIGTSTP); // for now not really all: we don't support background jobs, so keep this one blocked
signal::sigprocmask(signal::SigmaskHow::SIG_SETMASK, Some(&sigset), None)
.expect("signal mask");
// Reset signal handlers for child, sync with `terminal.rs`
let default = SigAction::new(SigHandler::SigDfl, SaFlags::empty(), SigSet::empty());
// SIGINT has special handling
let _ = sigaction(Signal::SIGQUIT, &default);
// We don't support background jobs, so keep some signals blocked for now
// let _ = sigaction(Signal::SIGTSTP, &default);
// let _ = sigaction(Signal::SIGTTIN, &default);
// let _ = sigaction(Signal::SIGTTOU, &default);
let _ = sigaction(Signal::SIGTERM, &default);
Ok(())
});
}
}
pub(super) fn set_foreground(
process: &std::process::Child,
existing_pgrp: u32,
interactive: bool,
) {
// called from the parent shell process - do the stdin tty check here
if interactive && std::io::stdin().is_terminal() {
set_foreground_pid(
Pid::from_raw(process.id() as i32),
existing_pgrp,
nix::libc::STDIN_FILENO,
);
}
pub fn set(process: &Child, existing_pgrp: u32) {
set_foreground_pid(Pid::from_raw(process.id() as i32), existing_pgrp);
}
// existing_pgrp is 0 when we don't have an existing foreground process in the pipeline.
// Conveniently, 0 means "current pid" to setpgid. But not to tcsetpgrp.
fn set_foreground_pid(pid: Pid, existing_pgrp: u32, tty: RawFd) {
let _ = unistd::setpgid(pid, Pid::from_raw(existing_pgrp as i32));
let _ = unistd::tcsetpgrp(
tty,
if existing_pgrp == 0 {
fn set_foreground_pid(pid: Pid, existing_pgrp: u32) {
// Safety: needs to be async-signal-safe.
// `setpgid` and `tcsetpgrp` are async-signal-safe.
// `existing_pgrp` is 0 when we don't have an existing foreground process in the pipeline.
// A pgrp of 0 means the calling process's pid for `setpgid`. But not for `tcsetpgrp`.
let pgrp = if existing_pgrp == 0 {
pid
} else {
Pid::from_raw(existing_pgrp as i32)
},
);
};
let _ = unistd::setpgid(pid, pgrp);
let _ = unistd::tcsetpgrp(libc::STDIN_FILENO, pgrp);
}
/// Reset the foreground process group to the shell
pub(super) fn reset_foreground_id(interactive: bool) {
if interactive && std::io::stdin().is_terminal() {
if let Err(e) = nix::unistd::tcsetpgrp(nix::libc::STDIN_FILENO, unistd::getpgrp()) {
println!("ERROR: reset foreground id failed, tcsetpgrp result: {e:?}");
}
pub fn reset() {
if let Err(e) = unistd::tcsetpgrp(libc::STDIN_FILENO, unistd::getpgrp()) {
eprintln!("ERROR: reset foreground id failed, tcsetpgrp result: {e:?}");
}
}
}
#[cfg(not(unix))]
mod fg_process_setup {
pub(super) fn prepare_to_foreground(_: &mut std::process::Command, _: u32, _: bool) {}
pub(super) fn set_foreground(_: &std::process::Child, _: u32, _: bool) {}
pub(super) fn reset_foreground_id(_: bool) {}
}

2
crates/nu-system/src/lib.rs
View file

@ -7,7 +7,7 @@ pub mod os_info;
#[cfg(target_os = "windows")]
mod windows;
pub use self::foreground::{ForegroundChild, ForegroundProcess};
pub use self::foreground::ForegroundChild;
#[cfg(any(target_os = "android", target_os = "linux"))]
pub use self::linux::*;
#[cfg(target_os = "macos")]

7
src/main.rs
View file

@ -4,6 +4,7 @@ mod ide;
mod logger;
mod run;
mod signals;
#[cfg(unix)]
mod terminal;
mod test_bins;
#[cfg(test)]
@ -17,7 +18,6 @@ use crate::{
command::parse_commandline_args,
config_files::set_config_path,
logger::{configure, logger},
terminal::acquire_terminal,
};
use command::gather_commandline_args;
use log::Level;
@ -185,8 +185,10 @@ fn main() -> Result<()> {
use_color,
);
#[cfg(unix)]
{
start_time = std::time::Instant::now();
acquire_terminal(engine_state.is_interactive);
terminal::acquire(engine_state.is_interactive);
perf(
"acquire_terminal",
start_time,
@ -195,6 +197,7 @@ fn main() -> Result<()> {
column!(),
use_color,
);
}
if let Some(include_path) = &parsed_nu_cli_args.include_path {
let span = include_path.span;

98
src/terminal.rs
View file

@ -1,63 +1,46 @@
#[cfg(unix)]
use std::{
io::IsTerminal,
sync::atomic::{AtomicI32, Ordering},
};
#[cfg(unix)]
use nix::{
errno::Errno,
libc,
sys::signal::{self, raise, signal, SaFlags, SigAction, SigHandler, SigSet, Signal},
sys::signal::{killpg, raise, sigaction, SaFlags, SigAction, SigHandler, SigSet, Signal},
unistd::{self, Pid},
};
#[cfg(unix)]
static INITIAL_PGID: AtomicI32 = AtomicI32::new(-1);
#[cfg(unix)]
pub(crate) fn acquire_terminal(interactive: bool) {
pub(crate) fn acquire(interactive: bool) {
if interactive && std::io::stdin().is_terminal() {
// see also: https://www.gnu.org/software/libc/manual/html_node/Initializing-the-Shell.html
if unsafe { libc::atexit(restore_terminal) } != 0 {
eprintln!("ERROR: failed to set exit function");
std::process::exit(1);
};
let initial_pgid = take_control();
INITIAL_PGID.store(initial_pgid.into(), Ordering::Relaxed);
unsafe {
if libc::atexit(restore_terminal) != 0 {
eprintln!("ERROR: failed to set exit function");
std::process::exit(1);
};
// SIGINT has special handling
signal(Signal::SIGQUIT, SigHandler::SigIgn).expect("signal ignore");
signal(Signal::SIGTSTP, SigHandler::SigIgn).expect("signal ignore");
signal(Signal::SIGTTIN, SigHandler::SigIgn).expect("signal ignore");
signal(Signal::SIGTTOU, SigHandler::SigIgn).expect("signal ignore");
// TODO: determine if this is necessary or not, since this breaks `rm` on macOS
// signal(Signal::SIGCHLD, SigHandler::SigIgn).expect("signal ignore");
signal_hook::low_level::register(signal_hook::consts::SIGTERM, || {
// Safety: can only call async-signal-safe functions here
// restore_terminal, signal, and raise are all async-signal-safe
restore_terminal();
if signal(Signal::SIGTERM, SigHandler::SigDfl).is_err() {
// Failed to set signal handler to default.
// This should not be possible, but if it does happen,
// then this could result in an infitite loop due to the raise below.
// So, we'll just exit immediately if this happens.
std::process::exit(1);
};
if raise(Signal::SIGTERM).is_err() {
std::process::exit(1);
};
})
.expect("signal hook");
let ignore = SigAction::new(SigHandler::SigIgn, SaFlags::empty(), SigSet::empty());
sigaction(Signal::SIGQUIT, &ignore).expect("signal ignore");
sigaction(Signal::SIGTSTP, &ignore).expect("signal ignore");
sigaction(Signal::SIGTTIN, &ignore).expect("signal ignore");
sigaction(Signal::SIGTTOU, &ignore).expect("signal ignore");
sigaction(
Signal::SIGTERM,
&SigAction::new(
SigHandler::Handler(sigterm_handler),
SaFlags::empty(),
SigSet::empty(),
),
)
.expect("signal action");
}
// Put ourselves in our own process group, if not already
@ -78,12 +61,8 @@ pub(crate) fn acquire_terminal(interactive: bool) {
}
}
#[cfg(not(unix))]
pub(crate) fn acquire_terminal(_: bool) {}
// Inspired by fish's acquire_tty_or_exit
// Returns our original pgid
#[cfg(unix)]
fn take_control() -> Pid {
let shell_pgid = unistd::getpgrp();
@ -101,16 +80,12 @@ fn take_control() -> Pid {
}
// Reset all signal handlers to default
let default = SigAction::new(SigHandler::SigDfl, SaFlags::empty(), SigSet::empty());
for sig in Signal::iterator() {
unsafe {
if let Ok(old_act) = signal::sigaction(
sig,
&SigAction::new(SigHandler::SigDfl, SaFlags::empty(), SigSet::empty()),
) {
if let Ok(old_act) = unsafe { sigaction(sig, &default) } {
// fish preserves ignored SIGHUP, presumably for nohup support, so let's do the same
if sig == Signal::SIGHUP && old_act.handler() == SigHandler::SigIgn {
let _ = signal::sigaction(sig, &old_act);
}
let _ = unsafe { sigaction(sig, &old_act) };
}
}
}
@ -131,7 +106,7 @@ fn take_control() -> Pid {
}
_ => {
// fish also has other heuristics than "too many attempts" for the orphan check, but they're optional
if signal::killpg(shell_pgid, Signal::SIGTTIN).is_err() {
if killpg(shell_pgid, Signal::SIGTTIN).is_err() {
eprintln!("ERROR: failed to SIGTTIN ourselves");
std::process::exit(1);
}
@ -143,12 +118,31 @@ fn take_control() -> Pid {
std::process::exit(1);
}
#[cfg(unix)]
extern "C" fn restore_terminal() {
// Safety: can only call async-signal-safe functions here
// tcsetpgrp and getpgrp are async-signal-safe
// `tcsetpgrp` and `getpgrp` are async-signal-safe
let initial_pgid = Pid::from_raw(INITIAL_PGID.load(Ordering::Relaxed));
if initial_pgid.as_raw() > 0 && initial_pgid != unistd::getpgrp() {
let _ = unistd::tcsetpgrp(libc::STDIN_FILENO, initial_pgid);
}
}
extern "C" fn sigterm_handler(_signum: libc::c_int) {
// Safety: can only call async-signal-safe functions here
// `restore_terminal`, `sigaction`, `raise`, and `_exit` are all async-signal-safe
restore_terminal();
let default = SigAction::new(SigHandler::SigDfl, SaFlags::empty(), SigSet::empty());
if unsafe { sigaction(Signal::SIGTERM, &default) }.is_err() {
// Failed to set signal handler to default.
// This should not be possible, but if it does happen,
// then this could result in an infinite loop due to the raise below.
// So, we'll just exit immediately if this happens.
unsafe { libc::_exit(1) };
};
if raise(Signal::SIGTERM).is_err() {
unsafe { libc::_exit(1) };
};
}