From df8b9b70954fd38c170b954967112d50ba299223 Mon Sep 17 00:00:00 2001 From: Fabian Boehm Date: Wed, 20 Mar 2024 16:44:59 +0100 Subject: [PATCH 1/2] Add `/dev/null | string match foo ``` This both makes it nicer and shorter, *and* helps with TOCTTOU - what if the file is removed/changed after the check? The reason for reading /dev/null instead of a closed fd is that a closed fd will often cause an error. In case opening /dev/null fails, it still skips the command. That's really a last resort for when the operating system has turned out to be a platypus and not a unix. Fixes #4865 --- doc_src/language.rst | 6 +++ src/highlight.rs | 2 +- src/io.rs | 90 ++++++++++++++++++++++---------------- src/redirection.rs | 3 +- src/tokenizer.rs | 3 ++ tests/checks/redirect.fish | 22 ++++++++++ 6 files changed, 86 insertions(+), 40 deletions(-) diff --git a/doc_src/language.rst b/doc_src/language.rst index 8fa9c1676069..a48da7ad124a 100644 --- a/doc_src/language.rst +++ b/doc_src/language.rst @@ -168,6 +168,7 @@ Each stream has a number called the file descriptor (FD): 0 for stdin, 1 for std The destination of a stream can be changed using something called *redirection*. For example, ``echo hello > output.txt``, redirects the standard output of the ``echo`` command to a text file. - To read standard input from a file, use ``DESTINATION``. - To write standard error to a file, use ``2>DESTINATION``. [#]_ - To append standard output to a file, use ``>>DESTINATION_FILE``. @@ -188,6 +189,8 @@ Any arbitrary file descriptor can be used in a redirection by prefixing the redi - To redirect the output of descriptor N, use ``N>DESTINATION``. - To append the output of descriptor N to a file, use ``N>>DESTINATION_FILE``. +File descriptors cannot be used with a ``&2 # <- this goes to stderr! end >/dev/null # ignore stdout, so this prints "stderr" + # print all lines that include "foo" from myfile, or nothing if it doesn't exist. + string match '*foo*' `. diff --git a/src/highlight.rs b/src/highlight.rs index 87e887881c75..0f72d3c2fff7 100644 --- a/src/highlight.rs +++ b/src/highlight.rs @@ -1235,7 +1235,7 @@ impl<'s> Highlighter<'s> { }; } } - RedirectionMode::input => { + RedirectionMode::input | RedirectionMode::try_input => { // Input redirections must have a readable non-directory. target_is_valid = waccess(&target_path, R_OK) == 0 && match wstat(&target_path) { diff --git a/src/io.rs b/src/io.rs index c57c53e03ebc..3248a63f1825 100644 --- a/src/io.rs +++ b/src/io.rs @@ -646,6 +646,35 @@ impl IoChain { #[allow(clippy::collapsible_else_if)] pub fn append_from_specs(&mut self, specs: &RedirectionSpecList, pwd: &wstr) -> bool { let mut have_error = false; + + let print_error = |err, target: &wstr| { + // If the error is that the file doesn't exist + // or there's a non-directory component, + // find the first problematic component for a better message. + if [ENOENT, ENOTDIR].contains(&err) { + FLOGF!(warning, FILE_ERROR, target); + let mut dname: &wstr = target; + while !dname.is_empty() { + let next: &wstr = wdirname(dname); + if let Ok(md) = wstat(next) { + if !md.is_dir() { + FLOGF!(warning, "Path '%ls' is not a directory", next); + } else { + FLOGF!(warning, "Path '%ls' does not exist", dname); + } + break; + } + dname = next; + } + } else if err != EINTR { + // If we get EINTR we had a cancel signal. + // That's expected (ctrl-c on the commandline), + // so no warning. + FLOGF!(warning, FILE_ERROR, target); + perror("open"); + } + }; + for spec in specs { match spec.mode { RedirectionMode::fd => { @@ -671,50 +700,35 @@ impl IoChain { Err(err) => { if oflags.intersects(OFlag::O_EXCL) && err == nix::Error::EEXIST { FLOGF!(warning, NOCLOB_ERROR, spec.target); - } else { + } else if spec.mode != RedirectionMode::try_input { if should_flog!(warning) { - let err = errno::errno().0; - // If the error is that the file doesn't exist - // or there's a non-directory component, - // find the first problematic component for a better message. - if [ENOENT, ENOTDIR].contains(&err) { - FLOGF!(warning, FILE_ERROR, spec.target); - let mut dname: &wstr = &spec.target; - while !dname.is_empty() { - let next: &wstr = wdirname(dname); - if let Ok(md) = wstat(next) { - if !md.is_dir() { - FLOGF!( - warning, - "Path '%ls' is not a directory", - next - ); - } else { - FLOGF!( - warning, - "Path '%ls' does not exist", - dname - ); - } - break; - } - dname = next; - } - } else if err != EINTR { - // If we get EINTR we had a cancel signal. - // That's expected (ctrl-c on the commandline), - // so no warning. - FLOGF!(warning, FILE_ERROR, spec.target); - perror("open"); - } + print_error(errno::errno().0, &spec.target); } } // If opening a file fails, insert a closed FD instead of the file redirection // and return false. This lets execution potentially recover and at least gives // the shell a chance to gracefully regain control of the shell (see #7038). - self.push(Arc::new(IoClose::new(spec.fd))); - have_error = true; - continue; + if spec.mode != RedirectionMode::try_input { + self.push(Arc::new(IoClose::new(spec.fd))); + have_error = true; + continue; + } else { + // If we're told to try via ` { + self.push(Arc::new(IoFile::new(spec.fd, fd))); + } + _ => { + // /dev/null can't be opened??? + if should_flog!(warning) { + print_error(errno::errno().0, L!("/dev/null")); + } + self.push(Arc::new(IoClose::new(spec.fd))); + have_error = true; + continue; + } + } + } } } } diff --git a/src/redirection.rs b/src/redirection.rs index 6ec0e50b8ce6..035fb92b8014 100644 --- a/src/redirection.rs +++ b/src/redirection.rs @@ -11,6 +11,7 @@ pub enum RedirectionMode { overwrite, // normal redirection: > file.txt append, // appending redirection: >> file.txt input, // input redirection: < file.txt + try_input, // try-input redirection: &1 noclob, // noclobber redirection: >? file.txt } @@ -38,7 +39,7 @@ impl RedirectionMode { RedirectionMode::append => Some(OFlag::O_CREAT | OFlag::O_APPEND | OFlag::O_WRONLY), RedirectionMode::overwrite => Some(OFlag::O_CREAT | OFlag::O_WRONLY | OFlag::O_TRUNC), RedirectionMode::noclob => Some(OFlag::O_CREAT | OFlag::O_EXCL | OFlag::O_WRONLY), - RedirectionMode::input => Some(OFlag::O_RDONLY), + RedirectionMode::input | RedirectionMode::try_input => Some(OFlag::O_RDONLY), _ => None, } } diff --git a/src/tokenizer.rs b/src/tokenizer.rs index 074a992f61cf..2960ada92a83 100644 --- a/src/tokenizer.rs +++ b/src/tokenizer.rs @@ -990,6 +990,9 @@ impl TryFrom<&wstr> for PipeOrRedir { consume(&mut cursor, '<'); if try_consume(&mut cursor, '&') { result.mode = RedirectionMode::fd; + } else if try_consume(&mut cursor, '?') { + // /bin/echo/file #CHECKERR: warning: An error occurred while redirecting file '/bin/echo/file' #CHECKERR: warning: Path '/bin/echo' is not a directory + +echo foo Date: Wed, 20 Mar 2024 17:40:27 +0100 Subject: [PATCH 2/2] share/: Use `/dev/null)`. In doing so it can avoid a weird race condition where the file changes between the check and the use, and it can avoid an external process or having to open the file twice. However it can also cause more work to happen - if you do ```fish if test -r file # do a lot of work with file end ``` it might be prudent to keep doing that. In the cases here it's usually just a few `string` calls, which would operate on nothing and so be pretty quick. --- share/functions/__fish_complete_groups.fish | 2 +- share/functions/__fish_complete_user_ids.fish | 4 +-- share/functions/__fish_complete_users.fish | 4 +-- share/functions/__fish_print_hostnames.fish | 22 +++++------- share/functions/__fish_set_locale.fish | 36 +++++++++---------- share/functions/fish_command_not_found.fish | 5 +-- share/functions/fish_git_prompt.fish | 12 +++---- share/functions/help.fish | 3 +- 8 files changed, 37 insertions(+), 51 deletions(-) diff --git a/share/functions/__fish_complete_groups.fish b/share/functions/__fish_complete_groups.fish index 5acc21290c72..2a3958cb6dd5 100644 --- a/share/functions/__fish_complete_groups.fish +++ b/share/functions/__fish_complete_groups.fish @@ -6,6 +6,6 @@ function __fish_complete_groups --description "Print a list of local groups, wit else while read -l line string split -f 1,4 : -- $line | string join \t - end &2 diff --git a/share/functions/fish_git_prompt.fish b/share/functions/fish_git_prompt.fish index c8b55547fe25..f92d9761a339 100644 --- a/share/functions/fish_git_prompt.fish +++ b/share/functions/fish_git_prompt.fish @@ -478,9 +478,9 @@ function __fish_git_prompt_operation_branch_bare --description "fish_git_prompt set -l total if test -d $git_dir/rebase-merge - set branch (cat $git_dir/rebase-merge/head-name 2>/dev/null) - set step (cat $git_dir/rebase-merge/msgnum 2>/dev/null) - set total (cat $git_dir/rebase-merge/end 2>/dev/null) + read branch /dev/null) - set total (cat $git_dir/rebase-apply/last 2>/dev/null) + read step /dev/null) + read branch