start_command(), if .in/.out > 0, closes file descriptors, not the callers

Callers of start_command() can set the members .in and .out of struct
child_process to a value > 0 to specify that this descriptor is used as
the stdin or stdout of the child process.

Previously, if start_command() was successful, this descriptor was closed
upon return. Here we now make sure that the descriptor is also closed in
case of failures. All callers are updated not to close the file descriptor
themselves after start_command() was called.

Note that earlier run_gpg_verify() of git-verify-tag set .out = 1, which
worked because start_command() treated this as a special case, but now
this is incorrect because it closes the descriptor. The intent here is to
inherit stdout to the child, which is achieved by .out = 0.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index ba9bc91a5c513273ba837dd04aa568f8f67b7121..b0cfae83fc4dcdd8f58e29b87fd8aa4b6af0f6c0 100644 (file)
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -404,12 +404,15 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
        if (!remote_tail)
                remote_tail = &remote_refs;
        if (match_refs(local_refs, remote_refs, &remote_tail,
-                                              nr_refspec, refspec, flags))
+                      nr_refspec, refspec, flags)) {
+               close(out);
                return -1;
+       }
 
        if (!remote_refs) {
                fprintf(stderr, "No refs in common and none specified; doing nothing.\n"
                        "Perhaps you should specify a branch such as 'master'.\n");
+               close(out);
                return 0;
        }
 
@@ -496,12 +499,11 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
 
        packet_flush(out);
        if (new_refs && !args.dry_run) {
-               if (pack_objects(out, remote_refs) < 0) {
-                       close(out);
+               if (pack_objects(out, remote_refs) < 0)
                        return -1;
-               }
        }
-       close(out);
+       else
+               close(out);
 
        if (expect_status_report)
                ret = receive_status(in, remote_refs);
@@ -649,7 +651,7 @@ int send_pack(struct send_pack_args *my_args,
        conn = git_connect(fd, dest, args.receivepack, args.verbose ? CONNECT_VERBOSE : 0);
        ret = do_send_pack(fd[0], fd[1], remote, dest, nr_heads, heads);
        close(fd[0]);
-       close(fd[1]);
+       /* do_send_pack always closes fd[1] */
        ret |= finish_connect(conn);
        return !!ret;
 }

diff --git a/builtin-verify-tag.c b/builtin-verify-tag.c
index b3010f9827a9df6c3113fc0b90a8fbde4cf18bb7..f3ef11fa2d582682b428d6e165da65f05e2d7511 100644 (file)
--- a/builtin-verify-tag.c
+++ b/builtin-verify-tag.c
@@ -45,7 +45,6 @@ static int run_gpg_verify(const char *buf, unsigned long size, int verbose)
        memset(&gpg, 0, sizeof(gpg));
        gpg.argv = args_gpg;
        gpg.in = -1;
-       gpg.out = 1;
        args_gpg[2] = path;
        if (start_command(&gpg))
                return error("could not run gpg.");

diff --git a/bundle.c b/bundle.c
index 4352ce817f0488face752bd40d630255fe6ca186..0ba5df17e15d679b03fe38af40260c118c9588fa 100644 (file)
--- a/bundle.c
+++ b/bundle.c
@@ -336,8 +336,9 @@ int create_bundle(struct bundle_header *header, const char *path,
        close(rls.in);
        if (finish_command(&rls))
                return error ("pack-objects died");
-
-       return bundle_to_stdout ? close(bundle_fd) : commit_lock_file(&lock);
+       if (!bundle_to_stdout)
+               commit_lock_file(&lock);
+       return 0;
 }
 
 int unbundle(struct bundle_header *header, int bundle_fd)

diff --git a/run-command.c b/run-command.c
index 2919330366bbd52aca7fb860e4f82e7216652925..743757c36ec0e5667fd8af28800ac095f1581adb 100644 (file)
--- a/run-command.c
+++ b/run-command.c
@@ -20,10 +20,18 @@ int start_command(struct child_process *cmd)
        int need_in, need_out, need_err;
        int fdin[2], fdout[2], fderr[2];
 
+       /*
+        * In case of errors we must keep the promise to close FDs
+        * that have been passed in via ->in and ->out.
+        */
+
        need_in = !cmd->no_stdin && cmd->in < 0;
        if (need_in) {
-               if (pipe(fdin) < 0)
+               if (pipe(fdin) < 0) {
+                       if (cmd->out > 0)
+                               close(cmd->out);
                        return -ERR_RUN_COMMAND_PIPE;
+               }
                cmd->in = fdin[1];
        }
 
@@ -34,6 +42,8 @@ int start_command(struct child_process *cmd)
                if (pipe(fdout) < 0) {
                        if (need_in)
                                close_pair(fdin);
+                       else if (cmd->in)
+                               close(cmd->in);
                        return -ERR_RUN_COMMAND_PIPE;
                }
                cmd->out = fdout[0];
@@ -44,8 +54,12 @@ int start_command(struct child_process *cmd)
                if (pipe(fderr) < 0) {
                        if (need_in)
                                close_pair(fdin);
+                       else if (cmd->in)
+                               close(cmd->in);
                        if (need_out)
                                close_pair(fdout);
+                       else if (cmd->out)
+                               close(cmd->out);
                        return -ERR_RUN_COMMAND_PIPE;
                }
                cmd->err = fderr[0];
@@ -55,8 +69,12 @@ int start_command(struct child_process *cmd)
        if (cmd->pid < 0) {
                if (need_in)
                        close_pair(fdin);
+               else if (cmd->in)
+                       close(cmd->in);
                if (need_out)
                        close_pair(fdout);
+               else if (cmd->out)
+                       close(cmd->out);
                if (need_err)
                        close_pair(fderr);
                return -ERR_RUN_COMMAND_FORK;
@@ -118,7 +136,7 @@ int start_command(struct child_process *cmd)
 
        if (need_out)
                close(fdout[1]);
-       else if (cmd->out > 1)
+       else if (cmd->out)
                close(cmd->out);
 
        if (need_err)

diff --git a/run-command.h b/run-command.h
index e9c84d03639a59178e16361ee70df95835b9beb3..debe3074b5a01fb5a19e61f07ff66c250cdc4f82 100644 (file)
--- a/run-command.h
+++ b/run-command.h
@@ -14,6 +14,24 @@ enum {
 struct child_process {
        const char **argv;
        pid_t pid;
+       /*
+        * Using .in, .out, .err:
+        * - Specify 0 for no redirections (child inherits stdin, stdout,
+        *   stderr from parent).
+        * - Specify -1 to have a pipe allocated as follows:
+        *     .in: returns the writable pipe end; parent writes to it,
+        *          the readable pipe end becomes child's stdin
+        *     .out, .err: returns the readable pipe end; parent reads from
+        *          it, the writable pipe end becomes child's stdout/stderr
+        *   The caller of start_command() must close the returned FDs
+        *   after it has completed reading from/writing to it!
+        * - Specify > 0 to set a channel to a particular FD as follows:
+        *     .in: a readable FD, becomes child's stdin
+        *     .out: a writable FD, becomes child's stdout/stderr
+        *     .err > 0 not supported
+        *   The specified FD is closed by start_command(), even in case
+        *   of errors!
+        */
        int in;
        int out;
        int err;