From: Brandon Williams <bmwill@google.com>
To: git@vger.kernel.org
Cc: Brandon Williams <bmwill@google.com>, jrnieder@gmail.com, e@80x24.org
Subject: [PATCH v2 5/6] run-command: eliminate calls to error handling functions in child
Date: Thu, 13 Apr 2017 11:32:51 -0700 [thread overview]
Message-ID: <20170413183252.4713-6-bmwill@google.com> (raw)
In-Reply-To: <20170413183252.4713-1-bmwill@google.com>
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>
---
run-command.c | 121 ++++++++++++++++++++++++++++++++++++++++++----------------
1 file changed, 89 insertions(+), 32 deletions(-)
diff --git a/run-command.c b/run-command.c
index 6751b8319..4230c4933 100644
--- 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;
-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)
@@ -355,13 +423,6 @@ static int wait_or_whine(pid_t pid, const char *argv0, int in_signal)
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);
}
@@ -449,6 +510,7 @@ int start_command(struct child_process *cmd)
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;
@@ -467,20 +529,16 @@ int start_command(struct child_process *cmd)
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];
- atexit(notify_parent);
if (cmd->no_stdin)
dup2(null_fd, 0);
@@ -515,19 +573,17 @@ int start_command(struct child_process *cmd)
}
if (cmd->dir && chdir(cmd->dir))
- die_errno("exec '%s': cd to '%s' failed", cmd->argv[0],
- cmd->dir);
+ child_die(CHILD_ERR_CHDIR);
execve(argv.argv[0], (char *const *) argv.argv,
(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 {
- die_errno("cannot exec '%s'", cmd->argv[0]);
+ child_die(CHILD_ERR_ERRNO);
}
}
if (cmd->pid < 0)
@@ -538,17 +594,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
- * 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]);
- if (read(notify_pipe[0], ¬ify_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);
+ child_err_spew(cmd, &cerr);
failed_errno = errno;
cmd->pid = -1;
}
--
2.12.2.762.g0e3151a226-goog
next prev parent reply other threads:[~2017-04-13 18:33 UTC|newest]
Thread overview: 140+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-04-10 23:49 [PATCH 0/5] forking and threading Brandon Williams
2017-04-10 23:49 ` [PATCH 1/5] run-command: convert sane_execvp to sane_execvpe Brandon Williams
2017-04-12 19:22 ` Brandon Williams
2017-04-10 23:49 ` [PATCH 2/5] run-command: prepare argv before forking Brandon Williams
2017-04-10 23:49 ` [PATCH 3/5] run-command: allocate child_err " Brandon Williams
2017-04-10 23:49 ` [PATCH 4/5] run-command: prepare child environment " Brandon Williams
2017-04-11 0:58 ` Jonathan Nieder
2017-04-11 17:27 ` Brandon Williams
2017-04-11 17:30 ` Jonathan Nieder
2017-04-10 23:49 ` [PATCH 5/5] run-command: add note about forking and threading Brandon Williams
2017-04-11 0:26 ` Jonathan Nieder
2017-04-11 0:53 ` Eric Wong
2017-04-11 17:33 ` Jonathan Nieder
2017-04-11 17:34 ` Brandon Williams
2017-04-11 17:40 ` Eric Wong
2017-04-11 7:05 ` [PATCH 6/5] run-command: avoid potential dangers in forked child Eric Wong
2017-04-11 16:29 ` Brandon Williams
2017-04-11 16:59 ` Eric Wong
2017-04-11 17:17 ` Brandon Williams
2017-04-11 17:37 ` [PATCH 0/5] forking and threading Jonathan Nieder
2017-04-11 17:54 ` Brandon Williams
2017-04-13 18:32 ` [PATCH v2 0/6] " Brandon Williams
2017-04-13 18:32 ` [PATCH v2 1/6] t5550: use write_script to generate post-update hook Brandon Williams
2017-04-13 20:43 ` Jonathan Nieder
2017-04-13 20:59 ` Eric Wong
2017-04-13 21:35 ` Brandon Williams
2017-04-13 21:39 ` Eric Wong
2017-04-13 18:32 ` [PATCH v2 2/6] run-command: prepare command before forking Brandon Williams
2017-04-13 21:14 ` Jonathan Nieder
2017-04-13 22:41 ` Brandon Williams
2017-04-13 18:32 ` [PATCH v2 3/6] run-command: prepare child environment " Brandon Williams
2017-04-13 18:32 ` [PATCH v2 4/6] run-command: don't die in child when duping /dev/null Brandon Williams
2017-04-13 19:29 ` Eric Wong
2017-04-13 19:43 ` Brandon Williams
2017-04-13 18:32 ` Brandon Williams [this message]
2017-04-13 18:32 ` [PATCH v2 6/6] run-command: add note about forking and threading Brandon Williams
2017-04-13 20:50 ` [PATCH v2 0/6] " Jonathan Nieder
2017-04-13 23:44 ` Brandon Williams
2017-04-13 21:14 ` [PATCH 7/6] run-command: block signals between fork and execve Eric Wong
2017-04-13 23:37 ` Brandon Williams
2017-04-14 2:42 ` Brandon Williams
2017-04-14 5:26 ` Eric Wong
2017-04-14 5:35 ` Eric Wong
2017-04-14 16:58 ` [PATCH v3 00/10] forking and threading Brandon Williams
2017-04-14 16:58 ` [PATCH v3 01/10] t5550: use write_script to generate post-update hook Brandon Williams
2017-04-14 16:58 ` [PATCH v3 02/10] t0061: run_command executes scripts without a #! line Brandon Williams
2017-04-14 16:58 ` [PATCH v3 03/10] run-command: prepare command before forking Brandon Williams
2017-04-14 16:58 ` [PATCH v3 04/10] run-command: use the async-signal-safe execv instead of execvp Brandon Williams
2017-04-14 16:58 ` [PATCH v3 05/10] run-command: prepare child environment before forking Brandon Williams
2017-04-14 16:58 ` [PATCH v3 06/10] run-command: don't die in child when duping /dev/null Brandon Williams
2017-04-14 19:38 ` Eric Wong
2017-04-14 20:19 ` Brandon Williams
2017-04-14 16:58 ` [PATCH v3 07/10] run-command: eliminate calls to error handling functions in child Brandon Williams
2017-04-14 18:50 ` Eric Wong
2017-04-14 20:22 ` Brandon Williams
2017-04-14 16:59 ` [PATCH v3 08/10] run-command: handle dup2 and close errors " Brandon Williams
2017-04-14 16:59 ` [PATCH v3 09/10] run-command: add note about forking and threading Brandon Williams
2017-04-14 16:59 ` [PATCH v3 10/10] run-command: block signals between fork and execve Brandon Williams
2017-04-14 20:24 ` Brandon Williams
2017-04-14 21:35 ` Eric Wong
2017-04-17 22:08 ` [PATCH v4 00/10] forking and threading Brandon Williams
2017-04-17 22:08 ` [PATCH v4 01/10] t5550: use write_script to generate post-update hook Brandon Williams
2017-04-17 22:08 ` [PATCH v4 02/10] t0061: run_command executes scripts without a #! line Brandon Williams
2017-04-17 22:08 ` [PATCH v4 03/10] run-command: prepare command before forking Brandon Williams
2017-04-17 22:08 ` [PATCH v4 04/10] run-command: use the async-signal-safe execv instead of execvp Brandon Williams
2017-04-17 22:08 ` [PATCH v4 05/10] run-command: prepare child environment before forking Brandon Williams
2017-04-18 0:26 ` Eric Wong
2017-04-18 21:02 ` Brandon Williams
2017-04-17 22:08 ` [PATCH v4 06/10] run-command: don't die in child when duping /dev/null Brandon Williams
2017-04-17 22:08 ` [PATCH v4 07/10] run-command: eliminate calls to error handling functions in child Brandon Williams
2017-04-17 22:08 ` [PATCH v4 08/10] run-command: handle dup2 and close errors " Brandon Williams
2017-04-17 22:08 ` [PATCH v4 09/10] run-command: add note about forking and threading Brandon Williams
2017-04-17 22:08 ` [PATCH v4 10/10] run-command: block signals between fork and execve Brandon Williams
2017-04-18 23:17 ` [PATCH v5 00/11] forking and threading Brandon Williams
2017-04-18 23:17 ` [PATCH v5 01/11] t5550: use write_script to generate post-update hook Brandon Williams
2017-04-18 23:17 ` [PATCH v5 02/11] t0061: run_command executes scripts without a #! line Brandon Williams
2017-04-19 5:43 ` Johannes Sixt
2017-04-19 6:21 ` Johannes Sixt
2017-04-19 15:56 ` Brandon Williams
2017-04-19 18:18 ` Johannes Sixt
2017-04-20 10:47 ` Johannes Schindelin
2017-04-20 17:02 ` Brandon Williams
2017-04-20 20:24 ` Johannes Schindelin
2017-04-20 20:49 ` Brandon Williams
2017-04-18 23:17 ` [PATCH v5 03/11] run-command: prepare command before forking Brandon Williams
2017-04-18 23:17 ` [PATCH v5 04/11] run-command: use the async-signal-safe execv instead of execvp Brandon Williams
2017-04-18 23:17 ` [PATCH v5 05/11] string-list: add string_list_remove function Brandon Williams
2017-04-18 23:31 ` Stefan Beller
2017-04-18 23:36 ` Brandon Williams
2017-04-18 23:40 ` Stefan Beller
2017-04-18 23:18 ` [PATCH v5 06/11] run-command: prepare child environment before forking Brandon Williams
2017-04-18 23:18 ` [PATCH v5 07/11] run-command: don't die in child when duping /dev/null Brandon Williams
2017-04-18 23:18 ` [PATCH v5 08/11] run-command: eliminate calls to error handling functions in child Brandon Williams
2017-04-18 23:18 ` [PATCH v5 09/11] run-command: handle dup2 and close errors " Brandon Williams
2017-04-18 23:18 ` [PATCH v5 10/11] run-command: add note about forking and threading Brandon Williams
2017-04-18 23:18 ` [PATCH v5 11/11] run-command: block signals between fork and execve Brandon Williams
2017-04-19 6:00 ` Johannes Sixt
2017-04-19 7:48 ` Eric Wong
2017-04-19 16:10 ` Brandon Williams
2017-04-19 23:13 ` [PATCH v6 00/11] forking and threading Brandon Williams
2017-04-19 23:13 ` [PATCH v6 01/11] t5550: use write_script to generate post-update hook Brandon Williams
2017-04-19 23:13 ` [PATCH v6 02/11] t0061: run_command executes scripts without a #! line Brandon Williams
2017-04-20 10:49 ` Johannes Schindelin
2017-04-20 16:58 ` Brandon Williams
2017-04-19 23:13 ` [PATCH v6 03/11] run-command: prepare command before forking Brandon Williams
2017-04-19 23:13 ` [PATCH v6 04/11] run-command: use the async-signal-safe execv instead of execvp Brandon Williams
2017-05-17 2:15 ` Junio C Hamano
2017-05-17 2:26 ` Jeff King
2017-05-17 2:28 ` Jeff King
2017-05-17 3:41 ` Junio C Hamano
2017-05-17 14:52 ` Brandon Williams
2017-04-19 23:13 ` [PATCH v6 05/11] string-list: add string_list_remove function Brandon Williams
2017-04-19 23:13 ` [PATCH v6 06/11] run-command: prepare child environment before forking Brandon Williams
2017-04-19 23:13 ` [PATCH v6 07/11] run-command: don't die in child when duping /dev/null Brandon Williams
2017-04-19 23:13 ` [PATCH v6 08/11] run-command: eliminate calls to error handling functions in child Brandon Williams
2017-04-19 23:13 ` [PATCH v6 09/11] run-command: handle dup2 and close errors " Brandon Williams
2017-04-19 23:13 ` [PATCH v6 10/11] run-command: add note about forking and threading Brandon Williams
2017-04-19 23:13 ` [PATCH v6 11/11] run-command: block signals between fork and execve Brandon Williams
2017-04-24 22:37 ` [PATCH v6 00/11] forking and threading Brandon Williams
2017-04-24 23:50 ` [PATCH v6 12/11] run-command: don't try to execute directories Brandon Williams
2017-04-25 0:17 ` Jonathan Nieder
2017-04-25 1:58 ` Junio C Hamano
2017-04-25 2:51 ` Jonathan Nieder
2017-04-25 2:56 ` Jeff King
2017-04-25 1:47 ` Junio C Hamano
2017-04-25 2:57 ` Jonathan Nieder
2017-04-25 17:54 ` [PATCH v7 1/2] exec_cmd: expose is_executable function Brandon Williams
2017-04-25 17:54 ` [PATCH v7 2/2] run-command: don't try to execute directories Brandon Williams
2017-04-25 18:51 ` Jonathan Nieder
2017-04-25 19:32 ` Brandon Williams
2017-04-25 18:04 ` [PATCH v7 1/2] exec_cmd: expose is_executable function Jonathan Nieder
2017-04-25 18:18 ` Johannes Sixt
2017-04-25 18:38 ` Brandon Williams
2017-04-25 23:46 ` [PATCH v8 1/2] run-command: " Brandon Williams
2017-04-25 23:47 ` [PATCH v8 2/2] run-command: restrict PATH search to executable files Brandon Williams
2017-04-25 23:50 ` Jonathan Nieder
2017-04-26 1:44 ` Junio C Hamano
2017-04-26 17:10 ` [PATCH v9 " Brandon Williams
2017-04-27 0:33 ` Junio C Hamano
2017-04-25 23:48 ` [PATCH v8 1/2] run-command: expose is_executable function Jonathan Nieder
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170413183252.4713-6-bmwill@google.com \
--to=bmwill@google.com \
--cc=e@80x24.org \
--cc=git@vger.kernel.org \
--cc=jrnieder@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).