Merge branch 'jk/run-command-capture'

The run-command interface was easy to abuse and make a pipe for us
to read from the process, wait for the process to finish and then
attempt to read its output, which is a pattern that lead to a
deadlock.  Fix such uses by introducing a helper to do this
correctly (i.e. we need to read first and then wait the process to
finish) and also add code to prevent such abuse in the run-command
helper.

* jk/run-command-capture:
run-command: forbid using run_command with piped output
trailer: use capture_command
submodule: use capture_command
wt-status: use capture_command
run-command: introduce capture_command helper
wt_status: fix signedness mismatch in strbuf_read call
wt-status: don't flush before running "submodule status"

diff --git a/run-command.c b/run-command.c
index 3afb124c79dcb234344e17795205153d4a2e0643..aad03ab705f301268980079282fcf370b9275c5a 100644 (file)
--- a/run-command.c
+++ b/run-command.c
@@ -557,7 +557,12 @@ int finish_command(struct child_process *cmd)
 
 int run_command(struct child_process *cmd)
 {
-       int code = start_command(cmd);
+       int code;
+
+       if (cmd->out < 0 || cmd->err < 0)
+               die("BUG: run_command with a pipe can cause deadlock");
+
+       code = start_command(cmd);
        if (code)
                return code;
        return finish_command(cmd);
@@ -829,3 +834,19 @@ int run_hook_le(const char *const *env, const char *name, ...)
 
        return ret;
 }
+
+int capture_command(struct child_process *cmd, struct strbuf *buf, size_t hint)
+{
+       cmd->out = -1;
+       if (start_command(cmd) < 0)
+               return -1;
+
+       if (strbuf_read(buf, cmd->out, hint) < 0) {
+               close(cmd->out);
+               finish_command(cmd); /* throw away exit code */
+               return -1;
+       }
+
+       close(cmd->out);
+       return finish_command(cmd);
+}

diff --git a/run-command.h b/run-command.h
index d6868dc8c868a61fb1ccc6230f43048dd9838a24..263b9662adeba011adcd018f77b7ccbbcd53e94a 100644 (file)
--- a/run-command.h
+++ b/run-command.h
@@ -71,6 +71,19 @@ int run_command_v_opt(const char **argv, int opt);
  */
 int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const char *const *env);
 
+/**
+ * Execute the given command, capturing its stdout in the given strbuf.
+ * Returns -1 if starting the command fails or reading fails, and otherwise
+ * returns the exit code of the command. The output collected in the
+ * buffer is kept even if the command returns a non-zero exit. The hint field
+ * gives a starting size for the strbuf allocation.
+ *
+ * The fields of "cmd" should be set up as they would for a normal run_command
+ * invocation. But note that there is no need to set cmd->out; the function
+ * sets it up for the caller.
+ */
+int capture_command(struct child_process *cmd, struct strbuf *buf, size_t hint);
+
 /*
  * The purpose of the following functions is to feed a pipe by running
  * a function asynchronously and providing output that the caller reads.

diff --git a/submodule.c b/submodule.c
index d37d400b2227c58f352795b9f6ede20a9cd54631..c0e6c81fc4656342fedeb4b5b68d9b938cb44b84 100644 (file)
--- a/submodule.c
+++ b/submodule.c
@@ -576,12 +576,10 @@ static int is_submodule_commit_present(const char *path, unsigned char sha1[20])
                cp.env = local_repo_env;
                cp.git_cmd = 1;
                cp.no_stdin = 1;
-               cp.out = -1;
                cp.dir = path;
-               if (!run_command(&cp) && !strbuf_read(&buf, cp.out, 1024))
+               if (!capture_command(&cp, &buf, 1024) && !buf.len)
                        is_present = 1;
 
-               close(cp.out);
                strbuf_release(&buf);
        }
        return is_present;

diff --git a/trailer.c b/trailer.c
index 05b3859b476dc19f37d8715ac012c29373e8f91e..4b14a567b418acd3181fcf6e37f246c91d60eb60 100644 (file)
--- a/trailer.c
+++ b/trailer.c
@@ -214,16 +214,6 @@ static struct trailer_item *remove_first(struct trailer_item **first)
        return item;
 }
 
-static int read_from_command(struct child_process *cp, struct strbuf *buf)
-{
-       if (run_command(cp))
-               return error("running trailer command '%s' failed", cp->argv[0]);
-       if (strbuf_read(buf, cp->out, 1024) < 1)
-               return error("reading from trailer command '%s' failed", cp->argv[0]);
-       strbuf_trim(buf);
-       return 0;
-}
-
 static const char *apply_command(const char *command, const char *arg)
 {
        struct strbuf cmd = STRBUF_INIT;
@@ -240,14 +230,16 @@ static const char *apply_command(const char *command, const char *arg)
        cp.argv = argv;
        cp.env = local_repo_env;
        cp.no_stdin = 1;
-       cp.out = -1;
        cp.use_shell = 1;
 
-       if (read_from_command(&cp, &buf)) {
+       if (capture_command(&cp, &buf, 1024)) {
+               error("running trailer command '%s' failed", cmd.buf);
                strbuf_release(&buf);
                result = xstrdup("");
-       } else
+       } else {
+               strbuf_trim(&buf);
                result = strbuf_detach(&buf, NULL);
+       }
 
        strbuf_release(&cmd);
        return result;

diff --git a/wt-status.c b/wt-status.c
index 7036fa28dc39f72ed2064ca0ad15f2e381808020..853419f05f232045cf4ad3f8a8a2af04be9329a0 100644 (file)
--- a/wt-status.c
+++ b/wt-status.c
@@ -729,7 +729,6 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt
        struct strbuf cmd_stdout = STRBUF_INIT;
        struct strbuf summary = STRBUF_INIT;
        char *summary_content;
-       size_t len;
 
        argv_array_pushf(&sm_summary.env_array, "GIT_INDEX_FILE=%s",
                         s->index_file);
@@ -745,15 +744,11 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt
 
        sm_summary.git_cmd = 1;
        sm_summary.no_stdin = 1;
-       fflush(s->fp);
-       sm_summary.out = -1;
 
-       run_command(&sm_summary);
-
-       len = strbuf_read(&cmd_stdout, sm_summary.out, 1024);
+       capture_command(&sm_summary, &cmd_stdout, 1024);
 
        /* prepend header, only if there's an actual output */
-       if (len) {
+       if (cmd_stdout.len) {
                if (uncommitted)
                        strbuf_addstr(&summary, _("Submodules changed but not updated:"));
                else
@@ -764,6 +759,7 @@ static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitt
        strbuf_release(&cmd_stdout);
 
        if (s->display_comment_prefix) {
+               size_t len;
                summary_content = strbuf_detach(&summary, &len);
                strbuf_add_commented_lines(&summary, summary_content, len);
                free(summary_content);