Don't use posix_spawn when file redirections are involved (except /dev/null) because the error handling is too difficult

Fix exec to correctly handle the case where a pid could not be created due to posix_spawn failing
Should fix fish-shell#364

Author: ridiculousfish

exec.cpp

@@ -504,11 +504,36 @@ static void do_builtin_io( const char *out, const char *err )
 	}
 }
 
+/* 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.
+
+*/
+static bool can_use_posix_spawn_for_job(const job_t *job, const process_t *process)
+{
+    bool result = true;
+    for (size_t idx = 0; idx < job->io.size(); idx++)
+    {
+        const io_data_t *io = job->io.at(idx);
+        if (io->io_mode == IO_FILE)
+        {
+            const char *path = io->filename_cstr;
+            /* This IO action is a file redirection. Only allow /dev/null, which is a common case we assume won't fail. */
+            if (strcmp(path, "/dev/null") != 0)
+            {
+                result = false;
+                break;
+            }
+        }
+    }
+    return result;
+}
+
 
 void exec( parser_t &parser, job_t *j )
 {
 	process_t *p;
-	pid_t pid;
+	pid_t pid = 0;
 	int mypipe[2];
 	sigset_t chldset; 
 
@@ -517,10 +542,10 @@ void exec( parser_t &parser, job_t *j )
 	io_data_t *io_buffer =0;
 
 	/*
-	  Set to 1 if something goes wrong while exec:ing the job, in
+	  Set to true if something goes wrong while exec:ing the job, in
 	  which case the cleanup code will kick in.
 	*/
-	int exec_error=0;
+	bool exec_error = false;
 
 	bool needs_keepalive = false;
 	process_t keepalive;
@@ -708,7 +733,7 @@ void exec( parser_t &parser, job_t *j )
 			{
 				debug( 1, PIPE_ERROR );
 				wperror (L"pipe");
-				exec_error=1;
+				exec_error = true;
 				break;
 			}
 
@@ -897,7 +922,7 @@ void exec( parser_t &parser, job_t *j )
 
 				if( builtin_stdin == -1 )
 				{
-					exec_error=1;
+					exec_error = true;
 					break;
 				}
 				else
@@ -1232,7 +1257,7 @@ void exec( parser_t &parser, job_t *j )
 
 #if FISH_USE_POSIX_SPAWN
                 /* Prefer to use posix_spawn, since it's faster on some systems like OS X */
-                bool use_posix_spawn = g_use_posix_spawn;
+                bool use_posix_spawn = g_use_posix_spawn && can_use_posix_spawn_for_job(j, p);
                 if (use_posix_spawn)
                 {
                     /* Create posix spawn attributes and actions */
@@ -1254,16 +1279,21 @@ void exec( parser_t &parser, job_t *j )
                         posix_spawn_file_actions_destroy(&actions);
                         posix_spawnattr_destroy(&attr);
                     }
+                    
+                    /* A 0 pid means we failed to posix_spawn. Since we have no pid, we'll never get told when it's exited, so we have to mark the process as failed. */
+                    if (pid == 0)
+                    {
+                        job_mark_process_as_failed(j, p);
+                        exec_error = true;
+                    }
                 }
                 else
 #endif
                 {
                     pid = execute_fork(false);
                     if (pid == 0)
                     {
-                        /*
-                          This is the child process. 
-                        */
+                        /* This is the child process. */
                         p->pid = getpid();
                         setup_child_process( j, p );
                         safe_launch_process( p, actual_cmd, argv, envv );

postfork.cpp

@@ -566,6 +566,7 @@ void safe_report_exec_error(int err, const char *actual_cmd, char **argv, char *
 
 		case ENOENT:
 		{
+            /* ENOENT is returned by exec() when the path fails, but also returned by posix_spawn if an open file action fails. These cases appear to be impossible to distinguish. We address this by not using posix_spawn for file redirections, so all the ENOENTs we find must be errors from exec(). */
             char interpreter_buff[128] = {}, *interpreter;
             interpreter = get_interpreter(actual_cmd, interpreter_buff, sizeof interpreter_buff);
 			if( interpreter && 0 != access( interpreter, X_OK ) )
@@ -574,7 +575,7 @@ void safe_report_exec_error(int err, const char *actual_cmd, char **argv, char *
 			}
 			else
 			{
-				debug_safe(0, "The file '%s' or a script or ELF interpreter does not exist, or a shared library needed for file or interpreter cannot be found.", actual_cmd);
+				debug_safe(0, "The file '%s' does not exist or could not be executed.", actual_cmd);
 			}
             break;
 		}

proc.cpp

@@ -388,6 +388,12 @@ static void mark_process_status( const job_t *j,
 	}
 }
 
+void job_mark_process_as_failed( const job_t *job, process_t *p )
+{
+    /* The given process failed to even lift off (e.g. posix_spawn failed) and so doesn't have a valid pid. Mark it as dead. */
+    p->completed = 1;
+}
+
 /**
    Handle status update for child \c pid. This function is called by
    the signal handler, so it mustn't use malloc or any such hitech

proc.h

@@ -509,6 +509,9 @@ void job_handle_signal( int signal, siginfo_t *info, void *con );
 */
 int job_signal( job_t *j, int signal );
 
+/* Marks a process as failed to execute (and therefore completed) */
+void job_mark_process_as_failed( const job_t *job, process_t *p );
+
 #ifdef HAVE__PROC_SELF_STAT
 /**
    Use the procfs filesystem to look up how many jiffies of cpu time