From aedb5dc343be58c17dc974be401f6d2a9fa26d1e Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 17 Jun 2016 19:38:35 -0400 Subject: [PATCH 1/7] gpg-interface: use child_process.args 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 Signed-off-by: Junio C Hamano --- gpg-interface.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/gpg-interface.c b/gpg-interface.c index c4b1e8c78d..0ed9fa75ff 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.")); From c752fcc8e0df02c6b1bd4daec1d08f0f2bcca58a Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 17 Jun 2016 19:38:39 -0400 Subject: [PATCH 2/7] verify_signed_buffer: drop pbuf variable 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 Signed-off-by: Junio C Hamano --- gpg-interface.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/gpg-interface.c b/gpg-interface.c index 0ed9fa75ff..216cad85be 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; From 4322353bfb53a83e6657af50603d3521ee9d7d0c Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 17 Jun 2016 19:38:43 -0400 Subject: [PATCH 3/7] verify_signed_buffer: use tempfile object 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 Signed-off-by: Junio C Hamano --- gpg-interface.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/gpg-interface.c b/gpg-interface.c index 216cad85be..854c1e56a3 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 */ From 96335bcf4d64c29add3692fb41671190123cf44e Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 17 Jun 2016 19:38:47 -0400 Subject: [PATCH 4/7] run-command: add pipe_command helper 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 Signed-off-by: Junio C Hamano --- 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 af0c8a10df..33bc63a1de 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) { - cmd->out = -1; + 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) +{ + 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 11f76b04ed..50666497ae 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 capture_command(struct child_process *cmd, struct strbuf *buf, size_t hint); +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. + */ +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 From 0d2b664efd815a3f6432723adb41732d90cc9be1 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 17 Jun 2016 19:38:52 -0400 Subject: [PATCH 5/7] verify_signed_buffer: use pipe_command 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 Signed-off-by: Junio C Hamano --- gpg-interface.c | 22 +++------------------- 1 file changed, 3 insertions(+), 19 deletions(-) diff --git a/gpg-interface.c b/gpg-interface.c index 854c1e56a3..c98035dffa 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); From 0581b546419627d4e82f7df8b195fa207ef42f6a Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 17 Jun 2016 19:38:55 -0400 Subject: [PATCH 6/7] sign_buffer: use pipe_command 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 Signed-off-by: Junio C Hamano --- gpg-interface.c | 24 +++++------------------- 1 file changed, 5 insertions(+), 19 deletions(-) diff --git a/gpg-interface.c b/gpg-interface.c index c98035dffa..74f08a2a0e 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. */ From efee9553a4f97b2ecd8f49be19606dd4cf7d9c28 Mon Sep 17 00:00:00 2001 From: Michael J Gruber Date: Fri, 17 Jun 2016 19:38:59 -0400 Subject: [PATCH 7/7] gpg-interface: check gpg signature creation status 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 Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- 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 74f08a2a0e..08356f92e7 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 f9b7d79af5..8b0f71a2ac 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