Mark stdin as nonblocking if we get EWOULDBLOCK, and before handing i…

…t off to child processes when either starting them or moving them to the foreground.

#176

env_universal.cpp

@@ -135,7 +135,7 @@ static int try_get_socket_once(void)
         return -1;
     }
 
-    if ((fcntl(s, F_SETFL, O_NONBLOCK) != 0) || (fcntl(s, F_SETFD, FD_CLOEXEC) != 0))
+    if ((make_fd_nonblocking(s) != 0) || (fcntl(s, F_SETFD, FD_CLOEXEC) != 0))
     {
         wperror(L"fcntl");
         close(s);

exec.cpp

@@ -502,10 +502,10 @@ static void internal_exec_helper(parser_t &parser,
 }
 
 /* Returns whether we can use posix spawn for a given process in a given job.
- Per https://github.com/fish-shell/fish-shell/issues/364 , error handling for file redirections is too difficult with posix_spawn
- So in that case we use fork/exec
+ Per https://github.com/fish-shell/fish-shell/issues/364 , error handling for file redirections is too difficult with posix_spawn,
+ so in that case we use fork/exec.
 
- Furthermore, to avoid the race between the caller calling tcsetpgrp() and the client checking the foreground process group, we don't use posix_spawn if we're going to foreground the process. (If we use fork(), we can call tcsetpgrp after the fork, before the exec, and avoid the racse).
+ Furthermore, to avoid the race between the caller calling tcsetpgrp() and the client checking the foreground process group, we don't use posix_spawn if we're going to foreground the process. (If we use fork(), we can call tcsetpgrp after the fork, before the exec, and avoid the race).
 */
 static bool can_use_posix_spawn_for_job(const job_t *job, const process_t *process)
 {
@@ -1312,6 +1312,9 @@ void exec(parser_t &parser, job_t *j)
                 /* Get argv and envv before we fork */
                 null_terminated_array_t<char> argv_array;
                 convert_wide_array_to_narrow(p->get_argv_array(), &argv_array);
+
+                /* Ensure that stdin is blocking before we hand it off (see issue #176). It's a little strange that we only do this with stdin and not with stdout or stderr. However in practice, setting or clearing O_NONBLOCK on stdin also sets it for the other two fds, presumably because they refer to the same underlying file (/dev/tty?) */
+                make_fd_blocking(STDIN_FILENO);
 
                 const char * const *argv = argv_array.get();
                 const char * const *envv = env_export_arr(false);

fishd.cpp

@@ -578,7 +578,7 @@ static int get_socket(void)
         goto unlock;
     }
 
-    if (fcntl(s, F_SETFL, O_NONBLOCK) != 0)
+    if (make_fd_nonblocking(s) != 0)
     {
         wperror(L"fcntl");
         close(s);
@@ -988,7 +988,7 @@ int main(int argc, char ** argv)
             {
                 debug(4, L"Connected with new child on fd %d", child_socket);
 
-                if (fcntl(child_socket, F_SETFL, O_NONBLOCK) != 0)
+                if (make_fd_nonblocking(child_socket) != 0)
                 {
                     wperror(L"fcntl");
                     close(child_socket);

input_common.cpp

@@ -81,7 +81,7 @@ void input_common_destroy()
 }
 
 /**
-   Internal function used by input_common_readch to read one byte from fd 1. This function should only be called by
+   Internal function used by input_common_readch to read one byte from fd 0. This function should only be called by
    input_common_readch().
 */
 static wint_t readb()

io.cpp

@@ -138,9 +138,7 @@ io_buffer_t *io_buffer_t::create(bool is_input, int fd)
         wperror(L"pipe");
         success = false;
     }
-    else if (fcntl(buffer_redirect->pipe_fd[0],
-                   F_SETFL,
-                   O_NONBLOCK))
+    else if (make_fd_nonblocking(buffer_redirect->pipe_fd[0]) != 0)
     {
         debug(1, PIPE_ERROR);
         wperror(L"fcntl");

proc.cpp

@@ -362,9 +362,7 @@ int job_signal(job_t *j, int signal)
    Return 0 if all went well, nonzero otherwise.
    This is called from a signal handler.
 */
-static void mark_process_status(const job_t *j,
-                                process_t *p,
-                                int status)
+static void mark_process_status(const job_t *j, process_t *p, int status)
 {
 //	debug( 0, L"Process %ls %ls", p->argv[0], WIFSTOPPED (status)?L"stopped":(WIFEXITED( status )?L"exited":(WIFSIGNALED( status )?L"signaled to exit":L"BLARGH")) );
     p->status = status;
@@ -418,8 +416,8 @@ void job_mark_process_as_failed(const job_t *job, process_t *p)
 static void handle_child_status(pid_t pid, int status)
 {
     bool found_proc = false;
-    const job_t *j=0;
-    process_t *p=0;
+    const job_t *j = NULL;
+    process_t *p = NULL;
 //	char mess[MESS_SIZE];
     /*
       snprintf( mess,
@@ -965,7 +963,7 @@ static void read_try(job_t *j)
    a job that has previously been stopped. In that case, we need to
    set the terminal attributes to those saved in the job.
  */
-static int terminal_give_to_job(job_t *j, int cont)
+static bool terminal_give_to_job(job_t *j, int cont)
 {
 
     if (tcsetpgrp(0, j->pgid))
@@ -975,7 +973,7 @@ static int terminal_give_to_job(job_t *j, int cont)
               j->job_id,
               j->command_wcstr());
         wperror(L"tcsetpgrp");
-        return 0;
+        return false;
     }
 
     if (cont)
@@ -987,10 +985,10 @@ static int terminal_give_to_job(job_t *j, int cont)
                   j->job_id,
                   j->command_wcstr());
             wperror(L"tcsetattr");
-            return 0;
+            return false;
         }
     }
-    return 1;
+    return true;
 }
 
 /**
@@ -1059,18 +1057,17 @@ void job_continue(job_t *j, bool cont)
     {
         if (job_get_flag(j, JOB_TERMINAL) && job_get_flag(j, JOB_FOREGROUND))
         {
-            /* Put the job into the foreground.  */
-            int ok;
-
+            /* Put the job into the foreground. Hack: ensure that stdin is marked as blocking first (#176). */
+            make_fd_blocking(STDIN_FILENO);
+            
             signal_block();
 
-            ok = terminal_give_to_job(j, cont);
+            bool ok = terminal_give_to_job(j, cont);
 
             signal_unblock();
 
             if (!ok)
                 return;
-
         }
 
         /*
@@ -1186,7 +1183,6 @@ void job_continue(job_t *j, bool cont)
             // were sometimes having their output combined with the set_color calls in the wrong order!
             read_try(j);
 
-
             process_t *p = j->first_process;
             while (p->next)
                 p = p->next;
@@ -1205,9 +1201,8 @@ void job_continue(job_t *j, bool cont)
                 }
             }
         }
-        /*
-           Put the shell back in the foreground.
-        */
+
+        /* Put the shell back in the foreground. */
         if (job_get_flag(j, JOB_TERMINAL) && job_get_flag(j, JOB_FOREGROUND))
         {
             int ok;

reader.cpp

@@ -2328,12 +2328,12 @@ void set_env_cmd_duration(struct timeval *after, struct timeval *before)
     }
 }
 
-void reader_run_command(parser_t &parser, const wchar_t *cmd)
+void reader_run_command(parser_t &parser, const wcstring &cmd)
 {
 
     struct timeval time_before, time_after;
 
-    wcstring ft = tok_first(cmd);
+    wcstring ft = tok_first(cmd.c_str());
 
     if (! ft.empty())
         env_set(L"_", ft.c_str(), ENV_GLOBAL);
@@ -2709,7 +2709,7 @@ static void handle_end_loop()
    Read interactively. Read input from stdin while providing editing
    facilities.
 */
-static int read_i()
+static int read_i(void)
 {
     reader_push(L"fish");
     reader_set_complete_function(&complete);
@@ -2724,8 +2724,6 @@ static int read_i()
 
     while ((!data->end_loop) && (!sanity_check()))
     {
-        const wchar_t *tmp;
-
         event_fire_generic(L"fish_prompt");
         if (function_exists(LEFT_PROMPT_FUNCTION_NAME))
             reader_set_left_prompt(LEFT_PROMPT_FUNCTION_NAME);
@@ -2744,23 +2742,19 @@ static int read_i()
           during evaluation.
         */
 
-
-        tmp = reader_readline();
-
+        const wchar_t *tmp = reader_readline();
 
         if (data->end_loop)
         {
             handle_end_loop();
         }
         else if (tmp)
         {
-            tmp = wcsdup(tmp);
-
+            wcstring command = tmp;
             data->buff_pos=0;
             data->command_line.clear();
             data->command_line_changed();
-            reader_run_command(parser, tmp);
-            free((void *)tmp);
+            reader_run_command(parser, command);
             if (data->end_loop)
             {
                 handle_end_loop();
@@ -2837,7 +2831,7 @@ static wchar_t unescaped_quote(const wcstring &str, size_t pos)
 }
 
 
-const wchar_t *reader_readline()
+const wchar_t *reader_readline(void)
 {
     wint_t c;
     int last_char=0;
@@ -3612,7 +3606,7 @@ static int read_ni(int fd, const io_chain_t &io)
     wchar_t *buff=0;
     std::vector<char> acc;
 
-    int des = fd == 0 ? dup(0) : fd;
+    int des = (fd == STDIN_FILENO ? dup(STDIN_FILENO) : fd);
     int res=0;
 
     if (des == -1)
@@ -3629,14 +3623,23 @@ static int read_ni(int fd, const io_chain_t &io)
             char buff[4096];
             size_t c = fread(buff, 1, 4096, in_stream);
 
-            if (ferror(in_stream) && (errno != EINTR))
+            if (ferror(in_stream))
             {
-                debug(1,
-                      _(L"Error while reading from file descriptor"));
+                if (errno == EINTR)
+                {
+                    /* We got a signal, just keep going */
+                    continue;
+                }
+                else if ((errno == EAGAIN || errno == EWOULDBLOCK) && make_fd_blocking(des) == 0)
+                {
+                    /* We succeeded in making the fd blocking, try again */
+                    continue;
+                }
+
+                /* Fatal error */
+                debug(1, _(L"Error while reading from file descriptor"));
 
-                /*
-                  Reset buffer on error. We won't evaluate incomplete files.
-                */
+                /* Reset buffer on error. We won't evaluate incomplete files. */
                 acc.clear();
                 break;
 

reader.h

@@ -85,7 +85,7 @@ void reader_repaint_if_needed();
    Run the specified command with the correct terminal modes, and
    while taking care to perform job notification, set the title, etc.
 */
-void reader_run_command(const wchar_t *buff);
+void reader_run_command(const wcstring &buff);
 
 /**
    Get the string of character currently entered into the command

wutil.cpp

@@ -264,12 +264,10 @@ int wstat(const wcstring &file_name, struct stat *buf)
 
 int lwstat(const wcstring &file_name, struct stat *buf)
 {
-    // fprintf(stderr, "%s\n", __PRETTY_FUNCTION__);
     cstring tmp = wcs2string(file_name);
     return lstat(tmp.c_str(), buf);
 }
 
-
 int waccess(const wcstring &file_name, int mode)
 {
     cstring tmp = wcs2string(file_name);
@@ -292,6 +290,28 @@ void wperror(const wcstring &s)
     fwprintf(stderr, L"%s\n", strerror(e));
 }
 
+int make_fd_nonblocking(int fd)
+{
+    int flags = fcntl(fd, F_GETFL, 0);
+    int err = 0;
+    if (! (flags & O_NONBLOCK))
+    {
+        err = fcntl(fd, F_SETFL, flags | O_NONBLOCK);
+    }
+    return err == -1 ? errno : 0;
+}
+
+int make_fd_blocking(int fd)
+{
+    int flags = fcntl(fd, F_GETFL, 0);
+    int err = 0;
+    if (flags & O_NONBLOCK)
+    {
+        err = fcntl(fd, F_SETFL, flags & ~O_NONBLOCK);
+    }
+    return err == -1 ? errno : 0;
+}
+
 static inline void safe_append(char *buffer, const char *s, size_t buffsize)
 {
     strncat(buffer, s, buffsize - strlen(buffer) - 1);

wutil.h

@@ -48,9 +48,13 @@ int wopen(const wcstring &pathname, int flags, mode_t mode = 0);
 /** Wide character version of open() that also sets the close-on-exec flag (atomically when possible). */
 int wopen_cloexec(const wcstring &pathname, int flags, mode_t mode = 0);
 
-/**
-   Wide character version of creat().
-*/
+/** Mark an fd as nonblocking; returns errno or 0 on success */
+int make_fd_nonblocking(int fd);
+
+/** Mark an fd as blocking; returns errno or 0 on success */
+int make_fd_blocking(int fd);
+
+/** Wide character version of creat(). */
 int wcreat(const wcstring &pathname, mode_t mode);