git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Johannes Sixt <johannes.sixt@telecom.at>
Date
Feb 21, 2008, 22:42 UTC
Message-ID
<200802212342.56629.johannes.sixt@telecom.at>
In-Reply-To
<200802171029.17850.johannes.sixt@telecom.at>

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>
---
On Sunday 17 February 2008 10:29, Johannes Sixt wrote:
Show 15 quoted lines
> On Saturday 16 February 2008 23:55, Junio C Hamano wrote:
> > Johannes Sixt <johannes.sixt@telecom.at> writes:
> > > +	 * - Specify > 0 to give away a 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!
> >
> > Perhaps you would need to spell out the semantic differences you
> > are assigning to "inherit" vs "give away".  I presume the former
> > is something run_command() would not touch vs the latter is
> > closed by run_command()?
>
> I'll clearify it.
And here it is.

The other issues you raised -- ->out sometimes compared > 0, sometimes > 1, and treated different from ->in -- I addressed, too, by adjusting all uses that were inconsitent. Note that there is still one ->out > 0, but that is because we don't have set up need_out at this time, and I also left the cmd->out > 1 in the child process alone.

Note also the amended commit message that describes the change in builtin-verify-tag.c.

-- Hannes
 builtin-send-pack.c  |   14 ++++++++------
 builtin-verify-tag.c |    1 -
 bundle.c             |    5 +++--
 run-command.c        |   22 ++++++++++++++++++++--
 run-command.h        |   18 ++++++++++++++++++
 5 files changed, 49 insertions(+), 11 deletions(-)
diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index ba9bc91..b0cfae8 100644
--- 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 b3010f9..f3ef11f 100644
--- 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 4352ce8..0ba5df1 100644
--- 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 2919330..743757c 100644
--- 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 e9c84d0..debe307 100644
--- 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;

-- 
1.5.4.1.126.ge5a7d
Previous: Johannes SixtNext: Junio C Hamano
Message 13 of 15 in “Automatically close stderr pipes created by run_command”
  1. 2/6 Automatically close stderr pipes created by run_commandShawn O. Pearce, Feb 14, 2008
  2. Johannes SixtFeb 14, 2008
  3. Junio C HamanoFeb 15, 2008
  4. Johannes SixtFeb 15, 2008
  5. Junio C HamanoFeb 15, 2008
  6. start_command(), .in/.out/.err = -1: Callers must close the file descriptorJohannes Sixt, Feb 16, 2008
  7. start_command(), if .in/.out > 0, closes file descriptors, not the callersJohannes Sixt, Feb 16, 2008
  8. Junio C HamanoFeb 16, 2008
  9. Shawn O. PearceFeb 17, 2008
  10. Junio C HamanoFeb 17, 2008
  11. Shawn O. PearceFeb 17, 2008
  12. Johannes SixtFeb 17, 2008
  13. start_command(), if .in/.out > 0, closes file descriptors, not the callersJohannes Sixt, Feb 21, 2008
  14. Junio C HamanoFeb 22, 2008
  15. Junio C HamanoFeb 16, 2008

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.