[PATCH 0/7] gpg-interface cleanups

[PATCH 0/7] gpg-interface cleanups

[Jeff King]

This started off with Michael's patch to sign_buffer, which is at the
tip, and then me trying to address the possible deadlocks there and in
verify_signed_buffer. While I was in the area, I took the opportunity to
do a few cleanups.

It's unclear to me whether the deadlocks are possible in practice; see
patch 5 for discussion. My guess is probably not, but the amount of code
to support doing it right is not all that much. But if we don't like it,
we can drop 4-6.

Patch 7 is still authored by Michael, but has been massaged greatly by
me. I'll comment more directly on the changes there.

  [1/7]: gpg-interface: use child_process.args
  [2/7]: verify_signed_buffer: drop pbuf variable
  [3/7]: verify_signed_buffer: use tempfile object
  [4/7]: run-command: add pipe_command helper
  [5/7]: verify_signed_buffer: use pipe_command
  [6/7]: sign_buffer: use pipe_command
  [7/7]: gpg-interface: check gpg signature creation status

[PATCH 1/7] gpg-interface: use child_process.args

[Jeff King]

Our argv allocations are relatively straightforward, but
this avoids us having to manually keep the count up to date
(or create new to-be-replaced slots in the declaration) when
we add new arguments.

Signed-off-by: Jeff King <peff@peff.net>
---
 gpg-interface.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index c4b1e8c..0ed9fa7 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -150,17 +150,15 @@ const char *get_signing_key(void)
 int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key)
 {
 	struct child_process gpg = CHILD_PROCESS_INIT;
-	const char *args[4];
 	ssize_t len;
 	size_t i, j, bottom;
 
-	gpg.argv = args;
 	gpg.in = -1;
 	gpg.out = -1;
-	args[0] = gpg_program;
-	args[1] = "-bsau";
-	args[2] = signing_key;
-	args[3] = NULL;
+	argv_array_pushl(&gpg.args,
+			 gpg_program,
+			 "-bsau", signing_key,
+			 NULL);
 
 	if (start_command(&gpg))
 		return error(_("could not run gpg."));
@@ -210,13 +208,11 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
 			 struct strbuf *gpg_output, struct strbuf *gpg_status)
 {
 	struct child_process gpg = CHILD_PROCESS_INIT;
-	const char *args_gpg[] = {NULL, "--status-fd=1", "--verify", "FILE", "-", NULL};
 	char path[PATH_MAX];
 	int fd, ret;
 	struct strbuf buf = STRBUF_INIT;
 	struct strbuf *pbuf = &buf;
 
-	args_gpg[0] = gpg_program;
 	fd = git_mkstemp(path, PATH_MAX, ".git_vtag_tmpXXXXXX");
 	if (fd < 0)
 		return error_errno(_("could not create temporary file '%s'"), path);
@@ -224,12 +220,15 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
 		return error_errno(_("failed writing detached signature to '%s'"), path);
 	close(fd);
 
-	gpg.argv = args_gpg;
+	argv_array_pushl(&gpg.args,
+			 gpg_program,
+			 "--status-fd=1",
+			 "--verify", path, "-",
+			 NULL);
 	gpg.in = -1;
 	gpg.out = -1;
 	if (gpg_output)
 		gpg.err = -1;
-	args_gpg[3] = path;
 	if (start_command(&gpg)) {
 		unlink(path);
 		return error(_("could not run gpg."));
-- 
2.9.0.160.g4984cba

[PATCH 2/7] verify_signed_buffer: drop pbuf variable

[Jeff King]

If our caller gave us a non-NULL gpg_status parameter, we
write the gpg status into their strbuf. If they didn't, then
we write it to a temporary local strbuf (since we still need
to look at it).  The variable "pbuf" adds an extra layer of
indirection so that the rest of the function can just access
whichever is appropriate.

However, the name "pbuf" isn't very descriptive, and it's
easy to get confused about what is supposed to be in it
(especially because we are reading both "status" and
"output" from gpg).

Rather than give it a more descriptive name, we can just use
gpg_status as our indirection pointer. Either it points to
the caller's input, or we can point it directly to our
temporary buffer.

Signed-off-by: Jeff King <peff@peff.net>
---
 gpg-interface.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index 0ed9fa7..216cad8 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -211,7 +211,6 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
 	char path[PATH_MAX];
 	int fd, ret;
 	struct strbuf buf = STRBUF_INIT;
-	struct strbuf *pbuf = &buf;
 
 	fd = git_mkstemp(path, PATH_MAX, ".git_vtag_tmpXXXXXX");
 	if (fd < 0)
@@ -242,9 +241,9 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
 		strbuf_read(gpg_output, gpg.err, 0);
 		close(gpg.err);
 	}
-	if (gpg_status)
-		pbuf = gpg_status;
-	strbuf_read(pbuf, gpg.out, 0);
+	if (!gpg_status)
+		gpg_status = &buf;
+	strbuf_read(gpg_status, gpg.out, 0);
 	close(gpg.out);
 
 	ret = finish_command(&gpg);
@@ -252,7 +251,7 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
 
 	unlink_or_warn(path);
 
-	ret |= !strstr(pbuf->buf, "\n[GNUPG:] GOODSIG ");
+	ret |= !strstr(gpg_status->buf, "\n[GNUPG:] GOODSIG ");
 	strbuf_release(&buf); /* no matter it was used or not */
 
 	return ret;
-- 
2.9.0.160.g4984cba

[PATCH 3/7] verify_signed_buffer: use tempfile object

[Jeff King]

We use git_mkstemp to create a temporary file, and try to
clean it up in all exit paths from the function. But that
misses any cases where we die by signal, or by calling die()
in a sub-function. In addition, we missed one of the exit
paths.

Let's convert to using a tempfile object, which handles the
hard cases for us, and add the missing cleanup call. Note
that we would not simply want to rely on program exit to
catch our missed cleanup, as this function may be called
many times in a single program (for the same reason, we use
a static tempfile instead of heap-allocating a new one; that
gives an upper bound on our memory usage).

Signed-off-by: Jeff King <peff@peff.net>
---
 gpg-interface.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index 216cad8..854c1e5 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -3,6 +3,7 @@
 #include "strbuf.h"
 #include "gpg-interface.h"
 #include "sigchain.h"
+#include "tempfile.h"
 
 static char *configured_signing_key;
 static const char *gpg_program = "gpg";
@@ -208,28 +209,32 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
 			 struct strbuf *gpg_output, struct strbuf *gpg_status)
 {
 	struct child_process gpg = CHILD_PROCESS_INIT;
-	char path[PATH_MAX];
+	static struct tempfile temp;
 	int fd, ret;
 	struct strbuf buf = STRBUF_INIT;
 
-	fd = git_mkstemp(path, PATH_MAX, ".git_vtag_tmpXXXXXX");
+	fd = mks_tempfile_t(&temp, ".git_vtag_tmpXXXXXX");
 	if (fd < 0)
-		return error_errno(_("could not create temporary file '%s'"), path);
-	if (write_in_full(fd, signature, signature_size) < 0)
-		return error_errno(_("failed writing detached signature to '%s'"), path);
+		return error_errno(_("could not create temporary file"));
+	if (write_in_full(fd, signature, signature_size) < 0) {
+		error_errno(_("failed writing detached signature to '%s'"),
+			    temp.filename.buf);
+		delete_tempfile(&temp);
+		return -1;
+	}
 	close(fd);
 
 	argv_array_pushl(&gpg.args,
 			 gpg_program,
 			 "--status-fd=1",
-			 "--verify", path, "-",
+			 "--verify", temp.filename.buf, "-",
 			 NULL);
 	gpg.in = -1;
 	gpg.out = -1;
 	if (gpg_output)
 		gpg.err = -1;
 	if (start_command(&gpg)) {
-		unlink(path);
+		delete_tempfile(&temp);
 		return error(_("could not run gpg."));
 	}
 
@@ -249,7 +254,7 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
 	ret = finish_command(&gpg);
 	sigchain_pop(SIGPIPE);
 
-	unlink_or_warn(path);
+	delete_tempfile(&temp);
 
 	ret |= !strstr(gpg_status->buf, "\n[GNUPG:] GOODSIG ");
 	strbuf_release(&buf); /* no matter it was used or not */
-- 
2.9.0.160.g4984cba

[PATCH 4/7] run-command: add pipe_command helper

[Jeff King]

We already have capture_command(), which captures the stdout
of a command in a way that avoids deadlocks. But sometimes
we need to do more I/O, like capturing stderr as well, or
sending data to stdin. It's easy to write code that
deadlocks racily in these situations depending on how fast
the command reads its input, or in which order it writes its
output.

Let's give callers an easy interface for doing this the
right way, similar to what capture_command() did for the
simple case.

The whole thing is backed by a generic poll() loop that can
feed an arbitrary number of buffers to descriptors, and fill
an arbitrary number of strbufs from other descriptors. This
seems like overkill, but the resulting code is actually a
bit cleaner than just handling the three descriptors
(because the output code for stdout/stderr is effectively
duplicated, so being able to loop is a benefit).

Signed-off-by: Jeff King <peff@peff.net>
---
It's possible this could come in handy other places, too. I didn't look,
but I suspect pump_io() could back some of the parallel submodule stuff.

 run-command.c | 152 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 run-command.h |  31 +++++++++---
 2 files changed, 171 insertions(+), 12 deletions(-)

diff --git a/run-command.c b/run-command.c
index af0c8a1..609fa4c 100644
--- a/run-command.c
+++ b/run-command.c
@@ -864,19 +864,161 @@ 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)
+struct io_pump {
+	/* initialized by caller */
+	int fd;
+	int type; /* POLLOUT or POLLIN */
+	union {
+		struct {
+			const char *buf;
+			size_t len;
+		} out;
+		struct {
+			struct strbuf *buf;
+			size_t hint;
+		} in;
+	} u;
+
+	/* returned by pump_io */
+	int error; /* 0 for success, otherwise errno */
+
+	/* internal use */
+	struct pollfd *pfd;
+};
+
+int pump_io_round(struct io_pump *slots, int nr, struct pollfd *pfd)
+{
+	int pollsize = 0;
+	int i;
+
+	for (i = 0; i < nr; i++) {
+		struct io_pump *io = &slots[i];
+		if (io->fd < 0)
+			continue;
+		pfd[pollsize].fd = io->fd;
+		pfd[pollsize].events = io->type;
+		io->pfd = &pfd[pollsize++];
+	}
+
+	if (!pollsize)
+		return 0;
+
+	if (poll(pfd, pollsize, -1) < 0) {
+		if (errno == EINTR)
+			return 1;
+		die_errno("poll failed");
+	}
+
+	for (i = 0; i < nr; i++) {
+		struct io_pump *io = &slots[i];
+
+		if (io->fd < 0)
+			continue;
+
+		if (!(io->pfd->revents & (POLLOUT|POLLIN|POLLHUP|POLLERR|POLLNVAL)))
+			continue;
+
+		if (io->type == POLLOUT) {
+			ssize_t len = xwrite(io->fd,
+					     io->u.out.buf, io->u.out.len);
+			if (len < 0) {
+				io->error = errno;
+				close(io->fd);
+				io->fd = -1;
+			} else {
+				io->u.out.buf += len;
+				io->u.out.len -= len;
+				if (!io->u.out.len) {
+					close(io->fd);
+					io->fd = -1;
+				}
+			}
+		}
+
+		if (io->type == POLLIN) {
+			ssize_t len = strbuf_read_once(io->u.in.buf,
+						       io->fd, io->u.in.hint);
+			if (len < 0)
+				io->error = errno;
+			if (len <= 0) {
+				close(io->fd);
+				io->fd = -1;
+			}
+		}
+	}
+
+	return 1;
+}
+
+int pump_io(struct io_pump *slots, int nr)
+{
+	struct pollfd *pfd;
+	int i;
+
+	for (i = 0; i < nr; i++)
+		slots[i].error = 0;
+
+	ALLOC_ARRAY(pfd, nr);
+	while (pump_io_round(slots, nr, pfd))
+		; /* nothing */
+	free(pfd);
+
+	/* There may be multiple errno values, so just pick the first. */
+	for (i = 0; i < nr; i++) {
+		if (slots[i].error) {
+			errno = slots[i].error;
+			return -1;
+		}
+	}
+	return 0;
+}
+
+
+int pipe_command(struct child_process *cmd,
+		 const char *in, size_t in_len,
+		 struct strbuf *out, size_t out_hint,
+		 struct strbuf *err, size_t err_hint)
 {
-	cmd->out = -1;
+	struct io_pump io[3];
+	int nr = 0;
+
+	if (in)
+		cmd->in = -1;
+	if (out)
+		cmd->out = -1;
+	if (err)
+		cmd->err = -1;
+
 	if (start_command(cmd) < 0)
 		return -1;
 
-	if (strbuf_read(buf, cmd->out, hint) < 0) {
-		close(cmd->out);
+	if (in) {
+		io[nr].fd = cmd->in;
+		io[nr].type = POLLOUT;
+		io[nr].u.out.buf = in;
+		io[nr].u.out.len = in_len;
+		nr++;
+	}
+	if (out) {
+		io[nr].fd = cmd->out;
+		io[nr].type = POLLIN;
+		io[nr].u.in.buf = out;
+		io[nr].u.in.hint = out_hint;
+		nr++;
+	}
+	if (err) {
+		io[nr].fd = cmd->err;
+		io[nr].type = POLLIN;
+		io[nr].u.in.buf = err;
+		io[nr].u.in.hint = err_hint;
+		nr++;
+	}
+
+	if (pump_io(io, nr) < 0) {
 		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 11f76b0..8c87654 100644
--- a/run-command.h
+++ b/run-command.h
@@ -79,17 +79,34 @@ 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.
+ * Execute the given command, sending "in" to its stdin, and capturing its
+ * stdout and stderr in the "out" and "err" strbufs. Any of the three may
+ * be NULL to skip processing.
+ *
  * 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.
+ * returns the exit code of the command. Any output collected in the
+ * buffers is kept even if the command returns a non-zero exit. The hint fields
+ * gives starting sizes for the strbuf allocations.
  *
  * 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.
+ * invocation. But note that there is no need to set the in, out, or err
+ * fields; capture_command handles that automatically.
+ */
+int pipe_command(struct child_process *cmd,
+		 const char *in, size_t in_len,
+		 struct strbuf *out, size_t out_hint,
+		 struct strbuf *err, size_t err_hint);
+
+/**
+ * Convenience wrapper around pipe_command for the common case
+ * of capturing only stdout.
  */
-int capture_command(struct child_process *cmd, struct strbuf *buf, size_t hint);
+static inline int capture_command(struct child_process *cmd,
+				  struct strbuf *out,
+				  size_t hint)
+{
+	return pipe_command(cmd, NULL, 0, out, hint, NULL, 0);
+}
 
 /*
  * The purpose of the following functions is to feed a pipe by running
-- 
2.9.0.160.g4984cba

Re: [PATCH 4/7] run-command: add pipe_command helper

[Eric Sunshine]

On Thu, Jun 16, 2016 at 5:37 AM, Jeff King <peff@peff.net> wrote:
> We already have capture_command(), which captures the stdout
> of a command in a way that avoids deadlocks. But sometimes
> we need to do more I/O, like capturing stderr as well, or
> sending data to stdin. It's easy to write code that
> deadlocks racily in these situations depending on how fast
> the command reads its input, or in which order it writes its
> output.
>
> Let's give callers an easy interface for doing this the
> right way, similar to what capture_command() did for the
> simple case.
>
> The whole thing is backed by a generic poll() loop that can
> feed an arbitrary number of buffers to descriptors, and fill
> an arbitrary number of strbufs from other descriptors. This
> seems like overkill, but the resulting code is actually a
> bit cleaner than just handling the three descriptors
> (because the output code for stdout/stderr is effectively
> duplicated, so being able to loop is a benefit).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> diff --git a/run-command.h b/run-command.h
> @@ -79,17 +79,34 @@ int run_command_v_opt(const char **argv, int opt);
>  /**
> - * Execute the given command, capturing its stdout in the given strbuf.
> + * Execute the given command, sending "in" to its stdin, and capturing its
> + * stdout and stderr in the "out" and "err" strbufs. Any of the three may
> + * be NULL to skip processing.
> + *
>   * 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.
> + * returns the exit code of the command. Any output collected in the

Did you mean s/returns/Returns/ ?

> + * buffers is kept even if the command returns a non-zero exit. The hint fields
> + * gives starting sizes for the strbuf allocations.
>   *
>   * 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.
> + * invocation. But note that there is no need to set the in, out, or err
> + * fields; capture_command handles that automatically.

s/capture_command/pipe_command/

> + */
> +int pipe_command(struct child_process *cmd,
> +                const char *in, size_t in_len,
> +                struct strbuf *out, size_t out_hint,
> +                struct strbuf *err, size_t err_hint);
> +

Re: [PATCH 4/7] run-command: add pipe_command helper

[Jeff King]

On Fri, Jun 17, 2016 at 04:03:18PM -0400, Eric Sunshine wrote:

> > diff --git a/run-command.h b/run-command.h
> > @@ -79,17 +79,34 @@ int run_command_v_opt(const char **argv, int opt);
> >  /**
> > - * Execute the given command, capturing its stdout in the given strbuf.
> > + * Execute the given command, sending "in" to its stdin, and capturing its
> > + * stdout and stderr in the "out" and "err" strbufs. Any of the three may
> > + * be NULL to skip processing.
> > + *
> >   * 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.
> > + * returns the exit code of the command. Any output collected in the
> 
> Did you mean s/returns/Returns/ ?

I don't think so. This is a continuatino of "...and otherwise" from the
context line above.

> > + * buffers is kept even if the command returns a non-zero exit. The hint fields
> > + * gives starting sizes for the strbuf allocations.
> >   *
> >   * 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.
> > + * invocation. But note that there is no need to set the in, out, or err
> > + * fields; capture_command handles that automatically.
> 
> s/capture_command/pipe_command/

Whoops, yes.

-Peff

Re: [PATCH 4/7] run-command: add pipe_command helper

[Eric Sunshine]

On Fri, Jun 17, 2016 at 7:28 PM, Jeff King <peff@peff.net> wrote:
> On Fri, Jun 17, 2016 at 04:03:18PM -0400, Eric Sunshine wrote:
>> > diff --git a/run-command.h b/run-command.h
>> > @@ -79,17 +79,34 @@ int run_command_v_opt(const char **argv, int opt);
>> >  /**
>> > - * Execute the given command, capturing its stdout in the given strbuf.
>> > + * Execute the given command, sending "in" to its stdin, and capturing its
>> > + * stdout and stderr in the "out" and "err" strbufs. Any of the three may
>> > + * be NULL to skip processing.
>> > + *
>> >   * 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.
>> > + * returns the exit code of the command. Any output collected in the
>>
>> Did you mean s/returns/Returns/ ?
>
> I don't think so. This is a continuatino of "...and otherwise" from the
> context line above.

Indeed. I somehow imagined a leading '-' on the line containing
"...and otherwise".

[PATCH 5/7] verify_signed_buffer: use pipe_command

[Jeff King]

This is shorter and should make the function easier to
follow. But more importantly, it removes the possibility of
any deadlocks based on reading or writing to gpg.

It's not clear if such a deadlock is possible in practice.

We do write the whole payload before reading anything, so we
could deadlock there. However, in practice gpg will need to
read our whole input to verify the signature, so it will
drain our payload first. It could write an error to stderr
before reading, but it's unlikely that such an error
wouldn't be followed by it immediately exiting, or that the
error would actually be larger than a pipe buffer.

On the writing side, we drain stderr (with the
human-readable output) in its entirety before reading stdout
(with the status-fd data). Running strace on "gpg --verify"
does show interleaved output on the two descriptors:

  write(2, "gpg: ", 5)                    = 5
  write(2, "Signature made Thu 16 Jun 2016 0"..., 73) = 73
  write(1, "[GNUPG:] SIG_ID tQw8KGcs9rBfLvAj"..., 66) = 66
  write(1, "[GNUPG:] GOODSIG 69808639F9430ED"..., 60) = 60
  write(2, "gpg: ", 5)                    = 5
  write(2, "Good signature from \"Jeff King <"..., 47) = 47
  write(2, "\n", 1)                       = 1
  write(2, "gpg: ", 5)                    = 5
  write(2, "                aka \"Jeff King <"..., 49) = 49
  write(2, "\n", 1)                       = 1
  write(1, "[GNUPG:] VALIDSIG C49CE24156AF08"..., 135) = 135
  write(1, "[GNUPG:] TRUST_ULTIMATE\n", 24) = 24

The second line written to stdout there contains the
signer's UID, which can be arbitrarily long. If it fills the
pipe buffer, then gpg would block writing to its stdout,
while we are blocked trying to read its stderr.

In practice, GPG seems to limit UIDs to 2048 bytes, so
unless your pipe buffer size is quite small, or unless gpg
does not enforce the limit under some conditions, this seems
unlikely in practice.

Still, it is not hard for us to be cautious and just use
pipe_command.

Signed-off-by: Jeff King <peff@peff.net>
---
 gpg-interface.c | 22 +++-------------------
 1 file changed, 3 insertions(+), 19 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index 854c1e5..c98035d 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -229,29 +229,13 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
 			 "--status-fd=1",
 			 "--verify", temp.filename.buf, "-",
 			 NULL);
-	gpg.in = -1;
-	gpg.out = -1;
-	if (gpg_output)
-		gpg.err = -1;
-	if (start_command(&gpg)) {
-		delete_tempfile(&temp);
-		return error(_("could not run gpg."));
-	}
 
-	sigchain_push(SIGPIPE, SIG_IGN);
-	write_in_full(gpg.in, payload, payload_size);
-	close(gpg.in);
-
-	if (gpg_output) {
-		strbuf_read(gpg_output, gpg.err, 0);
-		close(gpg.err);
-	}
 	if (!gpg_status)
 		gpg_status = &buf;
-	strbuf_read(gpg_status, gpg.out, 0);
-	close(gpg.out);
 
-	ret = finish_command(&gpg);
+	sigchain_push(SIGPIPE, SIG_IGN);
+	ret = pipe_command(&gpg, payload, payload_size,
+			   gpg_status, 0, gpg_output, 0);
 	sigchain_pop(SIGPIPE);
 
 	delete_tempfile(&temp);
-- 
2.9.0.160.g4984cba

[PATCH 6/7] sign_buffer: use pipe_command

[Jeff King]

Similar to the prior commit for verify_signed_buffer, the
motivation here is both to make the code simpler, and to
avoid any possible deadlocks with gpg.

In this case we have the same "write to stdin, then read
from stdout" that the verify case had. This is unlikely to
be a problem in practice, since stdout has the detached
signature, which it cannot compute until it has read all of
stdin (if it were a non-detached signature, that would be a
problem, though).

We don't read from stderr at all currently. However, we will
want to in a future patch, so this also prepares us there
(and in that case gpg _does_ write before reading all of the
input, though again, it is unlikely that a key uid will fill
up a pipe buffer).

Signed-off-by: Jeff King <peff@peff.net>
---
 gpg-interface.c | 24 +++++-------------------
 1 file changed, 5 insertions(+), 19 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index c98035d..74f08a2 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -151,40 +151,26 @@ const char *get_signing_key(void)
 int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key)
 {
 	struct child_process gpg = CHILD_PROCESS_INIT;
-	ssize_t len;
+	int ret;
 	size_t i, j, bottom;
 
-	gpg.in = -1;
-	gpg.out = -1;
 	argv_array_pushl(&gpg.args,
 			 gpg_program,
 			 "-bsau", signing_key,
 			 NULL);
 
-	if (start_command(&gpg))
-		return error(_("could not run gpg."));
+	bottom = signature->len;
 
 	/*
 	 * When the username signingkey is bad, program could be terminated
 	 * because gpg exits without reading and then write gets SIGPIPE.
 	 */
 	sigchain_push(SIGPIPE, SIG_IGN);
-
-	if (write_in_full(gpg.in, buffer->buf, buffer->len) != buffer->len) {
-		close(gpg.in);
-		close(gpg.out);
-		finish_command(&gpg);
-		return error(_("gpg did not accept the data"));
-	}
-	close(gpg.in);
-
-	bottom = signature->len;
-	len = strbuf_read(signature, gpg.out, 1024);
-	close(gpg.out);
-
+	ret = pipe_command(&gpg, buffer->buf, buffer->len,
+			   signature, 1024, NULL, 0);
 	sigchain_pop(SIGPIPE);
 
-	if (finish_command(&gpg) || !len || len < 0)
+	if (ret || signature->len == bottom)
 		return error(_("gpg failed to sign the data"));
 
 	/* Strip CR from the line endings, in case we are on Windows. */
-- 
2.9.0.160.g4984cba

[PATCH 7/7] gpg-interface: check gpg signature creation status

[Jeff King]

When we create a signature, it may happen that gpg returns with
"success" but not with an actual detached signature on stdout.

Check for the correct signature creation status to catch these cases
better. Really, --status-fd parsing is the only way to check gpg status
reliably. We do the same for verify already.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
Signed-off-by: Jeff King <peff@peff.net>
---
Obviously the big change from your v3 is that this just passes the
buffer to pipe_command() instead of doing the read itself.

I changed the name of your "err" strbuf to "gpg_status" to be more
descriptive, and to match the similar usage on the verification side.

In the test, I bumped the config-setting inside the test, and used
test_config to get the automatic cleanup (using "git -c" would also have
worked).

I left the test description as-is, though it is a bit misleading. We are
not really checking for bad input anymore, but rather "did gpg send us
the special stats". Your fake "echo" fails on both counts, so the test
is still effective. A more robust test would be to output something that
_looked_ like real gpg output, but not to output the correct status
token.


 gpg-interface.c | 8 ++++++--
 t/t7004-tag.sh  | 9 ++++++++-
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index 74f08a2..08356f9 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -153,9 +153,11 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig
 	struct child_process gpg = CHILD_PROCESS_INIT;
 	int ret;
 	size_t i, j, bottom;
+	struct strbuf gpg_status = STRBUF_INIT;
 
 	argv_array_pushl(&gpg.args,
 			 gpg_program,
+			 "--status-fd=2",
 			 "-bsau", signing_key,
 			 NULL);
 
@@ -167,10 +169,12 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig
 	 */
 	sigchain_push(SIGPIPE, SIG_IGN);
 	ret = pipe_command(&gpg, buffer->buf, buffer->len,
-			   signature, 1024, NULL, 0);
+			   signature, 1024, &gpg_status, 0);
 	sigchain_pop(SIGPIPE);
 
-	if (ret || signature->len == bottom)
+	ret |= !strstr(gpg_status.buf, "\n[GNUPG:] SIG_CREATED ");
+	strbuf_release(&gpg_status);
+	if (ret)
 		return error(_("gpg failed to sign the data"));
 
 	/* Strip CR from the line endings, in case we are on Windows. */
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index f9b7d79..8b0f71a 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1202,10 +1202,17 @@ test_expect_success GPG,RFC1991 \
 # try to sign with bad user.signingkey
 git config user.signingkey BobTheMouse
 test_expect_success GPG \
-	'git tag -s fails if gpg is misconfigured' \
+	'git tag -s fails if gpg is misconfigured (bad key)' \
 	'test_must_fail git tag -s -m tail tag-gpg-failure'
 git config --unset user.signingkey
 
+# try to produce invalid signature
+test_expect_success GPG \
+	'git tag -s fails if gpg is misconfigured (bad signature format)' \
+	'test_config gpg.program echo &&
+	 test_must_fail git tag -s -m tail tag-gpg-failure'
+
+
 # try to verify without gpg:
 
 rm -rf gpghome
-- 
2.9.0.160.g4984cba

Re: [PATCH 0/7] gpg-interface cleanups

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> This started off with Michael's patch to sign_buffer, which is at the
> tip, and then me trying to address the possible deadlocks there and in
> verify_signed_buffer. While I was in the area, I took the opportunity to
> do a few cleanups.
>
> It's unclear to me whether the deadlocks are possible in practice; see
> patch 5 for discussion.

I do recall thinking about the verification side and coming up with
the same conclusion as yours when we queued that code (i.e. they
need to read the whole thing before checking).

> My guess is probably not, but the amount of code
> to support doing it right is not all that much. But if we don't like it,
> we can drop 4-6.

Let's keep all of them; they all looked reasonable.

> Patch 7 is still authored by Michael, but has been massaged greatly by
> me. I'll comment more directly on the changes there.
>
>   [1/7]: gpg-interface: use child_process.args
>   [2/7]: verify_signed_buffer: drop pbuf variable
>   [3/7]: verify_signed_buffer: use tempfile object
>   [4/7]: run-command: add pipe_command helper
>   [5/7]: verify_signed_buffer: use pipe_command
>   [6/7]: sign_buffer: use pipe_command
>   [7/7]: gpg-interface: check gpg signature creation status

Thanks.

Re: [PATCH 0/7] gpg-interface cleanups

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 16.06.2016 20:20:
> Jeff King <peff@peff.net> writes:
> 
>> This started off with Michael's patch to sign_buffer, which is at the
>> tip, and then me trying to address the possible deadlocks there and in
>> verify_signed_buffer. While I was in the area, I took the opportunity to
>> do a few cleanups.
>>
>> It's unclear to me whether the deadlocks are possible in practice; see
>> patch 5 for discussion.
> 
> I do recall thinking about the verification side and coming up with
> the same conclusion as yours when we queued that code (i.e. they
> need to read the whole thing before checking).
> 
>> My guess is probably not, but the amount of code
>> to support doing it right is not all that much. But if we don't like it,
>> we can drop 4-6.
> 
> Let's keep all of them; they all looked reasonable.
> 
>> Patch 7 is still authored by Michael, but has been massaged greatly by
>> me. I'll comment more directly on the changes there.
>>
>>   [1/7]: gpg-interface: use child_process.args
>>   [2/7]: verify_signed_buffer: drop pbuf variable
>>   [3/7]: verify_signed_buffer: use tempfile object
>>   [4/7]: run-command: add pipe_command helper
>>   [5/7]: verify_signed_buffer: use pipe_command
>>   [6/7]: sign_buffer: use pipe_command
>>   [7/7]: gpg-interface: check gpg signature creation status
> 
> Thanks.

Thanks, too. I approve of and enjoyed all of Jeff's massaging.

(With tests, I'm always unsure whether to follow surrounding style or
the style guide; I did the former, Jeff improved the config handling by
doing the latter.)

Michael

Re: [PATCH 0/7] gpg-interface cleanups

[Jeff King]

On Fri, Jun 17, 2016 at 11:20:09AM +0200, Michael J Gruber wrote:

> (With tests, I'm always unsure whether to follow surrounding style or
> the style guide; I did the former, Jeff improved the config handling by
> doing the latter.)

Yes, but I kept the horrendous indentation as a nod to consistency. :)

-Peff

[PATCH v2 0/7] gpg-interface cleanups

[Jeff King]

On Thu, Jun 16, 2016 at 05:32:48AM -0400, Jeff King wrote:

>   [1/7]: gpg-interface: use child_process.args
>   [2/7]: verify_signed_buffer: drop pbuf variable
>   [3/7]: verify_signed_buffer: use tempfile object
>   [4/7]: run-command: add pipe_command helper
>   [5/7]: verify_signed_buffer: use pipe_command
>   [6/7]: sign_buffer: use pipe_command
>   [7/7]: gpg-interface: check gpg signature creation status

Here's a re-roll, fixing a few things in 4/7:

  - s/capture/pipe/ in the pipe_command docstring, from Eric

  - make internal pump_io functions static, from Ramsay

and more importantly:

  - I screwed up the in-body "From:" in the final patch; Michael should
    be the author

-Peff

[PATCH v2 1/7] gpg-interface: use child_process.args

[Jeff King]

Our argv allocations are relatively straightforward, but
this avoids us having to manually keep the count up to date
(or create new to-be-replaced slots in the declaration) when
we add new arguments.

Signed-off-by: Jeff King <peff@peff.net>
---
 gpg-interface.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index c4b1e8c..0ed9fa7 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -150,17 +150,15 @@ const char *get_signing_key(void)
 int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key)
 {
 	struct child_process gpg = CHILD_PROCESS_INIT;
-	const char *args[4];
 	ssize_t len;
 	size_t i, j, bottom;
 
-	gpg.argv = args;
 	gpg.in = -1;
 	gpg.out = -1;
-	args[0] = gpg_program;
-	args[1] = "-bsau";
-	args[2] = signing_key;
-	args[3] = NULL;
+	argv_array_pushl(&gpg.args,
+			 gpg_program,
+			 "-bsau", signing_key,
+			 NULL);
 
 	if (start_command(&gpg))
 		return error(_("could not run gpg."));
@@ -210,13 +208,11 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
 			 struct strbuf *gpg_output, struct strbuf *gpg_status)
 {
 	struct child_process gpg = CHILD_PROCESS_INIT;
-	const char *args_gpg[] = {NULL, "--status-fd=1", "--verify", "FILE", "-", NULL};
 	char path[PATH_MAX];
 	int fd, ret;
 	struct strbuf buf = STRBUF_INIT;
 	struct strbuf *pbuf = &buf;
 
-	args_gpg[0] = gpg_program;
 	fd = git_mkstemp(path, PATH_MAX, ".git_vtag_tmpXXXXXX");
 	if (fd < 0)
 		return error_errno(_("could not create temporary file '%s'"), path);
@@ -224,12 +220,15 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
 		return error_errno(_("failed writing detached signature to '%s'"), path);
 	close(fd);
 
-	gpg.argv = args_gpg;
+	argv_array_pushl(&gpg.args,
+			 gpg_program,
+			 "--status-fd=1",
+			 "--verify", path, "-",
+			 NULL);
 	gpg.in = -1;
 	gpg.out = -1;
 	if (gpg_output)
 		gpg.err = -1;
-	args_gpg[3] = path;
 	if (start_command(&gpg)) {
 		unlink(path);
 		return error(_("could not run gpg."));
-- 
2.9.0.165.g4aacdc3

[PATCH v2 2/7] verify_signed_buffer: drop pbuf variable

[Jeff King]

If our caller gave us a non-NULL gpg_status parameter, we
write the gpg status into their strbuf. If they didn't, then
we write it to a temporary local strbuf (since we still need
to look at it).  The variable "pbuf" adds an extra layer of
indirection so that the rest of the function can just access
whichever is appropriate.

However, the name "pbuf" isn't very descriptive, and it's
easy to get confused about what is supposed to be in it
(especially because we are reading both "status" and
"output" from gpg).

Rather than give it a more descriptive name, we can just use
gpg_status as our indirection pointer. Either it points to
the caller's input, or we can point it directly to our
temporary buffer.

Signed-off-by: Jeff King <peff@peff.net>
---
 gpg-interface.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index 0ed9fa7..216cad8 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -211,7 +211,6 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
 	char path[PATH_MAX];
 	int fd, ret;
 	struct strbuf buf = STRBUF_INIT;
-	struct strbuf *pbuf = &buf;
 
 	fd = git_mkstemp(path, PATH_MAX, ".git_vtag_tmpXXXXXX");
 	if (fd < 0)
@@ -242,9 +241,9 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
 		strbuf_read(gpg_output, gpg.err, 0);
 		close(gpg.err);
 	}
-	if (gpg_status)
-		pbuf = gpg_status;
-	strbuf_read(pbuf, gpg.out, 0);
+	if (!gpg_status)
+		gpg_status = &buf;
+	strbuf_read(gpg_status, gpg.out, 0);
 	close(gpg.out);
 
 	ret = finish_command(&gpg);
@@ -252,7 +251,7 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
 
 	unlink_or_warn(path);
 
-	ret |= !strstr(pbuf->buf, "\n[GNUPG:] GOODSIG ");
+	ret |= !strstr(gpg_status->buf, "\n[GNUPG:] GOODSIG ");
 	strbuf_release(&buf); /* no matter it was used or not */
 
 	return ret;
-- 
2.9.0.165.g4aacdc3

[PATCH v2 3/7] verify_signed_buffer: use tempfile object

[Jeff King]

We use git_mkstemp to create a temporary file, and try to
clean it up in all exit paths from the function. But that
misses any cases where we die by signal, or by calling die()
in a sub-function. In addition, we missed one of the exit
paths.

Let's convert to using a tempfile object, which handles the
hard cases for us, and add the missing cleanup call. Note
that we would not simply want to rely on program exit to
catch our missed cleanup, as this function may be called
many times in a single program (for the same reason, we use
a static tempfile instead of heap-allocating a new one; that
gives an upper bound on our memory usage).

Signed-off-by: Jeff King <peff@peff.net>
---
 gpg-interface.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index 216cad8..854c1e5 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -3,6 +3,7 @@
 #include "strbuf.h"
 #include "gpg-interface.h"
 #include "sigchain.h"
+#include "tempfile.h"
 
 static char *configured_signing_key;
 static const char *gpg_program = "gpg";
@@ -208,28 +209,32 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
 			 struct strbuf *gpg_output, struct strbuf *gpg_status)
 {
 	struct child_process gpg = CHILD_PROCESS_INIT;
-	char path[PATH_MAX];
+	static struct tempfile temp;
 	int fd, ret;
 	struct strbuf buf = STRBUF_INIT;
 
-	fd = git_mkstemp(path, PATH_MAX, ".git_vtag_tmpXXXXXX");
+	fd = mks_tempfile_t(&temp, ".git_vtag_tmpXXXXXX");
 	if (fd < 0)
-		return error_errno(_("could not create temporary file '%s'"), path);
-	if (write_in_full(fd, signature, signature_size) < 0)
-		return error_errno(_("failed writing detached signature to '%s'"), path);
+		return error_errno(_("could not create temporary file"));
+	if (write_in_full(fd, signature, signature_size) < 0) {
+		error_errno(_("failed writing detached signature to '%s'"),
+			    temp.filename.buf);
+		delete_tempfile(&temp);
+		return -1;
+	}
 	close(fd);
 
 	argv_array_pushl(&gpg.args,
 			 gpg_program,
 			 "--status-fd=1",
-			 "--verify", path, "-",
+			 "--verify", temp.filename.buf, "-",
 			 NULL);
 	gpg.in = -1;
 	gpg.out = -1;
 	if (gpg_output)
 		gpg.err = -1;
 	if (start_command(&gpg)) {
-		unlink(path);
+		delete_tempfile(&temp);
 		return error(_("could not run gpg."));
 	}
 
@@ -249,7 +254,7 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
 	ret = finish_command(&gpg);
 	sigchain_pop(SIGPIPE);
 
-	unlink_or_warn(path);
+	delete_tempfile(&temp);
 
 	ret |= !strstr(gpg_status->buf, "\n[GNUPG:] GOODSIG ");
 	strbuf_release(&buf); /* no matter it was used or not */
-- 
2.9.0.165.g4aacdc3

[PATCH v2 4/7] run-command: add pipe_command helper

[Jeff King]

We already have capture_command(), which captures the stdout
of a command in a way that avoids deadlocks. But sometimes
we need to do more I/O, like capturing stderr as well, or
sending data to stdin. It's easy to write code that
deadlocks racily in these situations depending on how fast
the command reads its input, or in which order it writes its
output.

Let's give callers an easy interface for doing this the
right way, similar to what capture_command() did for the
simple case.

The whole thing is backed by a generic poll() loop that can
feed an arbitrary number of buffers to descriptors, and fill
an arbitrary number of strbufs from other descriptors. This
seems like overkill, but the resulting code is actually a
bit cleaner than just handling the three descriptors
(because the output code for stdout/stderr is effectively
duplicated, so being able to loop is a benefit).

Signed-off-by: Jeff King <peff@peff.net>
---
 run-command.c | 152 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 run-command.h |  31 +++++++++---
 2 files changed, 171 insertions(+), 12 deletions(-)

diff --git a/run-command.c b/run-command.c
index af0c8a1..33bc63a 100644
--- a/run-command.c
+++ b/run-command.c
@@ -864,19 +864,161 @@ 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)
+struct io_pump {
+	/* initialized by caller */
+	int fd;
+	int type; /* POLLOUT or POLLIN */
+	union {
+		struct {
+			const char *buf;
+			size_t len;
+		} out;
+		struct {
+			struct strbuf *buf;
+			size_t hint;
+		} in;
+	} u;
+
+	/* returned by pump_io */
+	int error; /* 0 for success, otherwise errno */
+
+	/* internal use */
+	struct pollfd *pfd;
+};
+
+static int pump_io_round(struct io_pump *slots, int nr, struct pollfd *pfd)
+{
+	int pollsize = 0;
+	int i;
+
+	for (i = 0; i < nr; i++) {
+		struct io_pump *io = &slots[i];
+		if (io->fd < 0)
+			continue;
+		pfd[pollsize].fd = io->fd;
+		pfd[pollsize].events = io->type;
+		io->pfd = &pfd[pollsize++];
+	}
+
+	if (!pollsize)
+		return 0;
+
+	if (poll(pfd, pollsize, -1) < 0) {
+		if (errno == EINTR)
+			return 1;
+		die_errno("poll failed");
+	}
+
+	for (i = 0; i < nr; i++) {
+		struct io_pump *io = &slots[i];
+
+		if (io->fd < 0)
+			continue;
+
+		if (!(io->pfd->revents & (POLLOUT|POLLIN|POLLHUP|POLLERR|POLLNVAL)))
+			continue;
+
+		if (io->type == POLLOUT) {
+			ssize_t len = xwrite(io->fd,
+					     io->u.out.buf, io->u.out.len);
+			if (len < 0) {
+				io->error = errno;
+				close(io->fd);
+				io->fd = -1;
+			} else {
+				io->u.out.buf += len;
+				io->u.out.len -= len;
+				if (!io->u.out.len) {
+					close(io->fd);
+					io->fd = -1;
+				}
+			}
+		}
+
+		if (io->type == POLLIN) {
+			ssize_t len = strbuf_read_once(io->u.in.buf,
+						       io->fd, io->u.in.hint);
+			if (len < 0)
+				io->error = errno;
+			if (len <= 0) {
+				close(io->fd);
+				io->fd = -1;
+			}
+		}
+	}
+
+	return 1;
+}
+
+static int pump_io(struct io_pump *slots, int nr)
+{
+	struct pollfd *pfd;
+	int i;
+
+	for (i = 0; i < nr; i++)
+		slots[i].error = 0;
+
+	ALLOC_ARRAY(pfd, nr);
+	while (pump_io_round(slots, nr, pfd))
+		; /* nothing */
+	free(pfd);
+
+	/* There may be multiple errno values, so just pick the first. */
+	for (i = 0; i < nr; i++) {
+		if (slots[i].error) {
+			errno = slots[i].error;
+			return -1;
+		}
+	}
+	return 0;
+}
+
+
+int pipe_command(struct child_process *cmd,
+		 const char *in, size_t in_len,
+		 struct strbuf *out, size_t out_hint,
+		 struct strbuf *err, size_t err_hint)
 {
-	cmd->out = -1;
+	struct io_pump io[3];
+	int nr = 0;
+
+	if (in)
+		cmd->in = -1;
+	if (out)
+		cmd->out = -1;
+	if (err)
+		cmd->err = -1;
+
 	if (start_command(cmd) < 0)
 		return -1;
 
-	if (strbuf_read(buf, cmd->out, hint) < 0) {
-		close(cmd->out);
+	if (in) {
+		io[nr].fd = cmd->in;
+		io[nr].type = POLLOUT;
+		io[nr].u.out.buf = in;
+		io[nr].u.out.len = in_len;
+		nr++;
+	}
+	if (out) {
+		io[nr].fd = cmd->out;
+		io[nr].type = POLLIN;
+		io[nr].u.in.buf = out;
+		io[nr].u.in.hint = out_hint;
+		nr++;
+	}
+	if (err) {
+		io[nr].fd = cmd->err;
+		io[nr].type = POLLIN;
+		io[nr].u.in.buf = err;
+		io[nr].u.in.hint = err_hint;
+		nr++;
+	}
+
+	if (pump_io(io, nr) < 0) {
 		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 11f76b0..5066649 100644
--- a/run-command.h
+++ b/run-command.h
@@ -79,17 +79,34 @@ 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.
+ * Execute the given command, sending "in" to its stdin, and capturing its
+ * stdout and stderr in the "out" and "err" strbufs. Any of the three may
+ * be NULL to skip processing.
+ *
  * 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.
+ * returns the exit code of the command. Any output collected in the
+ * buffers is kept even if the command returns a non-zero exit. The hint fields
+ * gives starting sizes for the strbuf allocations.
  *
  * 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.
+ * invocation. But note that there is no need to set the in, out, or err
+ * fields; pipe_command handles that automatically.
+ */
+int pipe_command(struct child_process *cmd,
+		 const char *in, size_t in_len,
+		 struct strbuf *out, size_t out_hint,
+		 struct strbuf *err, size_t err_hint);
+
+/**
+ * Convenience wrapper around pipe_command for the common case
+ * of capturing only stdout.
  */
-int capture_command(struct child_process *cmd, struct strbuf *buf, size_t hint);
+static inline int capture_command(struct child_process *cmd,
+				  struct strbuf *out,
+				  size_t hint)
+{
+	return pipe_command(cmd, NULL, 0, out, hint, NULL, 0);
+}
 
 /*
  * The purpose of the following functions is to feed a pipe by running
-- 
2.9.0.165.g4aacdc3

[PATCH v2 5/7] verify_signed_buffer: use pipe_command

[Jeff King]

This is shorter and should make the function easier to
follow. But more importantly, it removes the possibility of
any deadlocks based on reading or writing to gpg.

It's not clear if such a deadlock is possible in practice.

We do write the whole payload before reading anything, so we
could deadlock there. However, in practice gpg will need to
read our whole input to verify the signature, so it will
drain our payload first. It could write an error to stderr
before reading, but it's unlikely that such an error
wouldn't be followed by it immediately exiting, or that the
error would actually be larger than a pipe buffer.

On the writing side, we drain stderr (with the
human-readable output) in its entirety before reading stdout
(with the status-fd data). Running strace on "gpg --verify"
does show interleaved output on the two descriptors:

  write(2, "gpg: ", 5)                    = 5
  write(2, "Signature made Thu 16 Jun 2016 0"..., 73) = 73
  write(1, "[GNUPG:] SIG_ID tQw8KGcs9rBfLvAj"..., 66) = 66
  write(1, "[GNUPG:] GOODSIG 69808639F9430ED"..., 60) = 60
  write(2, "gpg: ", 5)                    = 5
  write(2, "Good signature from \"Jeff King <"..., 47) = 47
  write(2, "\n", 1)                       = 1
  write(2, "gpg: ", 5)                    = 5
  write(2, "                aka \"Jeff King <"..., 49) = 49
  write(2, "\n", 1)                       = 1
  write(1, "[GNUPG:] VALIDSIG C49CE24156AF08"..., 135) = 135
  write(1, "[GNUPG:] TRUST_ULTIMATE\n", 24) = 24

The second line written to stdout there contains the
signer's UID, which can be arbitrarily long. If it fills the
pipe buffer, then gpg would block writing to its stdout,
while we are blocked trying to read its stderr.

In practice, GPG seems to limit UIDs to 2048 bytes, so
unless your pipe buffer size is quite small, or unless gpg
does not enforce the limit under some conditions, this seems
unlikely in practice.

Still, it is not hard for us to be cautious and just use
pipe_command.

Signed-off-by: Jeff King <peff@peff.net>
---
 gpg-interface.c | 22 +++-------------------
 1 file changed, 3 insertions(+), 19 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index 854c1e5..c98035d 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -229,29 +229,13 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
 			 "--status-fd=1",
 			 "--verify", temp.filename.buf, "-",
 			 NULL);
-	gpg.in = -1;
-	gpg.out = -1;
-	if (gpg_output)
-		gpg.err = -1;
-	if (start_command(&gpg)) {
-		delete_tempfile(&temp);
-		return error(_("could not run gpg."));
-	}
 
-	sigchain_push(SIGPIPE, SIG_IGN);
-	write_in_full(gpg.in, payload, payload_size);
-	close(gpg.in);
-
-	if (gpg_output) {
-		strbuf_read(gpg_output, gpg.err, 0);
-		close(gpg.err);
-	}
 	if (!gpg_status)
 		gpg_status = &buf;
-	strbuf_read(gpg_status, gpg.out, 0);
-	close(gpg.out);
 
-	ret = finish_command(&gpg);
+	sigchain_push(SIGPIPE, SIG_IGN);
+	ret = pipe_command(&gpg, payload, payload_size,
+			   gpg_status, 0, gpg_output, 0);
 	sigchain_pop(SIGPIPE);
 
 	delete_tempfile(&temp);
-- 
2.9.0.165.g4aacdc3

[PATCH v2 6/7] sign_buffer: use pipe_command

[Jeff King]

Similar to the prior commit for verify_signed_buffer, the
motivation here is both to make the code simpler, and to
avoid any possible deadlocks with gpg.

In this case we have the same "write to stdin, then read
from stdout" that the verify case had. This is unlikely to
be a problem in practice, since stdout has the detached
signature, which it cannot compute until it has read all of
stdin (if it were a non-detached signature, that would be a
problem, though).

We don't read from stderr at all currently. However, we will
want to in a future patch, so this also prepares us there
(and in that case gpg _does_ write before reading all of the
input, though again, it is unlikely that a key uid will fill
up a pipe buffer).

Signed-off-by: Jeff King <peff@peff.net>
---
 gpg-interface.c | 24 +++++-------------------
 1 file changed, 5 insertions(+), 19 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index c98035d..74f08a2 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -151,40 +151,26 @@ const char *get_signing_key(void)
 int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key)
 {
 	struct child_process gpg = CHILD_PROCESS_INIT;
-	ssize_t len;
+	int ret;
 	size_t i, j, bottom;
 
-	gpg.in = -1;
-	gpg.out = -1;
 	argv_array_pushl(&gpg.args,
 			 gpg_program,
 			 "-bsau", signing_key,
 			 NULL);
 
-	if (start_command(&gpg))
-		return error(_("could not run gpg."));
+	bottom = signature->len;
 
 	/*
 	 * When the username signingkey is bad, program could be terminated
 	 * because gpg exits without reading and then write gets SIGPIPE.
 	 */
 	sigchain_push(SIGPIPE, SIG_IGN);
-
-	if (write_in_full(gpg.in, buffer->buf, buffer->len) != buffer->len) {
-		close(gpg.in);
-		close(gpg.out);
-		finish_command(&gpg);
-		return error(_("gpg did not accept the data"));
-	}
-	close(gpg.in);
-
-	bottom = signature->len;
-	len = strbuf_read(signature, gpg.out, 1024);
-	close(gpg.out);
-
+	ret = pipe_command(&gpg, buffer->buf, buffer->len,
+			   signature, 1024, NULL, 0);
 	sigchain_pop(SIGPIPE);
 
-	if (finish_command(&gpg) || !len || len < 0)
+	if (ret || signature->len == bottom)
 		return error(_("gpg failed to sign the data"));
 
 	/* Strip CR from the line endings, in case we are on Windows. */
-- 
2.9.0.165.g4aacdc3

[PATCH v2 7/7] gpg-interface: check gpg signature creation status

[Jeff King]

From: Michael J Gruber <git@drmicha.warpmail.net>

When we create a signature, it may happen that gpg returns with
"success" but not with an actual detached signature on stdout.

Check for the correct signature creation status to catch these cases
better. Really, --status-fd parsing is the only way to check gpg status
reliably. We do the same for verify already.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
Signed-off-by: Jeff King <peff@peff.net>
---
 gpg-interface.c | 8 ++++++--
 t/t7004-tag.sh  | 9 ++++++++-
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index 74f08a2..08356f9 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -153,9 +153,11 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig
 	struct child_process gpg = CHILD_PROCESS_INIT;
 	int ret;
 	size_t i, j, bottom;
+	struct strbuf gpg_status = STRBUF_INIT;
 
 	argv_array_pushl(&gpg.args,
 			 gpg_program,
+			 "--status-fd=2",
 			 "-bsau", signing_key,
 			 NULL);
 
@@ -167,10 +169,12 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig
 	 */
 	sigchain_push(SIGPIPE, SIG_IGN);
 	ret = pipe_command(&gpg, buffer->buf, buffer->len,
-			   signature, 1024, NULL, 0);
+			   signature, 1024, &gpg_status, 0);
 	sigchain_pop(SIGPIPE);
 
-	if (ret || signature->len == bottom)
+	ret |= !strstr(gpg_status.buf, "\n[GNUPG:] SIG_CREATED ");
+	strbuf_release(&gpg_status);
+	if (ret)
 		return error(_("gpg failed to sign the data"));
 
 	/* Strip CR from the line endings, in case we are on Windows. */
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index f9b7d79..8b0f71a 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1202,10 +1202,17 @@ test_expect_success GPG,RFC1991 \
 # try to sign with bad user.signingkey
 git config user.signingkey BobTheMouse
 test_expect_success GPG \
-	'git tag -s fails if gpg is misconfigured' \
+	'git tag -s fails if gpg is misconfigured (bad key)' \
 	'test_must_fail git tag -s -m tail tag-gpg-failure'
 git config --unset user.signingkey
 
+# try to produce invalid signature
+test_expect_success GPG \
+	'git tag -s fails if gpg is misconfigured (bad signature format)' \
+	'test_config gpg.program echo &&
+	 test_must_fail git tag -s -m tail tag-gpg-failure'
+
+
 # try to verify without gpg:
 
 rm -rf gpghome
-- 
2.9.0.165.g4aacdc3