Skip to content

Commit

Permalink
fix: correct multiple issues with process substitution + redirection
Browse files Browse the repository at this point in the history
  • Loading branch information
@reubeno
reubeno committed Aug 4, 2024
1 parent d917de8 commit 96ba8a0
Show file tree
Hide file tree
Showing 5 changed files with 169 additions and 82 deletions.
175 changes: 103 additions & 72 deletions brush-core/src/interp.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -851,26 +851,28 @@ impl ExecuteInPipeline for ast::SimpleCommand {
{
match item {
CommandPrefixOrSuffixItem::IoRedirect(redirect) => {
if let Some(installed_fd_num) =
setup_redirect(&mut open_files, context.shell, redirect).await?
if setup_redirect(&mut open_files, context.shell, redirect)
.await?
.is_none()
{
if matches!(
redirect,
ast::IoRedirect::File(
_,
_,
ast::IoFileRedirectTarget::ProcessSubstitution(_)
)
) {
args.push(CommandArg::String(std::format!(
"/dev/fd/{installed_fd_num}"
)));
}
} else {
// Something went wrong.
return Ok(SpawnResult::ImmediateExit(1));
}
}
CommandPrefixOrSuffixItem::ProcessSubstitution(kind, subshell_command) => {
let (installed_fd_num, substitution_file) = setup_process_substitution(
&mut open_files,
context.shell,
kind,
subshell_command,
)?;

open_files.files.insert(installed_fd_num, substitution_file);

args.push(CommandArg::String(std::format!(
"/dev/fd/{installed_fd_num}"
)));
}
CommandPrefixOrSuffixItem::AssignmentWord(assignment, word) => {
if args.is_empty() {
// If we haven't yet seen any arguments, then this must be a proper
Expand Down Expand Up @@ -1309,42 +1311,35 @@ pub(crate) async fn setup_redirect<'a>(
ast::IoFileRedirectTarget::Filename(f) => {
let mut options = std::fs::File::options();

let default_fd_if_unspecified;
let default_fd_if_unspecified = get_default_fd_for_redirect_kind(kind);
match kind {
ast::IoFileRedirectKind::Read => {
default_fd_if_unspecified = 0;
options.read(true);
}
ast::IoFileRedirectKind::Write => {
// TODO: honor noclobber options
default_fd_if_unspecified = 1;
options.create(true);
options.write(true);
options.truncate(true);
}
ast::IoFileRedirectKind::Append => {
default_fd_if_unspecified = 1;
options.create(true);
options.append(true);
}
ast::IoFileRedirectKind::ReadAndWrite => {
default_fd_if_unspecified = 0;
options.create(true);
options.read(true);
options.write(true);
}
ast::IoFileRedirectKind::Clobber => {
default_fd_if_unspecified = 1;
options.create(true);
options.write(true);
options.truncate(true);
}
ast::IoFileRedirectKind::DuplicateInput => {
default_fd_if_unspecified = 0;
options.read(true);
}
ast::IoFileRedirectKind::DuplicateOutput => {
default_fd_if_unspecified = 1;
options.create(true);
options.write(true);
}
Expand Down Expand Up @@ -1389,56 +1384,25 @@ pub(crate) async fn setup_redirect<'a>(
return Ok(None);
}
}
ast::IoFileRedirectTarget::ProcessSubstitution(ast::SubshellCommand(
subshell_cmd,
)) => {
ast::IoFileRedirectTarget::ProcessSubstitution(substitution_kind, subshell_cmd) => {
match kind {
ast::IoFileRedirectKind::Read | ast::IoFileRedirectKind::Write => {
// TODO: Don't execute synchronously!
// Execute in a subshell.
let mut subshell = shell.clone();

// Set up pipe so we can connect to the command.
let (reader, writer) = sys::pipes::pipe()?;

if matches!(kind, ast::IoFileRedirectKind::Read) {
subshell
.open_files
.files
.insert(1, openfiles::OpenFile::PipeWriter(writer));
target_file = OpenFile::PipeReader(reader);
} else {
subshell
.open_files
.files
.insert(0, openfiles::OpenFile::PipeReader(reader));
target_file = OpenFile::PipeWriter(writer);
}

let exec_params = ExecutionParameters {
open_files: subshell.open_files.clone(),
};

// TODO: inspect result of execution?
let _ = subshell_cmd.execute(&mut subshell, &exec_params).await?;

// Make sure the subshell + parameters are closed; among other
// things, this ensures they're not holding onto the write end
// of the pipe.
drop(exec_params);
drop(subshell);

// Starting at 63 (a.k.a. 64-1)--and decrementing--look for an
// available fd.
let mut candidate_fd_num = 63;
while open_files.files.contains_key(&candidate_fd_num) {
candidate_fd_num -= 1;
if candidate_fd_num == 0 {
return error::unimp("no available file descriptors");
}
}

fd_num = candidate_fd_num;
ast::IoFileRedirectKind::Read
| ast::IoFileRedirectKind::Write
| ast::IoFileRedirectKind::Append
| ast::IoFileRedirectKind::ReadAndWrite
| ast::IoFileRedirectKind::Clobber => {
let (substitution_fd, substitution_file) = setup_process_substitution(
open_files,
shell,
substitution_kind,
subshell_cmd,
)?;

target_file = substitution_file.try_dup()?;
open_files.files.insert(substitution_fd, substitution_file);

fd_num = specified_fd_num
.unwrap_or_else(|| get_default_fd_for_redirect_kind(kind));
}
_ => return error::unimp("invalid process substitution"),
}
Expand Down Expand Up @@ -1475,6 +1439,73 @@ pub(crate) async fn setup_redirect<'a>(
}
}

fn get_default_fd_for_redirect_kind(kind: &ast::IoFileRedirectKind) -> u32 {
match kind {
ast::IoFileRedirectKind::Read => 0,
ast::IoFileRedirectKind::Write => 1,
ast::IoFileRedirectKind::Append => 1,
ast::IoFileRedirectKind::ReadAndWrite => 0,
ast::IoFileRedirectKind::Clobber => 1,
ast::IoFileRedirectKind::DuplicateInput => 0,
ast::IoFileRedirectKind::DuplicateOutput => 1,
}
}

fn setup_process_substitution(
open_files: &mut OpenFiles,
shell: &mut Shell,
kind: &ast::ProcessSubstitutionKind,
subshell_cmd: &ast::SubshellCommand,
) -> Result<(u32, OpenFile), error::Error> {
// TODO: Don't execute synchronously!
// Execute in a subshell.
let mut subshell = shell.clone();

// Set up pipe so we can connect to the command.
let (reader, writer) = sys::pipes::pipe()?;

let target_file = match kind {
ast::ProcessSubstitutionKind::Read => {
subshell
.open_files
.files
.insert(1, openfiles::OpenFile::PipeWriter(writer));
OpenFile::PipeReader(reader)
}
ast::ProcessSubstitutionKind::Write => {
subshell
.open_files
.files
.insert(0, openfiles::OpenFile::PipeReader(reader));
OpenFile::PipeWriter(writer)
}
};

let exec_params = ExecutionParameters {
open_files: subshell.open_files.clone(),
};

// Asynchronously spawn off the subshell; we intentionally don't block on its
// completion.
let subshell_cmd = subshell_cmd.to_owned();
tokio::spawn(async move {
// Intentionally ignore the result of the subshell command.
let _ = subshell_cmd.0.execute(&mut subshell, &exec_params).await;
});

// Starting at 63 (a.k.a. 64-1)--and decrementing--look for an
// available fd.
let mut candidate_fd_num = 63;
while open_files.files.contains_key(&candidate_fd_num) {
candidate_fd_num -= 1;
if candidate_fd_num == 0 {
return error::unimp("no available file descriptors");
}
}

Ok((candidate_fd_num, target_file))
}

#[allow(unused_variables)]
fn setup_open_file_with_contents(contents: &str) -> Result<OpenFile, error::Error> {
let (reader, mut writer) = sys::pipes::pipe()?;
Expand Down
30 changes: 27 additions & 3 deletions brush-parser/src/ast.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -614,6 +614,25 @@ impl Display for CommandSuffix {
}
}

/// Represents the I/O direction of a process substitution.
#[derive(Clone, Debug)]
#[cfg_attr(feature = "fuzz-testing", derive(arbitrary::Arbitrary))]
pub enum ProcessSubstitutionKind {
/// The process is read from.
Read,
/// The process is written to.
Write,
}

impl Display for ProcessSubstitutionKind {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
ProcessSubstitutionKind::Read => write!(f, "<"),
ProcessSubstitutionKind::Write => write!(f, ">"),
}
}
}

/// A prefix or suffix for a simple command.
#[derive(Clone, Debug)]
#[cfg_attr(feature = "fuzz-testing", derive(arbitrary::Arbitrary))]
Expand All @@ -624,6 +643,8 @@ pub enum CommandPrefixOrSuffixItem {
Word(Word),
/// An assignment/declaration word.
AssignmentWord(Assignment, Word),
/// A process substitution.
ProcessSubstitution(ProcessSubstitutionKind, SubshellCommand),
}

impl Display for CommandPrefixOrSuffixItem {
Expand All @@ -632,6 +653,9 @@ impl Display for CommandPrefixOrSuffixItem {
CommandPrefixOrSuffixItem::IoRedirect(io_redirect) => write!(f, "{}", io_redirect),
CommandPrefixOrSuffixItem::Word(word) => write!(f, "{}", word),
CommandPrefixOrSuffixItem::AssignmentWord(_assignment, word) => write!(f, "{}", word),
CommandPrefixOrSuffixItem::ProcessSubstitution(kind, subshell_command) => {
write!(f, "{}({})", kind, subshell_command)
}
}
}
}
Expand Down Expand Up @@ -834,16 +858,16 @@ pub enum IoFileRedirectTarget {
Fd(u32),
/// Process substitution: substitution with the results of executing the given
/// command in a subshell.
ProcessSubstitution(SubshellCommand),
ProcessSubstitution(ProcessSubstitutionKind, SubshellCommand),
}

impl Display for IoFileRedirectTarget {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
IoFileRedirectTarget::Filename(word) => write!(f, "{}", word),
IoFileRedirectTarget::Fd(fd) => write!(f, "{}", fd),
IoFileRedirectTarget::ProcessSubstitution(subshell_command) => {
write!(f, "{}", subshell_command)
IoFileRedirectTarget::ProcessSubstitution(kind, subshell_command) => {
write!(f, "{kind}{subshell_command}")
}
}
}
Expand Down
20 changes: 15 additions & 5 deletions brush-parser/src/parser.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -558,6 +558,10 @@ peg::parser! {

rule cmd_suffix() -> ast::CommandSuffix =
s:(
non_posix_extensions_enabled() sub:process_substitution() {
let (kind, subshell) = sub;
ast::CommandPrefixOrSuffixItem::ProcessSubstitution(kind, subshell)
} /
i:io_redirect() {
ast::CommandPrefixOrSuffixItem::IoRedirect(i)
} /
Expand All @@ -577,9 +581,9 @@ peg::parser! {
// N.B. here strings are extensions to the POSIX standard.
rule io_redirect() -> ast::IoRedirect =
n:io_number()? f:io_file() {
let (kind, target) = f;
ast::IoRedirect::File(n, kind, target)
} /
let (kind, target) = f;
ast::IoRedirect::File(n, kind, target)
} /
non_posix_extensions_enabled() specific_operator("&>>") target:filename() { ast::IoRedirect::OutputAndError(ast::Word::from(target), true) } /
non_posix_extensions_enabled() specific_operator("&>") target:filename() { ast::IoRedirect::OutputAndError(ast::Word::from(target), false) } /
non_posix_extensions_enabled() n:io_number()? specific_operator("<<<") w:word() { ast::IoRedirect::HereString(n, ast::Word::from(w)) } /
Expand All @@ -588,10 +592,8 @@ peg::parser! {

// N.B. Process substitution forms are extensions to the POSIX standard.
rule io_file() -> (ast::IoFileRedirectKind, ast::IoFileRedirectTarget) =
non_posix_extensions_enabled() specific_operator("<") s:subshell() { (ast::IoFileRedirectKind::Read, ast::IoFileRedirectTarget::ProcessSubstitution(s)) } /
specific_operator("<") f:io_filename() { (ast::IoFileRedirectKind::Read, f) } /
specific_operator("<&") f:io_filename_or_fd() { (ast::IoFileRedirectKind::DuplicateInput, f) } /
non_posix_extensions_enabled() specific_operator(">") s:subshell() { (ast::IoFileRedirectKind::Write, ast::IoFileRedirectTarget::ProcessSubstitution(s)) } /
specific_operator(">") f:io_filename() { (ast::IoFileRedirectKind::Write, f) } /
specific_operator(">&") f:io_filename_or_fd() { (ast::IoFileRedirectKind::DuplicateOutput, f) } /
specific_operator(">>") f:io_filename() { (ast::IoFileRedirectKind::Append, f) } /
Expand All @@ -606,6 +608,10 @@ peg::parser! {
w:[Token::Word(_, _)] {? w.to_str().parse().or(Err("io_fd u32")) }

rule io_filename() -> ast::IoFileRedirectTarget =
non_posix_extensions_enabled() sub:process_substitution() {
let (kind, subshell) = sub;
ast::IoFileRedirectTarget::ProcessSubstitution(kind, subshell)
} /
f:filename() { ast::IoFileRedirectTarget::Filename(ast::Word::from(f)) }

rule filename() -> &'input Token =
Expand All @@ -618,6 +624,10 @@ peg::parser! {
rule here_end() -> &'input Token =
word()

rule process_substitution() -> (ast::ProcessSubstitutionKind, ast::SubshellCommand) =
specific_operator("<") s:subshell() { (ast::ProcessSubstitutionKind::Read, s) } /
specific_operator(">") s:subshell() { (ast::ProcessSubstitutionKind::Write, s) }

rule newline_list() -> () =
newline()+ {}

Expand Down
1 change: 0 additions & 1 deletion brush-shell/tests/cases/builtins/read.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ cases:
echo "myvar: ${myvar}"
- name: "read from process substitution"
known_failure: true
stdin: |
read myvar < <(echo hello)
echo "myvar: ${myvar}"
Loading

0 comments on commit 96ba8a0

Please sign in to comment.