Release 3.1.2

Commit 5fccfd83ec, with the fix for #6806,
switched eval to buffer its output (like other builtins do). But this
prevents using eval with commands that wants to see the tty, especially
fzf. So only buffer the output if the output is piped to the next process.

CHANGELOG.md

@@ -1,3 +1,13 @@
+# fish 3.1.2 (released April 29, 2020)
+
+This release of fish fixes a major issue discovered in fish 3.1.1:
+
+- Commands such as `fzf` and `enhancd`, when used with `eval`, would hang. `eval` buffered output too aggressively, which has been fixed (#6955).
+
+If you are upgrading from version 3.0.0 or before, please also review the release notes for 3.1.1, 3.1.0 and 3.1b1 (included below).
+
+---
+
 # fish 3.1.1 (released April 27, 2020)
 
 This release of fish fixes a number of major issues discovered in fish 3.1.0.

src/builtin_eval.cpp

@@ -27,24 +27,40 @@ int builtin_eval(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
         new_cmd += argv[i];
     }
 
-    // The output and errput of eval must go to our streams, not to the io_chain in our streams,
-    // because they may contain a pipe which is intended to be consumed by a process which is not
-    // yet launched (#6806). Make bufferfills to capture it.
-    // TODO: we are sloppy and do not honor other redirections; eval'd code won't see for example a
-    // redirection of fd 3.
-    shared_ptr<io_bufferfill_t> stdout_fill =
-        io_bufferfill_t::create(fd_set_t{}, parser.libdata().read_limit, STDOUT_FILENO);
-    shared_ptr<io_bufferfill_t> stderr_fill =
-        io_bufferfill_t::create(fd_set_t{}, parser.libdata().read_limit, STDERR_FILENO);
-    if (!stdout_fill || !stderr_fill) {
-        // This can happen if we are unable to create a pipe.
-        return STATUS_CMD_ERROR;
+    // If stdout is piped, then its output must go to the streams, not to the io_chain in our
+    // streams, because the pipe may be intended to be consumed by a process which
+    // is not yet launched (#6806). If stdout is NOT redirected, it must see the tty (#6955). So
+    // create a bufferfill for stdout if and only if stdout is piped.
+    // Note do not do this if stdout is merely redirected (say, to a file); we don't want to
+    // buffer in that case.
+    shared_ptr<io_bufferfill_t> stdout_fill{};
+    if (streams.out_is_piped) {
+        stdout_fill =
+            io_bufferfill_t::create(fd_set_t{}, parser.libdata().read_limit, STDOUT_FILENO);
+        if (!stdout_fill) {
+            // We were unable to create a pipe, probably fd exhaustion.
+            return STATUS_CMD_ERROR;
+        }
     }
 
+    // Of course the same applies to stderr.
+    shared_ptr<io_bufferfill_t> stderr_fill{};
+    if (streams.err_is_piped) {
+        stderr_fill =
+            io_bufferfill_t::create(fd_set_t{}, parser.libdata().read_limit, STDERR_FILENO);
+        if (!stderr_fill) {
+            return STATUS_CMD_ERROR;
+        }
+    }
+
+    // Construct the full io chain, perhaps with our bufferfills appended.
+    io_chain_t ios = *streams.io_chain;
+    if (stdout_fill) ios.push_back(stdout_fill);
+    if (stderr_fill) ios.push_back(stderr_fill);
+
     const auto cached_exec_count = parser.libdata().exec_count;
     int status = STATUS_CMD_OK;
-    if (parser.eval(new_cmd, io_chain_t{stdout_fill, stderr_fill}, block_type_t::top,
-                    streams.parent_pgid) != eval_result_t::ok) {
+    if (parser.eval(new_cmd, ios, block_type_t::top, streams.parent_pgid) != eval_result_t::ok) {
         status = STATUS_CMD_ERROR;
     } else if (cached_exec_count == parser.libdata().exec_count) {
         // Issue #5692, in particular, to catch `eval ""`, `eval "begin; end;"`, etc.
@@ -55,14 +71,17 @@ int builtin_eval(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
     }
 
     // Finish the bufferfills - exhaust and close our pipes.
+    // Copy the output from the bufferfill back to the streams.
     // Note it is important that we hold no other references to the bufferfills here - they need to
     // deallocate to close.
-    std::shared_ptr<io_buffer_t> output = io_bufferfill_t::finish(std::move(stdout_fill));
-    std::shared_ptr<io_buffer_t> errput = io_bufferfill_t::finish(std::move(stderr_fill));
-
-    // Copy the output from the bufferfill back to the streams.
-    streams.out.append_narrow_buffer(output->buffer());
-    streams.err.append_narrow_buffer(errput->buffer());
-
+    ios.clear();
+    if (stdout_fill) {
+        std::shared_ptr<io_buffer_t> output = io_bufferfill_t::finish(std::move(stdout_fill));
+        streams.out.append_narrow_buffer(output->buffer());
+    }
+    if (stderr_fill) {
+        std::shared_ptr<io_buffer_t> errput = io_bufferfill_t::finish(std::move(stderr_fill));
+        streams.err.append_narrow_buffer(errput->buffer());
+    }
     return status;
 }

src/exec.cpp

@@ -428,9 +428,16 @@ static bool exec_internal_builtin_proc(parser_t &parser, const std::shared_ptr<j
         }
     }
 
+    // Pull out the IOs for stdout and stderr.
+    auto out_io = proc_io_chain.io_for_fd(STDOUT_FILENO);
+    auto err_io = proc_io_chain.io_for_fd(STDERR_FILENO);
+
+    // Set up our streams.
     streams.stdin_fd = local_builtin_stdin;
-    streams.out_is_redirected = proc_io_chain.io_for_fd(STDOUT_FILENO) != nullptr;
-    streams.err_is_redirected = proc_io_chain.io_for_fd(STDERR_FILENO) != nullptr;
+    streams.out_is_redirected = out_io != nullptr;
+    streams.err_is_redirected = err_io != nullptr;
+    streams.out_is_piped = (out_io != nullptr && out_io->io_mode == io_mode_t::pipe);
+    streams.err_is_piped = (err_io != nullptr && err_io->io_mode == io_mode_t::pipe);
     streams.stdin_is_directly_redirected = stdin_is_directly_redirected;
     streams.io_chain = &proc_io_chain;
 

src/io.h

@@ -451,7 +451,12 @@ struct io_streams_t {
     // < foo.txt
     bool stdin_is_directly_redirected{false};
 
-    // Indicates whether stdout and stderr are redirected (e.g. to a file or piped).
+    // Indicates whether stdout and stderr are specifically piped.
+    // If this is set, then the is_redirected flags must also be set.
+    bool out_is_piped{false};
+    bool err_is_piped{false};
+
+    // Indicates whether stdout and stderr are at all redirected (e.g. to a file or piped).
     bool out_is_redirected{false};
     bool err_is_redirected{false};