From c39a8e159823f22548cc18f387cb2b3f627ab4ff Mon Sep 17 00:00:00 2001 From: reuben olinsky Date: Fri, 14 Jun 2024 17:08:48 -0700 Subject: [PATCH] fix: allow interrupting read builtin, run pipeline cmds in subshell --- brush-core/Cargo.toml | 2 + brush-core/src/builtins/read.rs | 97 +++++++++++++++++----- brush-core/src/extendedtests.rs | 10 ++- brush-core/src/interp.rs | 39 ++++++--- brush-core/src/openfiles.rs | 50 +++++++++++ brush-shell/tests/cases/builtins/read.yaml | 18 ++++ brush-shell/tests/cases/pipeline.yaml | 11 +++ 7 files changed, 192 insertions(+), 35 deletions(-) diff --git a/brush-core/Cargo.toml b/brush-core/Cargo.toml index 35b9bb8c..d341f34f 100644 --- a/brush-core/Cargo.toml +++ b/brush-core/Cargo.toml @@ -30,6 +30,8 @@ os_pipe = { version = "1.1.5", features = ["io_safety"] } rand = "0.8.5" thiserror = "1.0.61" tokio = { version = "1.37.0", features = [ + "fs", + "io-std", "io-util", "macros", "process", diff --git a/brush-core/src/builtins/read.rs b/brush-core/src/builtins/read.rs index 6a504458..e13cf2d9 100644 --- a/brush-core/src/builtins/read.rs +++ b/brush-core/src/builtins/read.rs @@ -1,5 +1,6 @@ use clap::Parser; -use std::{collections::VecDeque, io::Read}; +use std::collections::VecDeque; +use std::io::Read; use crate::{builtin, commands, env, error, openfiles, variables}; @@ -72,9 +73,6 @@ impl builtin::Command for ReadCommand { if self.raw_mode { tracing::debug!("read -r is not implemented"); } - if self.silent { - return error::unimp("read -s"); - } if self.timeout_in_seconds.is_some() { return error::unimp("read -t"); } @@ -82,9 +80,10 @@ impl builtin::Command for ReadCommand { return error::unimp("read -u"); } - let input_line = read_line(context.stdin()); + let input_line = self.read_line(context.stdin())?; if let Some(input_line) = input_line { let mut variable_names: VecDeque = self.variable_names.clone().into(); + let mut spillover: Option = None; for field in input_line.split_ascii_whitespace() { if let Some(variable_name) = variable_names.pop_front() { context.shell.env.update_or_add( @@ -95,9 +94,26 @@ impl builtin::Command for ReadCommand { env::EnvironmentScope::Global, )?; } else { - return error::unimp("too few variable names"); + match &mut spillover { + Some(s) => { + s.push(' '); + s.push_str(field); + } + None => spillover = Some(field.to_owned()), + } } } + + if let Some(spillover) = spillover { + context.shell.env.update_or_add( + "REPLY", + variables::ShellValueLiteral::Scalar(spillover), + |_| Ok(()), + env::EnvironmentLookup::Anywhere, + env::EnvironmentScope::Global, + )?; + } + Ok(crate::builtin::ExitCode::Success) } else { Ok(crate::builtin::ExitCode::Custom(1)) @@ -105,26 +121,63 @@ impl builtin::Command for ReadCommand { } } -fn read_line(mut file: openfiles::OpenFile) -> Option { - let mut line = String::new(); - let mut buffer = [0; 1]; // 1-byte buffer +impl ReadCommand { + fn read_line(&self, mut file: openfiles::OpenFile) -> Result, error::Error> { + let orig_term_attr = file.get_term_attr()?; + if let Some(orig_term_attr) = &orig_term_attr { + let mut updated_term_attr = orig_term_attr.to_owned(); + + updated_term_attr + .local_flags + .remove(nix::sys::termios::LocalFlags::ICANON); + updated_term_attr + .local_flags + .remove(nix::sys::termios::LocalFlags::ISIG); + + if self.silent { + updated_term_attr + .local_flags + .remove(nix::sys::termios::LocalFlags::ECHO); + } + + file.set_term_attr(nix::sys::termios::SetArg::TCSANOW, &updated_term_attr)?; + } + + let mut line = String::new(); + let mut buffer = [0; 1]; // 1-byte buffer + + // TODO: Look at ignoring errors here. + while let Ok(n) = file.read(&mut buffer) { + if n == 0 { + break; // EOF reached. + } + + let ch = buffer[0] as char; + + if ch == '\x03' { + return Ok(None); // Ctrl+C aborts. + } + + if ch == '\n' { + break; // End of line reached + } + + // Ignore other control characters. + if ch.is_ascii_control() { + continue; + } - // TODO: Look at ignoring errors here. - while let Ok(n) = file.read(&mut buffer) { - if n == 0 { - break; // EOF reached + line.push(ch); } - let ch = buffer[0] as char; - if ch == '\n' { - break; // End of line reached + if let Some(orig_term_attr) = &orig_term_attr { + file.set_term_attr(nix::sys::termios::SetArg::TCSANOW, orig_term_attr)?; } - line.push(ch); - } - if line.is_empty() { - None - } else { - Some(line) + if line.is_empty() { + Ok(None) + } else { + Ok(Some(line)) + } } } diff --git a/brush-core/src/extendedtests.rs b/brush-core/src/extendedtests.rs index 1fe5ad75..81c92f67 100644 --- a/brush-core/src/extendedtests.rs +++ b/brush-core/src/extendedtests.rs @@ -115,7 +115,15 @@ pub(crate) fn apply_unary_predicate_to_str( } } ast::UnaryPredicate::FdIsOpenTerminal => { - error::unimp("unary extended test predicate: FdIsOpenTerminal") + if let Ok(fd) = operand.parse::() { + if let Some(open_file) = shell.open_files.files.get(&fd) { + Ok(open_file.is_term()) + } else { + Ok(false) + } + } else { + Ok(false) + } } ast::UnaryPredicate::FileExistsAndIsSetuid => { let path = Path::new(operand); diff --git a/brush-core/src/interp.rs b/brush-core/src/interp.rs index 67eb4488..441e4a18 100644 --- a/brush-core/src/interp.rs +++ b/brush-core/src/interp.rs @@ -79,7 +79,7 @@ struct PipelineExecutionContext<'a> { current_pipeline_index: usize, pipeline_len: usize, - output_pipes: Vec, + output_pipes: &'a mut Vec, params: ExecutionParameters, } @@ -301,20 +301,35 @@ impl Execute for ast::Pipeline { shell: &mut Shell, params: &ExecutionParameters, ) -> Result { - let mut pipeline_context = PipelineExecutionContext { - shell, - current_pipeline_index: 0, - pipeline_len: self.seq.len(), - output_pipes: vec![], - params: params.clone(), - }; - + let pipeline_len = self.seq.len(); + let mut output_pipes = vec![]; let mut spawn_results = VecDeque::new(); - for command in &self.seq { - let spawn_result = command.execute_in_pipeline(&mut pipeline_context).await?; + for (current_pipeline_index, command) in self.seq.iter().enumerate() { + // If there's only one command in the pipeline, then we run directly in the current shell. + // Otherwise, we spawn a separate subshell for each command in the pipeline. + let spawn_result = if pipeline_len > 1 { + let mut subshell = shell.clone(); + let mut pipeline_context = PipelineExecutionContext { + shell: &mut subshell, + current_pipeline_index, + pipeline_len, + output_pipes: &mut output_pipes, + params: params.clone(), + }; + command.execute_in_pipeline(&mut pipeline_context).await? + } else { + let mut pipeline_context = PipelineExecutionContext { + shell, + current_pipeline_index, + pipeline_len, + output_pipes: &mut output_pipes, + params: params.clone(), + }; + command.execute_in_pipeline(&mut pipeline_context).await? + }; + spawn_results.push_back(spawn_result); - pipeline_context.current_pipeline_index += 1; } let mut result = ExecutionResult::success(); diff --git a/brush-core/src/openfiles.rs b/brush-core/src/openfiles.rs index 6b7694e6..3c20905a 100644 --- a/brush-core/src/openfiles.rs +++ b/brush-core/src/openfiles.rs @@ -1,4 +1,5 @@ use std::collections::HashMap; +use std::io::IsTerminal; #[cfg(unix)] use std::os::fd::AsFd; #[cfg(unix)] @@ -76,6 +77,55 @@ impl OpenFile { OpenFile::HereDocument(_) => error::unimp("as_raw_fd for here doc"), } } + + pub(crate) fn is_term(&self) -> bool { + match self { + OpenFile::Stdin => std::io::stdin().is_terminal(), + OpenFile::Stdout => std::io::stdout().is_terminal(), + OpenFile::Stderr => std::io::stderr().is_terminal(), + OpenFile::Null => false, + OpenFile::File(f) => f.is_terminal(), + OpenFile::PipeReader(_) => false, + OpenFile::PipeWriter(_) => false, + OpenFile::HereDocument(_) => false, + } + } + + pub(crate) fn get_term_attr(&self) -> Result, error::Error> { + if !self.is_term() { + return Ok(None); + } + + let result = match self { + OpenFile::Stdin => Some(nix::sys::termios::tcgetattr(std::io::stdin())?), + OpenFile::Stdout => Some(nix::sys::termios::tcgetattr(std::io::stdout())?), + OpenFile::Stderr => Some(nix::sys::termios::tcgetattr(std::io::stderr())?), + OpenFile::Null => None, + OpenFile::File(f) => Some(nix::sys::termios::tcgetattr(f)?), + OpenFile::PipeReader(_) => None, + OpenFile::PipeWriter(_) => None, + OpenFile::HereDocument(_) => None, + }; + Ok(result) + } + + pub(crate) fn set_term_attr( + &self, + action: nix::sys::termios::SetArg, + termios: &nix::sys::termios::Termios, + ) -> Result<(), error::Error> { + match self { + OpenFile::Stdin => nix::sys::termios::tcsetattr(std::io::stdin(), action, termios)?, + OpenFile::Stdout => nix::sys::termios::tcsetattr(std::io::stdout(), action, termios)?, + OpenFile::Stderr => nix::sys::termios::tcsetattr(std::io::stderr(), action, termios)?, + OpenFile::Null => (), + OpenFile::File(f) => nix::sys::termios::tcsetattr(f, action, termios)?, + OpenFile::PipeReader(_) => (), + OpenFile::PipeWriter(_) => (), + OpenFile::HereDocument(_) => (), + } + Ok(()) + } } impl From for Stdio { diff --git a/brush-shell/tests/cases/builtins/read.yaml b/brush-shell/tests/cases/builtins/read.yaml index b7064330..e1f0d8c4 100644 --- a/brush-shell/tests/cases/builtins/read.yaml +++ b/brush-shell/tests/cases/builtins/read.yaml @@ -17,3 +17,21 @@ cases: b stdin: | (echo a; echo b) | while read name; do echo "Hello, $name"; done + + - name: "read from here string" + known_failure: true + stdin: | + read myvar <<< "hello" + echo "myvar: ${myvar}" + + - name: "read from process substitution" + known_failure: true + stdin: | + read myvar < <(echo hello) + echo "myvar: ${myvar}" + + - name: "read from command substitution" + known_failure: true + stdin: | + read myvar <<< "$(echo hello)" + echo "myvar: ${myvar}" diff --git a/brush-shell/tests/cases/pipeline.yaml b/brush-shell/tests/cases/pipeline.yaml index 265ed9d7..4a60272d 100644 --- a/brush-shell/tests/cases/pipeline.yaml +++ b/brush-shell/tests/cases/pipeline.yaml @@ -25,3 +25,14 @@ cases: exit $1 stdin: | ./script.sh 10 | ./script.sh 0 | ./script.sh 33 + + - name: "Side effects in pipe commands" + stdin: | + var=0 + echo "var: ${var}" + { var=1; } + echo "var: ${var}" + { var=2; echo hi; } | cat + echo "var: ${var}" + echo hi | { var=3; cat; } + echo "var: ${var}"