From b4638d7d843dcaeb3446a7e7d77833356d3a12a2 Mon Sep 17 00:00:00 2001 From: reuben olinsky Date: Sat, 25 Jan 2025 07:44:10 -0800 Subject: [PATCH] fix(redirection): make sure redirection fd + operator are contiguous (#359) --- brush-parser/src/parser.rs | 19 +++++++++++-- brush-shell/tests/cases/redirection.yaml | 35 +++++++++++++++++++++++ brush-shell/tests/compat_tests.rs | 36 +++++++++++++++++++++--- 3 files changed, 83 insertions(+), 7 deletions(-) diff --git a/brush-parser/src/parser.rs b/brush-parser/src/parser.rs index 292eed1c..2fce40be 100644 --- a/brush-parser/src/parser.rs +++ b/brush-parser/src/parser.rs @@ -771,10 +771,16 @@ peg::parser! { linebreak() [Token::Word(e, _)] linebreak() { e } // N.B. An I/O number must be a string of only digits, and it must be - // followed by a '<' or '>' character (but not consume them). + // followed by a '<' or '>' character (but not consume them). We also + // need to make sure that there was no space between the number and the + // redirection operator; unfortunately we don't have the space anymore + // but we can infer it by looking at the tokens' locations. rule io_number() -> u32 = - [Token::Word(w, _) if w.chars().all(|c: char| c.is_ascii_digit())] - &([Token::Operator(o, _) if o.starts_with('<') || o.starts_with('>')]) { + [Token::Word(w, num_loc) if w.chars().all(|c: char| c.is_ascii_digit())] + &([Token::Operator(o, redir_loc) if + o.starts_with(['<', '>']) && + locations_are_contiguous(num_loc, redir_loc)]) { + w.parse().unwrap() } @@ -881,6 +887,13 @@ fn add_pipe_extension_redirection(c: &mut ast::Command) -> Result<(), &'static s Ok(()) } +fn locations_are_contiguous( + loc_left: &crate::TokenLocation, + loc_right: &crate::TokenLocation, +) -> bool { + loc_left.end.index == loc_right.start.index +} + fn parse_array_assignment( word: &str, elements: &[&String], diff --git a/brush-shell/tests/cases/redirection.yaml b/brush-shell/tests/cases/redirection.yaml index 15de2555..5fa2d97d 100644 --- a/brush-shell/tests/cases/redirection.yaml +++ b/brush-shell/tests/cases/redirection.yaml @@ -29,6 +29,20 @@ cases: stdin: | echo hi >&2 + - name: "Redirecting an fd" + stdin: | + echo hi-out 1>stdout.txt + echo hi-error 2>stderr.txt >&2 + + - name: "Almost redirecting but not actually redirecting" + stdin: | + # Checks to make sure the '1' isn't treated as a file descriptor + echo 1 >&2 + + - name: "Redirection with arbitrary fd number" + stdin: | + echo hi 4>file.txt >&4 + - name: "Process substitution: basic" stdin: | shopt -u -o posix @@ -76,3 +90,24 @@ cases: stdin: | shopt -u -o posix cp <(echo hi) >(cat) + + - name: "Redirection in command substitution" + stdin: | + echo $(echo hi >&2) 2>stderr.txt + + - name: "Redirection in command substitution to non-standard fd" + ignore_stderr: true + stdin: | + echo $(echo hi >&3) 3>output.txt + + - name: "Redirection in command substitution in braces to non-standard fd" + known_failure: true # https://github.com/reubeno/brush/issues/358 + ignore_stderr: true + stdin: | + { : $(echo hi >&3); } 3>output.txt + + - name: "Redirection in command substitution in subshell to non-standard fd" + known_failure: true # https://github.com/reubeno/brush/issues/358 + ignore_stderr: true + stdin: | + ( : $(echo hi >&3); ) 3>output.txt diff --git a/brush-shell/tests/compat_tests.rs b/brush-shell/tests/compat_tests.rs index 0d75fb5e..45217922 100644 --- a/brush-shell/tests/compat_tests.rs +++ b/brush-shell/tests/compat_tests.rs @@ -715,12 +715,38 @@ impl TestCaseResult { for entry in entries { const INDENT: &str = " "; match entry { - DirComparisonEntry::Different(left, right) => { + DirComparisonEntry::Different( + left_path, + left_contents, + right_path, + right_contents, + ) => { writeln!( writer, "{INDENT}oracle file {} differs from test file {}", - left.to_string_lossy(), - right.to_string_lossy() + left_path.to_string_lossy(), + right_path.to_string_lossy() + )?; + + writeln!( + writer, + "{INDENT}{}", + "------ Oracle <> Test: file ---------------------------------" + .cyan() + )?; + + write_diff( + &mut writer, + 8, + left_contents.as_str(), + right_contents.as_str(), + )?; + + writeln!( + writer, + " {}", + "---------------------------------------------------------------" + .cyan() )?; } DirComparisonEntry::LeftOnly(p) => { @@ -1227,7 +1253,7 @@ impl StringComparison { enum DirComparisonEntry { LeftOnly(PathBuf), RightOnly(PathBuf), - Different(PathBuf, PathBuf), + Different(PathBuf, String, PathBuf, String), } enum DirComparison { @@ -1265,7 +1291,9 @@ fn diff_dirs(oracle_path: &Path, test_path: &Path) -> Result { } dir_cmp::full::DirCmpEntry::Both(l, r, _) => DirComparisonEntry::Different( pathdiff::diff_paths(l, oracle_path).unwrap(), + std::fs::read_to_string(l).unwrap(), pathdiff::diff_paths(r, test_path).unwrap(), + std::fs::read_to_string(r).unwrap(), ), }) .collect();