From 84d59accfc322e112f7a1fde2256cda2e8d80ead Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Wed, 10 Feb 2021 17:19:08 -0800 Subject: [PATCH] builtins to allow stdin to be closed Prior to this fix, if stdin were explicitly closed, then builtins would silently fail. For example: count <&- would just fail with status 1. Remove this limitation and allow each builtin to handle a closed stdin how it sees fit. --- CHANGELOG.rst | 1 + src/builtin.cpp | 10 ++++++++-- src/builtin_math.cpp | 7 ++++++- src/builtin_read.cpp | 6 ++++++ src/builtin_source.cpp | 4 ++++ src/builtin_string.cpp | 1 + src/exec.cpp | 2 -- src/io.h | 6 ++++-- tests/checks/redirect.fish | 15 ++++++++++++++- 9 files changed, 44 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index e7f33da03..0040d872c 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -184,6 +184,7 @@ Scripting improvements - The ``pwd`` command supports the long options ``--logical`` and ``--physical``, matching other implementations (:issue:`6787`). - ``fish --profile`` now only starts the profile after fish's startup (including config.fish) is done. For profiling startup there is a new ``--profile-startup`` option that profiles only startup (:issue:`7648`). - ``set --query``'s $status will now saturate at 255 instead of wrapping around when checking more than 255 variables at once (:issue:`7698`). +- It is no longer an error to run builtin with closed stdin. For example ``count <&-`` now prints 0, instead of failing. Interactive improvements diff --git a/src/builtin.cpp b/src/builtin.cpp index 994af2007..bedd5c419 100644 --- a/src/builtin.cpp +++ b/src/builtin.cpp @@ -236,11 +236,17 @@ static maybe_t builtin_count(parser_t &parser, io_streams_t &streams, wchar // Count the newlines coming in via stdin like `wc -l`. if (streams.stdin_is_directly_redirected) { + assert(streams.stdin_fd >= 0 && + "Should have a valid fd since stdin is directly redirected"); char buf[COUNT_CHUNK_SIZE]; while (true) { long n = read_blocked(streams.stdin_fd, buf, COUNT_CHUNK_SIZE); - // Ignore all errors for now. - if (n <= 0) break; + if (n == 0) { + break; + } else if (n < 0) { + wperror(L"read"); + return STATUS_CMD_ERROR; + } for (int i = 0; i < n; i++) { if (buf[i] == L'\n') { argc++; diff --git a/src/builtin_math.cpp b/src/builtin_math.cpp index 2d3bc7933..9c8f4c8ae 100644 --- a/src/builtin_math.cpp +++ b/src/builtin_math.cpp @@ -120,7 +120,10 @@ static const wchar_t *math_get_arg_stdin(wcstring *storage, const io_streams_t & char ch = '\0'; long rc = read_blocked(streams.stdin_fd, &ch, 1); - if (rc < 0) return nullptr; // failure + if (rc < 0) { // error + wperror(L"read"); + return nullptr; + } if (rc == 0) { // EOF if (arg.empty()) return nullptr; @@ -146,6 +149,8 @@ static const wchar_t *math_get_arg_argv(int *argidx, wchar_t **argv) { static const wchar_t *math_get_arg(int *argidx, wchar_t **argv, wcstring *storage, const io_streams_t &streams) { if (math_args_from_stdin(streams)) { + assert(streams.stdin_fd >= 0 && + "stdin should not be closed since it is directly redirected"); return math_get_arg_stdin(storage, streams); } return math_get_arg_argv(argidx, argv); diff --git a/src/builtin_read.cpp b/src/builtin_read.cpp index fc4057432..6b9707375 100644 --- a/src/builtin_read.cpp +++ b/src/builtin_read.cpp @@ -461,6 +461,12 @@ maybe_t builtin_read(parser_t &parser, io_streams_t &streams, wchar_t **arg retval = validate_read_args(cmd, opts, argc, argv, parser, streams); if (retval != STATUS_CMD_OK) return retval; + // stdin may have been explicitly closed + if (streams.stdin_fd < 0) { + streams.err.append_format(_(L"%ls: stdin is closed\n"), cmd); + return STATUS_CMD_ERROR; + } + if (opts.one_line) { // --line is the same as read -d \n repeated N times opts.have_delimiter = true; diff --git a/src/builtin_source.cpp b/src/builtin_source.cpp index be3ada8e7..8a3430dfc 100644 --- a/src/builtin_source.cpp +++ b/src/builtin_source.cpp @@ -47,6 +47,10 @@ maybe_t builtin_source(parser_t &parser, io_streams_t &streams, wchar_t **a const wchar_t *fn, *fn_intern; if (argc == optind || std::wcscmp(argv[optind], L"-") == 0) { + if (streams.stdin_fd < 0) { + streams.err.append_format(_(L"%ls: stdin is closed\n"), cmd); + return STATUS_CMD_ERROR; + } // Either a bare `source` which means to implicitly read from stdin or an explicit `-`. if (argc == optind && isatty(streams.stdin_fd)) { // Don't implicitly read from the terminal. diff --git a/src/builtin_string.cpp b/src/builtin_string.cpp index d0d428bd9..2e5aed796 100644 --- a/src/builtin_string.cpp +++ b/src/builtin_string.cpp @@ -83,6 +83,7 @@ class arg_iterator_t { /// not. On true, the string is stored in storage_. bool get_arg_stdin() { assert(string_args_from_stdin(streams_) && "should not be reading from stdin"); + assert(streams_.stdin_fd >= 0 && "should have a valid fd"); // Read in chunks from fd until buffer has a line (or the end if split_ is unset). size_t pos; while (!split_ || (pos = buffer_.find('\n')) == std::string::npos) { diff --git a/src/exec.cpp b/src/exec.cpp index 7132308d2..98cf562c9 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -396,8 +396,6 @@ static launch_result_t exec_internal_builtin_proc(parser_t &parser, process_t *p } } - if (local_builtin_stdin == -1) return launch_result_t::failed; - // Determine if we have a "direct" redirection for stdin. bool stdin_is_directly_redirected = false; if (!p->is_first_in_job) { diff --git a/src/io.h b/src/io.h index e29eec16d..6aa5fc613 100644 --- a/src/io.h +++ b/src/io.h @@ -466,11 +466,13 @@ struct io_streams_t { output_stream_t &err; // fd representing stdin. This is not closed by the destructor. + // Note: if stdin is explicitly closed by `<&-` then this is -1! int stdin_fd{-1}; // Whether stdin is "directly redirected," meaning it is the recipient of a pipe (foo | cmd) or - // direct redirection (cmd < foo.txt). An "indirect redirection" would be e.g. begin ; cmd ; end - // < foo.txt + // direct redirection (cmd < foo.txt). An "indirect redirection" would be e.g. + // begin ; cmd ; end < foo.txt + // If stdin is closed (cmd <&-) this is false. bool stdin_is_directly_redirected{false}; // Indicates whether stdout and stderr are specifically piped. diff --git a/tests/checks/redirect.fish b/tests/checks/redirect.fish index 1cae2a77f..50222c264 100644 --- a/tests/checks/redirect.fish +++ b/tests/checks/redirect.fish @@ -50,7 +50,7 @@ test -f $tmpdir/file.txt && echo "File exists" || echo "File does not exist" function foo if set -q argv[1] - foo > $argv[1] + foo >$argv[1] end echo foo end @@ -89,6 +89,19 @@ begin end 2>| cat >/dev/null #CHECK: is_stdout +# Verify builtin behavior with closed stdin. +# count silently ignores closed stdin, others may print an error. +true <&- +echo $status +#CHECK: 0 +test -t 0 <&- +echo $status +#CHECK: 1 +read abc <&- +#CHECKERR: read: stdin is closed + + + # "Verify that pipes don't conflict with fd redirections" # This code is very similar to eval. We go over a bunch of fads # to make it likely that we will nominally conflict with a pipe