diff options
author | Junio C Hamano <gitster@pobox.com> | 2016-07-06 23:38:12 +0300 |
---|---|---|
committer | Junio C Hamano <gitster@pobox.com> | 2016-07-06 23:38:12 +0300 |
commit | ed0f7bdec93478e280c9faa2a8ffb13a183cda09 (patch) | |
tree | 9cf0d27c9613aacbb38339d009c0e961840d79b1 | |
parent | 1d77bed8b02d902f11310f578debe933b90791f6 (diff) | |
parent | efee9553a4f97b2ecd8f49be19606dd4cf7d9c28 (diff) |
Merge branch 'jk/gpg-interface-cleanup'
A new run-command API function pipe_command() is introduced to
sanely feed data to the standard input while capturing data from
the standard output and the standard error of an external process,
which is cumbersome to hand-roll correctly without deadlocking.
The codepath to sign data in a prepared buffer with GPG has been
updated to use this API to read from the status-fd to check for
errors (instead of relying on GPG's exit status).
* jk/gpg-interface-cleanup:
gpg-interface: check gpg signature creation status
sign_buffer: use pipe_command
verify_signed_buffer: use pipe_command
run-command: add pipe_command helper
verify_signed_buffer: use tempfile object
verify_signed_buffer: drop pbuf variable
gpg-interface: use child_process.args
-rw-r--r-- | gpg-interface.c | 93 | ||||
-rw-r--r-- | run-command.c | 152 | ||||
-rw-r--r-- | run-command.h | 31 | ||||
-rwxr-xr-x | t/t7004-tag.sh | 9 |
4 files changed, 214 insertions, 71 deletions
diff --git a/gpg-interface.c b/gpg-interface.c index c4b1e8c78d..08356f92e7 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"; @@ -150,42 +151,30 @@ 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; + int ret; size_t i, j, bottom; + struct strbuf gpg_status = STRBUF_INIT; - 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, + "--status-fd=2", + "-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, &gpg_status, 0); sigchain_pop(SIGPIPE); - if (finish_command(&gpg) || !len || len < 0) + 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. */ @@ -210,50 +199,38 @@ 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]; + static struct tempfile temp; 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"); + 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); - gpg.argv = args_gpg; - 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.")); - } + argv_array_pushl(&gpg.args, + gpg_program, + "--status-fd=1", + "--verify", temp.filename.buf, "-", + NULL); - sigchain_push(SIGPIPE, SIG_IGN); - write_in_full(gpg.in, payload, payload_size); - close(gpg.in); + if (!gpg_status) + gpg_status = &buf; - if (gpg_output) { - strbuf_read(gpg_output, gpg.err, 0); - close(gpg.err); - } - if (gpg_status) - pbuf = gpg_status; - strbuf_read(pbuf, 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); - unlink_or_warn(path); + delete_tempfile(&temp); - 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; 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) +{ + 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 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 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 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 |