From c81cf56c0b6355e9d5e50d74e56fc00e1039d40a Mon Sep 17 00:00:00 2001
From: Mahmoud Al-Qudsi <mqudsi@neosmart.net>
Date: Wed, 26 Jul 2017 15:11:44 -0500
Subject: [PATCH] Don't indiscriminately unblock previous cmd for internal
 builtin/functions

In the last commit, we introduced an indiscriminate if !EXTERNAL check
that unblocks a previously SIGSTOP'd command (if any) to allow the main
loop in exec_job to read from it without deadlocking (since builtins and
functions read directly from input as an optimization, sometimes).

Now only unblocking where a fork will not happen to ensure that if a
builtin ends up forking, that fork'd process is guaranteed to be able to
join the previous process' process group and access its output pipes.
---
 src/exec.cpp | 35 ++++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)

diff --git a/src/exec.cpp b/src/exec.cpp
index 1c811af85..ab13a5513 100644
--- a/src/exec.cpp
+++ b/src/exec.cpp
@@ -592,16 +592,6 @@ void exec_job(parser_t &parser, job_t *j) {
             }
             env_export_arr();
         }
-        else {
-            // Since the main loop itself is going to be (potentially) reading from the previous
-            // process, we need to unblock it here instead of after "executing" the next process
-            // in the chain.
-            if (blocked_pid != -1) {
-                //now that next command in the chain has been started, unblock the previous command
-                unblock_pid(blocked_pid);
-                blocked_pid = -1;
-            }
-        }
 
         // Set up fds that will be used in the pipe.
         if (pipes_to_next_command) {
@@ -642,12 +632,23 @@ void exec_job(parser_t &parser, job_t *j) {
         // This is the IO buffer we use for storing the output of a block or function when it is in
         // a pipeline.
         shared_ptr<io_buffer_t> block_output_io_buffer;
+        auto unblock_previous = [&] () {
+            if (blocked_pid != -1) {
+                //now that next command in the chain has been started, unblock the previous command
+                unblock_pid(blocked_pid);
+                blocked_pid = -1;
+            }
+        };
 
         // This is the io_streams we pass to internal builtins.
         std::unique_ptr<io_streams_t> builtin_io_streams(new io_streams_t(stdout_read_limit));
 
         switch (p->type) {
             case INTERNAL_FUNCTION: {
+                //internal function never forks
+                //unblock previous (if any) to prevent hang
+                unblock_previous();
+
                 // Calls to function_get_definition might need to source a file as a part of
                 // autoloading, hence there must be no blocks.
                 signal_unblock();
@@ -825,6 +826,9 @@ void exec_job(parser_t &parser, job_t *j) {
 
                     signal_unblock();
 
+                    //main loop is performing a blocking read from previous command's output
+                    //make sure read source is not blocked
+                    unblock_previous();
                     p->status = builtin_run(parser, p->get_argv(), *builtin_io_streams);
 
                     signal_block();
@@ -945,6 +949,10 @@ void exec_job(parser_t &parser, job_t *j) {
                 // output, so that we can truncate the file. Does not apply to /dev/null.
                 bool must_fork = redirection_is_to_real_file(stdout_io.get()) ||
                                  redirection_is_to_real_file(stderr_io.get());
+                if (!must_fork) {
+                    //we are handling reads directly in the main loop. Make sure source is unblocked.
+                    unblock_previous();
+                }
                 if (!must_fork && p->is_last_in_job) {
                     const bool stdout_is_to_buffer = stdout_io && stdout_io->io_mode == IO_BUFFER;
                     const bool no_stdout_output = stdout_buffer.empty();
@@ -1162,12 +1170,9 @@ void exec_job(parser_t &parser, job_t *j) {
             }
         }
 
-        if (blocked_pid != -1) {
-            //now that next command in the chain has been started, unblock the previous command
-            unblock_pid(blocked_pid);
-            blocked_pid = -1;
-        }
 
+        //if the command we ran before this one was SIGSTOP'd to let this one catch up, unblock it now.
+        unblock_previous();
         if (command_blocked) {
             //store the newly-blocked command's PID so that it can be SIGCONT'd once the next process
             //in the chain is started. That may be in this job or in another job.