run-command: eliminate calls to error handling functions in child

All of our standard error handling paths have the potential to
call malloc or take stdio locks; so we must avoid them inside
the forked child.

Instead, the child only writes an 8 byte struct atomically to
the parent through the notification pipe to propagate an error.
All user-visible error reporting happens from the parent;
even avoiding functions like atexit(3) and exit(3).

Helped-by: Eric Wong <e@80x24.org>
Signed-off-by: Brandon Williams <bmwill@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/run-command.c b/run-command.c
index b3a35dd82db18590bfad48f557873e7a47a3761d..1f15714b157db79231d7cfaf6f90e354b855647d 100644 (file)
--- a/run-command.c
+++ b/run-command.c
@@ -211,14 +211,82 @@ static const char **prepare_shell_cmd(struct argv_array *out, const char **argv)
 #ifndef GIT_WINDOWS_NATIVE
 static int child_notifier = -1;
 
 #ifndef GIT_WINDOWS_NATIVE
 static int child_notifier = -1;
 

-static void notify_parent(void)
+enum child_errcode {
+       CHILD_ERR_CHDIR,
+       CHILD_ERR_ENOENT,
+       CHILD_ERR_SILENT,
+       CHILD_ERR_ERRNO
+};
+
+struct child_err {
+       enum child_errcode err;
+       int syserr; /* errno */
+};
+
+static void child_die(enum child_errcode err)

 {
 {

-       /*
-        * execvp failed.  If possible, we'd like to let start_command
-        * know, so failures like ENOENT can be handled right away; but
-        * otherwise, finish_command will still report the error.
-        */
-       xwrite(child_notifier, "", 1);
+       struct child_err buf;
+
+       buf.err = err;
+       buf.syserr = errno;
+
+       /* write(2) on buf smaller than PIPE_BUF (min 512) is atomic: */
+       xwrite(child_notifier, &buf, sizeof(buf));
+       _exit(1);
+}
+
+/*
+ * parent will make it look like the child spewed a fatal error and died
+ * this is needed to prevent changes to t0061.
+ */
+static void fake_fatal(const char *err, va_list params)
+{
+       vreportf("fatal: ", err, params);
+}
+
+static void child_error_fn(const char *err, va_list params)
+{
+       const char msg[] = "error() should not be called in child\n";
+       xwrite(2, msg, sizeof(msg) - 1);
+}
+
+static void child_warn_fn(const char *err, va_list params)
+{
+       const char msg[] = "warn() should not be called in child\n";
+       xwrite(2, msg, sizeof(msg) - 1);
+}
+
+static void NORETURN child_die_fn(const char *err, va_list params)
+{
+       const char msg[] = "die() should not be called in child\n";
+       xwrite(2, msg, sizeof(msg) - 1);
+       _exit(2);
+}
+
+/* this runs in the parent process */
+static void child_err_spew(struct child_process *cmd, struct child_err *cerr)
+{
+       static void (*old_errfn)(const char *err, va_list params);
+
+       old_errfn = get_error_routine();
+       set_error_routine(fake_fatal);
+       errno = cerr->syserr;
+
+       switch (cerr->err) {
+       case CHILD_ERR_CHDIR:
+               error_errno("exec '%s': cd to '%s' failed",
+                           cmd->argv[0], cmd->dir);
+               break;
+       case CHILD_ERR_ENOENT:
+               error_errno("cannot run %s", cmd->argv[0]);
+               break;
+       case CHILD_ERR_SILENT:
+               break;
+       case CHILD_ERR_ERRNO:
+               error_errno("cannot exec '%s'", cmd->argv[0]);
+               break;
+       }
+       set_error_routine(old_errfn);

 }
 
 static void prepare_cmd(struct argv_array *out, const struct child_process *cmd)
 }
 
 static void prepare_cmd(struct argv_array *out, const struct child_process *cmd)


@@ -341,13 +409,6 @@ static int wait_or_whine(pid_t pid, const char *argv0, int in_signal)
                code += 128;
        } else if (WIFEXITED(status)) {
                code = WEXITSTATUS(status);
                code += 128;
        } else if (WIFEXITED(status)) {
                code = WEXITSTATUS(status);

-               /*
-                * Convert special exit code when execvp failed.
-                */
-               if (code == 127) {
-                       code = -1;
-                       failed_errno = ENOENT;
-               }

        } else {
                error("waitpid is confused (%s)", argv0);
        }
        } else {
                error("waitpid is confused (%s)", argv0);
        }


@@ -435,6 +496,7 @@ int start_command(struct child_process *cmd)
        int null_fd = -1;
        char **childenv;
        struct argv_array argv = ARGV_ARRAY_INIT;
        int null_fd = -1;
        char **childenv;
        struct argv_array argv = ARGV_ARRAY_INIT;

+       struct child_err cerr;

 
        if (pipe(notify_pipe))
                notify_pipe[0] = notify_pipe[1] = -1;
 
        if (pipe(notify_pipe))
                notify_pipe[0] = notify_pipe[1] = -1;


@@ -453,20 +515,16 @@ int start_command(struct child_process *cmd)
        failed_errno = errno;
        if (!cmd->pid) {
                /*
        failed_errno = errno;
        if (!cmd->pid) {
                /*

-                * Redirect the channel to write syscall error messages to
-                * before redirecting the process's stderr so that all die()
-                * in subsequent call paths use the parent's stderr.
+                * Ensure the default die/error/warn routines do not get
+                * called, they can take stdio locks and malloc.

                 */
                 */

-               if (cmd->no_stderr || need_err) {
-                       int child_err = dup(2);
-                       set_cloexec(child_err);
-                       set_error_handle(fdopen(child_err, "w"));
-               }
+               set_die_routine(child_die_fn);
+               set_error_routine(child_error_fn);
+               set_warn_routine(child_warn_fn);

 
                close(notify_pipe[0]);
                set_cloexec(notify_pipe[1]);
                child_notifier = notify_pipe[1];
 
                close(notify_pipe[0]);
                set_cloexec(notify_pipe[1]);
                child_notifier = notify_pipe[1];

-               atexit(notify_parent);

 
                if (cmd->no_stdin)
                        dup2(null_fd, 0);
 
                if (cmd->no_stdin)
                        dup2(null_fd, 0);


@@ -501,8 +559,7 @@ int start_command(struct child_process *cmd)
                }
 
                if (cmd->dir && chdir(cmd->dir))
                }
 
                if (cmd->dir && chdir(cmd->dir))

-                       die_errno("exec '%s': cd to '%s' failed", cmd->argv[0],
-                           cmd->dir);
+                       child_die(CHILD_ERR_CHDIR);

 
                /*
                 * Attempt to exec using the command and arguments starting at
 
                /*
                 * Attempt to exec using the command and arguments starting at


@@ -517,12 +574,11 @@ int start_command(struct child_process *cmd)
                               (char *const *) childenv);
 
                if (errno == ENOENT) {
                               (char *const *) childenv);
 
                if (errno == ENOENT) {

-                       if (!cmd->silent_exec_failure)
-                               error("cannot run %s: %s", cmd->argv[0],
-                                       strerror(ENOENT));
-                       exit(127);
+                       if (cmd->silent_exec_failure)
+                               child_die(CHILD_ERR_SILENT);
+                       child_die(CHILD_ERR_ENOENT);

                } else {
                } else {

-                       die_errno("cannot exec '%s'", cmd->argv[0]);
+                       child_die(CHILD_ERR_ERRNO);

                }
        }
        if (cmd->pid < 0)
                }
        }
        if (cmd->pid < 0)


@@ -533,17 +589,18 @@ int start_command(struct child_process *cmd)
        /*
         * Wait for child's exec. If the exec succeeds (or if fork()
         * failed), EOF is seen immediately by the parent. Otherwise, the
        /*
         * Wait for child's exec. If the exec succeeds (or if fork()
         * failed), EOF is seen immediately by the parent. Otherwise, the

-        * child process sends a single byte.
+        * child process sends a child_err struct.

         * Note that use of this infrastructure is completely advisory,
         * therefore, we keep error checks minimal.
         */
        close(notify_pipe[1]);
         * Note that use of this infrastructure is completely advisory,
         * therefore, we keep error checks minimal.
         */
        close(notify_pipe[1]);

-       if (read(notify_pipe[0], &notify_pipe[1], 1) == 1) {
+       if (xread(notify_pipe[0], &cerr, sizeof(cerr)) == sizeof(cerr)) {

                /*
                 * At this point we know that fork() succeeded, but exec()
                 * failed. Errors have been reported to our stderr.
                 */
                wait_or_whine(cmd->pid, cmd->argv[0], 0);
                /*
                 * At this point we know that fork() succeeded, but exec()
                 * failed. Errors have been reported to our stderr.
                 */
                wait_or_whine(cmd->pid, cmd->argv[0], 0);

+               child_err_spew(cmd, &cerr);

                failed_errno = errno;
                cmd->pid = -1;
        }
                failed_errno = errno;
                cmd->pid = -1;
        }