Merge branch 'jk/gpg-interface-cleanup'
authorJunio C Hamano <gitster@pobox.com>
Wed, 6 Jul 2016 20:38:12 +0000 (13:38 -0700)
committerJunio C Hamano <gitster@pobox.com>
Wed, 6 Jul 2016 20:38:12 +0000 (13:38 -0700)
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

gpg-interface.c
run-command.c
run-command.h
t/t7004-tag.sh
index c4b1e8c78d396194753d8a7287678bc476f858d6..08356f92e7b39787deb145868b01bf4c2cf0102a 100644 (file)
@@ -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;
index af0c8a10df0ca06e900d26486fa6c1a86bb2ff7f..33bc63a1de8d41b52a0ec353cf80b0f2e65dc8fd 100644 (file)
@@ -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);
 }
 
index 11f76b04edf65fe6117604782c220e55b2d80661..50666497aeca8f4d3d403cfaeb0afeb350216a42 100644 (file)
@@ -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
index f9b7d79af571ab2c377edeb7d46c1605078a4e02..8b0f71a2ac15d65f977b0daa2a53ad47b64a043a 100755 (executable)
@@ -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