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

@@ -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

src/builtin.cpp

@@ -236,11 +236,17 @@ static maybe_t<int> 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++;

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);

src/builtin_read.cpp

@@ -461,6 +461,12 @@ maybe_t<int> 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;

src/builtin_source.cpp

@@ -47,6 +47,10 @@ maybe_t<int> 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.

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) {

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) {

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.

tests/checks/redirect.fish

@@ -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