diff --git a/exec.cpp b/exec.cpp index 00abd5d93..eeb59ca8a 100644 --- a/exec.cpp +++ b/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,6 +1279,13 @@ 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 @@ -1261,9 +1293,7 @@ void exec( parser_t &parser, job_t *j ) 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 ); diff --git a/postfork.cpp b/postfork.cpp index 9f94ae006..4379ca2dd 100644 --- a/postfork.cpp +++ b/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; } diff --git a/proc.cpp b/proc.cpp index 59d64ee95..b61baf59f 100644 --- a/proc.cpp +++ b/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 diff --git a/proc.h b/proc.h index aea95ba62..6feb9afe7 100644 --- a/proc.h +++ b/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