Merge pull request #393 from grisenti/main

jgarzik · web-flow · commit 0773ce6b1ac7 · 2025-04-02T14:12:11.000-04:00
sh: fixes
diff --git a/sh/src/builtin/read.rs b/sh/src/builtin/read.rs
@@ -81,7 +81,7 @@ fn read_until_from_non_blocking_fd(
             }
         }
         // might receive signals while reading
-        shell.update_global_state();
+        shell.handle_async_events();
         std::thread::sleep(Duration::from_millis(16));
     }
     if !buffer.is_empty() {
diff --git a/sh/src/builtin/trap.rs b/sh/src/builtin/trap.rs
@@ -14,7 +14,7 @@ use crate::signals::Signal;
 use std::fmt::Display;
 use std::str::FromStr;
 
-#[derive(Clone)]
+#[derive(Clone, PartialEq, Eq)]
 pub enum TrapAction {
     Default,
     Ignore,
diff --git a/sh/src/cli/vi/mod.rs b/sh/src/cli/vi/mod.rs
@@ -821,6 +821,13 @@ impl ViEditor {
         }
         Ok(Action::None)
     }
+
+    pub fn reset_current_line(&mut self) {
+        self.edit_line.clear();
+        self.cursor.position = 0;
+        self.current_history_command = 0;
+        self.mode = EditorMode::Insert;
+    }
 }
 
 impl Default for ViEditor {
diff --git a/sh/src/main.rs b/sh/src/main.rs
@@ -11,13 +11,13 @@ use crate::cli::args::{parse_args, ExecutionMode};
 use crate::cli::terminal::is_attached_to_terminal;
 use crate::cli::{clear_line, set_cursor_pos};
 use crate::shell::Shell;
-use crate::signals::setup_signal_handling;
+use crate::signals::{
+    handle_signal_ignore, handle_signal_write_to_signal_buffer, setup_signal_handling, Signal,
+};
 use crate::utils::is_process_in_foreground;
 use cli::terminal::read_nonblocking_char;
 use cli::vi::{Action, ViEditor};
 use gettextrs::{bind_textdomain_codeset, setlocale, textdomain, LocaleCategory};
-use nix::sys::signal::{sigaction, SaFlags, SigAction, SigSet};
-use nix::sys::signal::{SigHandler, Signal as NixSignal};
 use std::error::Error;
 use std::io;
 use std::io::Write;
@@ -140,7 +140,14 @@ fn standard_repl(shell: &mut Shell) {
             flush_stdout();
         }
         std::thread::sleep(Duration::from_millis(16));
-        shell.update_global_state();
+        shell.signal_manager.reset_sigint_count();
+        shell.handle_async_events();
+        if shell.signal_manager.get_sigint_count() > 0 {
+            program_buffer.clear();
+            line_buffer.clear();
+            println!();
+            eprint!("{}", shell.get_ps1());
+        }
         if shell.set_options.vi {
             return;
         }
@@ -149,7 +156,7 @@ fn standard_repl(shell: &mut Shell) {
 
 fn vi_repl(shell: &mut Shell) {
     let mut editor = ViEditor::default();
-    let mut buffer = Vec::new();
+    let mut program_buffer = Vec::new();
     let mut print_ps2 = false;
     clear_line();
     flush_stdout();
@@ -158,29 +165,29 @@ fn vi_repl(shell: &mut Shell) {
         while let Some(c) = read_nonblocking_char() {
             match editor.process_new_input(c, shell) {
                 Ok(Action::Execute(command)) => {
-                    buffer.extend(command.iter());
-                    if buffer.ends_with(b"\\\n") {
+                    program_buffer.extend(command.iter());
+                    if program_buffer.ends_with(b"\\\n") {
                         continue;
                     }
-                    let program_string = match std::str::from_utf8(&buffer) {
+                    let program_string = match std::str::from_utf8(&program_buffer) {
                         Ok(buf) => buf,
                         Err(_) => {
                             eprintln!("sh: invalid utf-8 sequence");
-                            buffer.clear();
+                            program_buffer.clear();
                             continue;
                         }
                     };
                     println!();
                     shell.terminal.reset();
                     match shell.execute_program(program_string) {
                         Ok(_) => {
-                            buffer.clear();
+                            program_buffer.clear();
                             print_ps2 = false;
                         }
                         Err(syntax_err) => {
                             if !syntax_err.could_be_resolved_with_more_input {
                                 eprintln!("sh: syntax error: {}", syntax_err.message);
-                                buffer.clear();
+                                program_buffer.clear();
                             } else {
                                 print_ps2 = true;
                             }
@@ -205,7 +212,14 @@ fn vi_repl(shell: &mut Shell) {
             flush_stdout()
         }
         std::thread::sleep(Duration::from_millis(16));
-        shell.update_global_state();
+        shell.signal_manager.reset_sigint_count();
+        shell.handle_async_events();
+        if shell.signal_manager.get_sigint_count() > 0 {
+            program_buffer.clear();
+            editor.reset_current_line();
+            println!();
+            eprint!("{}", shell.get_ps1());
+        }
         if !shell.set_options.vi {
             return;
         }
@@ -218,24 +232,14 @@ fn interactive_shell(shell: &mut Shell) {
         nix::unistd::tcsetpgrp(io::stdin().as_fd(), pgid).unwrap();
     }
     shell.terminal.set_nonblocking_no_echo();
-    let ignore_action = SigAction::new(SigHandler::SigIgn, SaFlags::empty(), SigSet::empty());
-    unsafe {
-        sigaction(NixSignal::SIGQUIT, &ignore_action).unwrap();
-    }
-    unsafe {
-        sigaction(NixSignal::SIGTERM, &ignore_action).unwrap();
-    }
+    unsafe { handle_signal_ignore(Signal::SigQuit) }
+    unsafe { handle_signal_ignore(Signal::SigTerm) }
+    unsafe { handle_signal_write_to_signal_buffer(Signal::SigInt) }
     if shell.set_options.monitor {
         // job control signals
-        unsafe {
-            sigaction(NixSignal::SIGTTIN, &ignore_action).unwrap();
-        }
-        unsafe {
-            sigaction(NixSignal::SIGTTOU, &ignore_action).unwrap();
-        }
-        unsafe {
-            sigaction(NixSignal::SIGTSTP, &ignore_action).unwrap();
-        }
+        unsafe { handle_signal_ignore(Signal::SigTtin) }
+        unsafe { handle_signal_ignore(Signal::SigTtou) }
+        unsafe { handle_signal_ignore(Signal::SigTstp) }
     }
     loop {
         if shell.set_options.vi {
diff --git a/sh/src/shell/mod.rs b/sh/src/shell/mod.rs
@@ -34,8 +34,10 @@ use crate::utils::{
 use crate::wordexp::{expand_word, expand_word_to_string, word_to_pattern};
 use nix::errno::Errno;
 use nix::libc;
+use nix::sys::signal::kill;
+use nix::sys::signal::Signal as NixSignal;
 use nix::sys::wait::{WaitPidFlag, WaitStatus};
-use nix::unistd::{getcwd, getpgrp, getpid, getppid, setpgid, ForkResult, Pid};
+use nix::unistd::{getcwd, getpgid, getpgrp, getpid, getppid, setpgid, tcsetpgrp, ForkResult, Pid};
 use std::collections::HashMap;
 use std::ffi::{CString, OsString};
 use std::fmt::{Display, Formatter};
@@ -217,15 +219,15 @@ impl Shell {
                     return Ok(signal_to_exit_status(signal));
                 }
                 WaitStatus::StillAlive => {
-                    self.update_global_state();
+                    self.handle_async_events();
                     std::thread::sleep(Duration::from_millis(16));
                 }
                 _ => unreachable!(),
             }
         }
     }
 
-    pub fn update_global_state(&mut self) {
+    pub fn handle_async_events(&mut self) {
         self.process_signals();
         if self.set_options.monitor {
             if let Err(err) = self.background_jobs.update_jobs() {
@@ -262,8 +264,8 @@ impl Shell {
     }
 
     pub fn process_signals(&mut self) {
-        while let Some(action) = self.signal_manager.get_pending_action() {
-            self.execute_action(action.clone())
+        while let Some(action) = self.signal_manager.get_pending_action().cloned() {
+            self.execute_action(action)
         }
     }
 
@@ -729,17 +731,22 @@ impl Shell {
             match fork()? {
                 ForkResult::Child => {
                     self.become_subshell();
-                    setpgid(Pid::from_raw(0), Pid::from_raw(0))
-                        .expect("failed to create new process group for pipeline");
+                    // this should never fail as both arguments are valid
+                    setpgid(Pid::from_raw(0), Pid::from_raw(0)).unwrap();
                     let pipeline_pgid = getpgrp();
+                    // wait for the parent process to put the subshell in the foreground
+                    if let Err(err) = kill(Pid::from_raw(0), NixSignal::SIGTSTP) {
+                        self.eprint(&format!("sh: internal call to kill failed ({err})"));
+                        self.exit(1);
+                    }
 
                     let mut current_stdin = libc::STDIN_FILENO;
                     for command in pipeline.commands.head() {
                         let (read_pipe, write_pipe) = pipe()?;
                         match fork()? {
                             ForkResult::Child => {
-                                setpgid(Pid::from_raw(0), pipeline_pgid)
-                                    .expect("failed to set pipeline pgid");
+                                // should never fail as `pipeline_pgid` is a valid process group
+                                setpgid(Pid::from_raw(0), pipeline_pgid).unwrap();
                                 drop(read_pipe);
                                 dup2(current_stdin, libc::STDIN_FILENO)?;
                                 dup2(write_pipe.as_raw_fd(), libc::STDOUT_FILENO)?;
@@ -763,12 +770,46 @@ impl Shell {
                     self.exit(return_status);
                 }
                 ForkResult::Parent { child } => {
-                    if is_process_in_foreground() {
-                        nix::unistd::tcsetpgrp(io::stdin().as_fd(), child).unwrap();
-                        pipeline_exit_status = self.wait_child_process(child)?;
-                        nix::unistd::tcsetpgrp(io::stdin().as_fd(), getpgrp()).unwrap();
-                    } else {
-                        pipeline_exit_status = self.wait_child_process(child)?;
+                    loop {
+                        match waitpid(child, Some(WaitPidFlag::WNOHANG | WaitPidFlag::WUNTRACED))? {
+                            WaitStatus::Exited(_, _) => {
+                                // the only way this happened is if there was an error before going
+                                // the child went to sleep
+                                return Ok(1);
+                            }
+                            WaitStatus::Signaled(_, _, _) => {
+                                self.eprint("sh: unsynchronised pipeline was terminated by another process\n");
+                                return Ok(1);
+                            }
+                            WaitStatus::Continued(_) => {
+                                self.eprint("sh: unsynchronised pipeline was restarted by another process\n");
+                                return Ok(1);
+                            }
+                            WaitStatus::Stopped(_, _) => {
+                                if is_process_in_foreground() {
+                                    // should never fail as child is a valid process id and
+                                    // in the same session as the current shell
+                                    let child_gpid = getpgid(Some(child)).unwrap();
+                                    // should never fail as stdin is a valid file descriptor and
+                                    // child gpid is valid and in the same session
+                                    tcsetpgrp(io::stdin().as_fd(), child_gpid).unwrap();
+                                    kill(child, NixSignal::SIGCONT).unwrap();
+                                    pipeline_exit_status = self.wait_child_process(child)?;
+                                    // should never fail
+                                    tcsetpgrp(io::stdin().as_fd(), getpgrp()).unwrap();
+                                    break;
+                                } else {
+                                    kill(child, NixSignal::SIGCONT).unwrap();
+                                    pipeline_exit_status = self.wait_child_process(child)?;
+                                    break;
+                                }
+                            }
+                            WaitStatus::StillAlive => {
+                                self.handle_async_events();
+                                std::thread::sleep(Duration::from_millis(16));
+                            }
+                            _ => unreachable!(),
+                        }
                     }
                 }
             }
@@ -958,6 +999,7 @@ impl Shell {
             history,
             set_options,
             is_interactive,
+            signal_manager: SignalManager::new(is_interactive),
             ..Default::default()
         }
     }
@@ -1019,7 +1061,7 @@ impl Default for Shell {
             is_interactive: false,
             last_lineno: 0,
             exit_action: TrapAction::Default,
-            signal_manager: SignalManager::default(),
+            signal_manager: SignalManager::new(false),
             background_jobs: JobManager::default(),
             history: History::new(32767),
             umask: !0o022 & 0o777,
diff --git a/sh/src/signals.rs b/sh/src/signals.rs

Original file line number	Diff line number	Diff line change
`@@ -81,7 +81,7 @@ fn read_until_from_non_blocking_fd(`
`81`	`81`	`}`
`82`	`82`	`}`
`83`	`83`	`// might receive signals while reading`
`84`		`- shell.update_global_state();`
	`84`	`+ shell.handle_async_events();`
`85`	`85`	`std::thread::sleep(Duration::from_millis(16));`
`86`	`86`	`}`
`87`	`87`	`if !buffer.is_empty() {`
Original file line number	Diff line number	Diff line change
`@@ -821,6 +821,13 @@ impl ViEditor {`
`821`	`821`	`}`
`822`	`822`	`Ok(Action::None)`
`823`	`823`	`}`
	`824`	`+`
	`825`	`+ pub fn reset_current_line(&mut self) {`
	`826`	`+ self.edit_line.clear();`
	`827`	`+ self.cursor.position = 0;`
	`828`	`+ self.current_history_command = 0;`
	`829`	`+ self.mode = EditorMode::Insert;`
	`830`	`+ }`
`824`	`831`	`}`
`825`	`832`
`826`	`833`	`impl Default for ViEditor {`