{"thread":{"id":"42626","subject":"[PATCH 0/7] gpg-interface cleanups","startedAt":"2016-06-16T09:33:02Z","lastAt":"2016-06-17T23:39:06Z","messageCount":22,"participants":["Jeff King","Junio C Hamano","Michael J Gruber","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"289338","messageId":"20160616093248.GA15130@sigill.intra.peff.net","threadId":"42626","inReplyTo":null,"subject":"[PATCH 0/7] gpg-interface cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-16T09:32:48Z","receivedAt":"2016-06-16T09:33:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This started off with Michael's patch to sign_buffer, which is at the\ntip, and then me trying to address the possible deadlocks there and in\nverify_signed_buffer. While I was in the area, I took the opportunity to\ndo a few cleanups.\n\nIt's unclear to me whether the deadlocks are possible in practice; see\npatch 5 for discussion. My guess is probably not, but the amount of code\nto support doing it right is not all that much. But if we don't like it,\nwe can drop 4-6.\n\nPatch 7 is still authored by Michael, but has been massaged greatly by\nme. I'll comment more directly on the changes there.\n\n  [1/7]: gpg-interface: use child_process.args\n  [2/7]: verify_signed_buffer: drop pbuf variable\n  [3/7]: verify_signed_buffer: use tempfile object\n  [4/7]: run-command: add pipe_command helper\n  [5/7]: verify_signed_buffer: use pipe_command\n  [6/7]: sign_buffer: use pipe_command\n  [7/7]: gpg-interface: check gpg signature creation status\n"},{"id":"289339","messageId":"20160616093328.GA15851@sigill.intra.peff.net","threadId":"42626","inReplyTo":"20160616093248.GA15130@sigill.intra.peff.net","subject":"[PATCH 1/7] gpg-interface: use child_process.args","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-16T09:33:29Z","receivedAt":"2016-06-16T09:33:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Our argv allocations are relatively straightforward, but\nthis avoids us having to manually keep the count up to date\n(or create new to-be-replaced slots in the declaration) when\nwe add new arguments.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n gpg-interface.c | 19 +++++++++----------\n 1 file changed, 9 insertions(+), 10 deletions(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex c4b1e8c..0ed9fa7 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -150,17 +150,15 @@ const char *get_signing_key(void)\n int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key)\n {\n \tstruct child_process gpg = CHILD_PROCESS_INIT;\n-\tconst char *args[4];\n \tssize_t len;\n \tsize_t i, j, bottom;\n \n-\tgpg.argv = args;\n \tgpg.in = -1;\n \tgpg.out = -1;\n-\targs[0] = gpg_program;\n-\targs[1] = \"-bsau\";\n-\targs[2] = signing_key;\n-\targs[3] = NULL;\n+\targv_array_pushl(&gpg.args,\n+\t\t\t gpg_program,\n+\t\t\t \"-bsau\", signing_key,\n+\t\t\t NULL);\n \n \tif (start_command(&gpg))\n \t\treturn error(_(\"could not run gpg.\"));\n@@ -210,13 +208,11 @@ int verify_signed_buffer(const char *payload, size_t payload_size,\n \t\t\t struct strbuf *gpg_output, struct strbuf *gpg_status)\n {\n \tstruct child_process gpg = CHILD_PROCESS_INIT;\n-\tconst char *args_gpg[] = {NULL, \"--status-fd=1\", \"--verify\", \"FILE\", \"-\", NULL};\n \tchar path[PATH_MAX];\n \tint fd, ret;\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct strbuf *pbuf = &buf;\n \n-\targs_gpg[0] = gpg_program;\n \tfd = git_mkstemp(path, PATH_MAX, \".git_vtag_tmpXXXXXX\");\n \tif (fd < 0)\n \t\treturn error_errno(_(\"could not create temporary file '%s'\"), path);\n@@ -224,12 +220,15 @@ int verify_signed_buffer(const char *payload, size_t payload_size,\n \t\treturn error_errno(_(\"failed writing detached signature to '%s'\"), path);\n \tclose(fd);\n \n-\tgpg.argv = args_gpg;\n+\targv_array_pushl(&gpg.args,\n+\t\t\t gpg_program,\n+\t\t\t \"--status-fd=1\",\n+\t\t\t \"--verify\", path, \"-\",\n+\t\t\t NULL);\n \tgpg.in = -1;\n \tgpg.out = -1;\n \tif (gpg_output)\n \t\tgpg.err = -1;\n-\targs_gpg[3] = path;\n \tif (start_command(&gpg)) {\n \t\tunlink(path);\n \t\treturn error(_(\"could not run gpg.\"));\n-- \n2.9.0.160.g4984cba\n\n"},{"id":"289340","messageId":"20160616093406.GB15851@sigill.intra.peff.net","threadId":"42626","inReplyTo":"20160616093248.GA15130@sigill.intra.peff.net","subject":"[PATCH 2/7] verify_signed_buffer: drop pbuf variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-16T09:34:07Z","receivedAt":"2016-06-16T09:34:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If our caller gave us a non-NULL gpg_status parameter, we\nwrite the gpg status into their strbuf. If they didn't, then\nwe write it to a temporary local strbuf (since we still need\nto look at it).  The variable \"pbuf\" adds an extra layer of\nindirection so that the rest of the function can just access\nwhichever is appropriate.\n\nHowever, the name \"pbuf\" isn't very descriptive, and it's\neasy to get confused about what is supposed to be in it\n(especially because we are reading both \"status\" and\n\"output\" from gpg).\n\nRather than give it a more descriptive name, we can just use\ngpg_status as our indirection pointer. Either it points to\nthe caller's input, or we can point it directly to our\ntemporary buffer.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n gpg-interface.c | 9 ++++-----\n 1 file changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 0ed9fa7..216cad8 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -211,7 +211,6 @@ int verify_signed_buffer(const char *payload, size_t payload_size,\n \tchar path[PATH_MAX];\n \tint fd, ret;\n \tstruct strbuf buf = STRBUF_INIT;\n-\tstruct strbuf *pbuf = &buf;\n \n \tfd = git_mkstemp(path, PATH_MAX, \".git_vtag_tmpXXXXXX\");\n \tif (fd < 0)\n@@ -242,9 +241,9 @@ int verify_signed_buffer(const char *payload, size_t payload_size,\n \t\tstrbuf_read(gpg_output, gpg.err, 0);\n \t\tclose(gpg.err);\n \t}\n-\tif (gpg_status)\n-\t\tpbuf = gpg_status;\n-\tstrbuf_read(pbuf, gpg.out, 0);\n+\tif (!gpg_status)\n+\t\tgpg_status = &buf;\n+\tstrbuf_read(gpg_status, gpg.out, 0);\n \tclose(gpg.out);\n \n \tret = finish_command(&gpg);\n@@ -252,7 +251,7 @@ int verify_signed_buffer(const char *payload, size_t payload_size,\n \n \tunlink_or_warn(path);\n \n-\tret |= !strstr(pbuf->buf, \"\\n[GNUPG:] GOODSIG \");\n+\tret |= !strstr(gpg_status->buf, \"\\n[GNUPG:] GOODSIG \");\n \tstrbuf_release(&buf); /* no matter it was used or not */\n \n \treturn ret;\n-- \n2.9.0.160.g4984cba\n\n"},{"id":"289341","messageId":"20160616093501.GC15851@sigill.intra.peff.net","threadId":"42626","inReplyTo":"20160616093248.GA15130@sigill.intra.peff.net","subject":"[PATCH 3/7] verify_signed_buffer: use tempfile object","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-16T09:35:01Z","receivedAt":"2016-06-16T09:35:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We use git_mkstemp to create a temporary file, and try to\nclean it up in all exit paths from the function. But that\nmisses any cases where we die by signal, or by calling die()\nin a sub-function. In addition, we missed one of the exit\npaths.\n\nLet's convert to using a tempfile object, which handles the\nhard cases for us, and add the missing cleanup call. Note\nthat we would not simply want to rely on program exit to\ncatch our missed cleanup, as this function may be called\nmany times in a single program (for the same reason, we use\na static tempfile instead of heap-allocating a new one; that\ngives an upper bound on our memory usage).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n gpg-interface.c | 21 +++++++++++++--------\n 1 file changed, 13 insertions(+), 8 deletions(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 216cad8..854c1e5 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -3,6 +3,7 @@\n #include \"strbuf.h\"\n #include \"gpg-interface.h\"\n #include \"sigchain.h\"\n+#include \"tempfile.h\"\n \n static char *configured_signing_key;\n static const char *gpg_program = \"gpg\";\n@@ -208,28 +209,32 @@ int verify_signed_buffer(const char *payload, size_t payload_size,\n \t\t\t struct strbuf *gpg_output, struct strbuf *gpg_status)\n {\n \tstruct child_process gpg = CHILD_PROCESS_INIT;\n-\tchar path[PATH_MAX];\n+\tstatic struct tempfile temp;\n \tint fd, ret;\n \tstruct strbuf buf = STRBUF_INIT;\n \n-\tfd = git_mkstemp(path, PATH_MAX, \".git_vtag_tmpXXXXXX\");\n+\tfd = mks_tempfile_t(&temp, \".git_vtag_tmpXXXXXX\");\n \tif (fd < 0)\n-\t\treturn error_errno(_(\"could not create temporary file '%s'\"), path);\n-\tif (write_in_full(fd, signature, signature_size) < 0)\n-\t\treturn error_errno(_(\"failed writing detached signature to '%s'\"), path);\n+\t\treturn error_errno(_(\"could not create temporary file\"));\n+\tif (write_in_full(fd, signature, signature_size) < 0) {\n+\t\terror_errno(_(\"failed writing detached signature to '%s'\"),\n+\t\t\t    temp.filename.buf);\n+\t\tdelete_tempfile(&temp);\n+\t\treturn -1;\n+\t}\n \tclose(fd);\n \n \targv_array_pushl(&gpg.args,\n \t\t\t gpg_program,\n \t\t\t \"--status-fd=1\",\n-\t\t\t \"--verify\", path, \"-\",\n+\t\t\t \"--verify\", temp.filename.buf, \"-\",\n \t\t\t NULL);\n \tgpg.in = -1;\n \tgpg.out = -1;\n \tif (gpg_output)\n \t\tgpg.err = -1;\n \tif (start_command(&gpg)) {\n-\t\tunlink(path);\n+\t\tdelete_tempfile(&temp);\n \t\treturn error(_(\"could not run gpg.\"));\n \t}\n \n@@ -249,7 +254,7 @@ int verify_signed_buffer(const char *payload, size_t payload_size,\n \tret = finish_command(&gpg);\n \tsigchain_pop(SIGPIPE);\n \n-\tunlink_or_warn(path);\n+\tdelete_tempfile(&temp);\n \n \tret |= !strstr(gpg_status->buf, \"\\n[GNUPG:] GOODSIG \");\n \tstrbuf_release(&buf); /* no matter it was used or not */\n-- \n2.9.0.160.g4984cba\n\n"},{"id":"289342","messageId":"20160616093745.GD15851@sigill.intra.peff.net","threadId":"42626","inReplyTo":"20160616093248.GA15130@sigill.intra.peff.net","subject":"[PATCH 4/7] run-command: add pipe_command helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-16T09:37:46Z","receivedAt":"2016-06-16T09:37:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We already have capture_command(), which captures the stdout\nof a command in a way that avoids deadlocks. But sometimes\nwe need to do more I/O, like capturing stderr as well, or\nsending data to stdin. It's easy to write code that\ndeadlocks racily in these situations depending on how fast\nthe command reads its input, or in which order it writes its\noutput.\n\nLet's give callers an easy interface for doing this the\nright way, similar to what capture_command() did for the\nsimple case.\n\nThe whole thing is backed by a generic poll() loop that can\nfeed an arbitrary number of buffers to descriptors, and fill\nan arbitrary number of strbufs from other descriptors. This\nseems like overkill, but the resulting code is actually a\nbit cleaner than just handling the three descriptors\n(because the output code for stdout/stderr is effectively\nduplicated, so being able to loop is a benefit).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nIt's possible this could come in handy other places, too. I didn't look,\nbut I suspect pump_io() could back some of the parallel submodule stuff.\n\n run-command.c | 152 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--\n run-command.h |  31 +++++++++---\n 2 files changed, 171 insertions(+), 12 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex af0c8a1..609fa4c 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -864,19 +864,161 @@ int run_hook_le(const char *const *env, const char *name, ...)\n \treturn ret;\n }\n \n-int capture_command(struct child_process *cmd, struct strbuf *buf, size_t hint)\n+struct io_pump {\n+\t/* initialized by caller */\n+\tint fd;\n+\tint type; /* POLLOUT or POLLIN */\n+\tunion {\n+\t\tstruct {\n+\t\t\tconst char *buf;\n+\t\t\tsize_t len;\n+\t\t} out;\n+\t\tstruct {\n+\t\t\tstruct strbuf *buf;\n+\t\t\tsize_t hint;\n+\t\t} in;\n+\t} u;\n+\n+\t/* returned by pump_io */\n+\tint error; /* 0 for success, otherwise errno */\n+\n+\t/* internal use */\n+\tstruct pollfd *pfd;\n+};\n+\n+int pump_io_round(struct io_pump *slots, int nr, struct pollfd *pfd)\n+{\n+\tint pollsize = 0;\n+\tint i;\n+\n+\tfor (i = 0; i < nr; i++) {\n+\t\tstruct io_pump *io = &slots[i];\n+\t\tif (io->fd < 0)\n+\t\t\tcontinue;\n+\t\tpfd[pollsize].fd = io->fd;\n+\t\tpfd[pollsize].events = io->type;\n+\t\tio->pfd = &pfd[pollsize++];\n+\t}\n+\n+\tif (!pollsize)\n+\t\treturn 0;\n+\n+\tif (poll(pfd, pollsize, -1) < 0) {\n+\t\tif (errno == EINTR)\n+\t\t\treturn 1;\n+\t\tdie_errno(\"poll failed\");\n+\t}\n+\n+\tfor (i = 0; i < nr; i++) {\n+\t\tstruct io_pump *io = &slots[i];\n+\n+\t\tif (io->fd < 0)\n+\t\t\tcontinue;\n+\n+\t\tif (!(io->pfd->revents & (POLLOUT|POLLIN|POLLHUP|POLLERR|POLLNVAL)))\n+\t\t\tcontinue;\n+\n+\t\tif (io->type == POLLOUT) {\n+\t\t\tssize_t len = xwrite(io->fd,\n+\t\t\t\t\t     io->u.out.buf, io->u.out.len);\n+\t\t\tif (len < 0) {\n+\t\t\t\tio->error = errno;\n+\t\t\t\tclose(io->fd);\n+\t\t\t\tio->fd = -1;\n+\t\t\t} else {\n+\t\t\t\tio->u.out.buf += len;\n+\t\t\t\tio->u.out.len -= len;\n+\t\t\t\tif (!io->u.out.len) {\n+\t\t\t\t\tclose(io->fd);\n+\t\t\t\t\tio->fd = -1;\n+\t\t\t\t}\n+\t\t\t}\n+\t\t}\n+\n+\t\tif (io->type == POLLIN) {\n+\t\t\tssize_t len = strbuf_read_once(io->u.in.buf,\n+\t\t\t\t\t\t       io->fd, io->u.in.hint);\n+\t\t\tif (len < 0)\n+\t\t\t\tio->error = errno;\n+\t\t\tif (len <= 0) {\n+\t\t\t\tclose(io->fd);\n+\t\t\t\tio->fd = -1;\n+\t\t\t}\n+\t\t}\n+\t}\n+\n+\treturn 1;\n+}\n+\n+int pump_io(struct io_pump *slots, int nr)\n+{\n+\tstruct pollfd *pfd;\n+\tint i;\n+\n+\tfor (i = 0; i < nr; i++)\n+\t\tslots[i].error = 0;\n+\n+\tALLOC_ARRAY(pfd, nr);\n+\twhile (pump_io_round(slots, nr, pfd))\n+\t\t; /* nothing */\n+\tfree(pfd);\n+\n+\t/* There may be multiple errno values, so just pick the first. */\n+\tfor (i = 0; i < nr; i++) {\n+\t\tif (slots[i].error) {\n+\t\t\terrno = slots[i].error;\n+\t\t\treturn -1;\n+\t\t}\n+\t}\n+\treturn 0;\n+}\n+\n+\n+int pipe_command(struct child_process *cmd,\n+\t\t const char *in, size_t in_len,\n+\t\t struct strbuf *out, size_t out_hint,\n+\t\t struct strbuf *err, size_t err_hint)\n {\n-\tcmd->out = -1;\n+\tstruct io_pump io[3];\n+\tint nr = 0;\n+\n+\tif (in)\n+\t\tcmd->in = -1;\n+\tif (out)\n+\t\tcmd->out = -1;\n+\tif (err)\n+\t\tcmd->err = -1;\n+\n \tif (start_command(cmd) < 0)\n \t\treturn -1;\n \n-\tif (strbuf_read(buf, cmd->out, hint) < 0) {\n-\t\tclose(cmd->out);\n+\tif (in) {\n+\t\tio[nr].fd = cmd->in;\n+\t\tio[nr].type = POLLOUT;\n+\t\tio[nr].u.out.buf = in;\n+\t\tio[nr].u.out.len = in_len;\n+\t\tnr++;\n+\t}\n+\tif (out) {\n+\t\tio[nr].fd = cmd->out;\n+\t\tio[nr].type = POLLIN;\n+\t\tio[nr].u.in.buf = out;\n+\t\tio[nr].u.in.hint = out_hint;\n+\t\tnr++;\n+\t}\n+\tif (err) {\n+\t\tio[nr].fd = cmd->err;\n+\t\tio[nr].type = POLLIN;\n+\t\tio[nr].u.in.buf = err;\n+\t\tio[nr].u.in.hint = err_hint;\n+\t\tnr++;\n+\t}\n+\n+\tif (pump_io(io, nr) < 0) {\n \t\tfinish_command(cmd); /* throw away exit code */\n \t\treturn -1;\n \t}\n \n-\tclose(cmd->out);\n \treturn finish_command(cmd);\n }\n \ndiff --git a/run-command.h b/run-command.h\nindex 11f76b0..8c87654 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -79,17 +79,34 @@ int run_command_v_opt(const char **argv, int opt);\n int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const char *const *env);\n \n /**\n- * Execute the given command, capturing its stdout in the given strbuf.\n+ * Execute the given command, sending \"in\" to its stdin, and capturing its\n+ * stdout and stderr in the \"out\" and \"err\" strbufs. Any of the three may\n+ * be NULL to skip processing.\n+ *\n  * Returns -1 if starting the command fails or reading fails, and otherwise\n- * returns the exit code of the command. The output collected in the\n- * buffer is kept even if the command returns a non-zero exit. The hint field\n- * gives a starting size for the strbuf allocation.\n+ * returns the exit code of the command. Any output collected in the\n+ * buffers is kept even if the command returns a non-zero exit. The hint fields\n+ * gives starting sizes for the strbuf allocations.\n  *\n  * The fields of \"cmd\" should be set up as they would for a normal run_command\n- * invocation. But note that there is no need to set cmd->out; the function\n- * sets it up for the caller.\n+ * invocation. But note that there is no need to set the in, out, or err\n+ * fields; capture_command handles that automatically.\n+ */\n+int pipe_command(struct child_process *cmd,\n+\t\t const char *in, size_t in_len,\n+\t\t struct strbuf *out, size_t out_hint,\n+\t\t struct strbuf *err, size_t err_hint);\n+\n+/**\n+ * Convenience wrapper around pipe_command for the common case\n+ * of capturing only stdout.\n  */\n-int capture_command(struct child_process *cmd, struct strbuf *buf, size_t hint);\n+static inline int capture_command(struct child_process *cmd,\n+\t\t\t\t  struct strbuf *out,\n+\t\t\t\t  size_t hint)\n+{\n+\treturn pipe_command(cmd, NULL, 0, out, hint, NULL, 0);\n+}\n \n /*\n  * The purpose of the following functions is to feed a pipe by running\n-- \n2.9.0.160.g4984cba\n\n"},{"id":"289343","messageId":"20160616093852.GE15851@sigill.intra.peff.net","threadId":"42626","inReplyTo":"20160616093248.GA15130@sigill.intra.peff.net","subject":"[PATCH 5/7] verify_signed_buffer: use pipe_command","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-16T09:38:52Z","receivedAt":"2016-06-16T09:38:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is shorter and should make the function easier to\nfollow. But more importantly, it removes the possibility of\nany deadlocks based on reading or writing to gpg.\n\nIt's not clear if such a deadlock is possible in practice.\n\nWe do write the whole payload before reading anything, so we\ncould deadlock there. However, in practice gpg will need to\nread our whole input to verify the signature, so it will\ndrain our payload first. It could write an error to stderr\nbefore reading, but it's unlikely that such an error\nwouldn't be followed by it immediately exiting, or that the\nerror would actually be larger than a pipe buffer.\n\nOn the writing side, we drain stderr (with the\nhuman-readable output) in its entirety before reading stdout\n(with the status-fd data). Running strace on \"gpg --verify\"\ndoes show interleaved output on the two descriptors:\n\n  write(2, \"gpg: \", 5)                    = 5\n  write(2, \"Signature made Thu 16 Jun 2016 0\"..., 73) = 73\n  write(1, \"[GNUPG:] SIG_ID tQw8KGcs9rBfLvAj\"..., 66) = 66\n  write(1, \"[GNUPG:] GOODSIG 69808639F9430ED\"..., 60) = 60\n  write(2, \"gpg: \", 5)                    = 5\n  write(2, \"Good signature from \\\"Jeff King <\"..., 47) = 47\n  write(2, \"\\n\", 1)                       = 1\n  write(2, \"gpg: \", 5)                    = 5\n  write(2, \"                aka \\\"Jeff King <\"..., 49) = 49\n  write(2, \"\\n\", 1)                       = 1\n  write(1, \"[GNUPG:] VALIDSIG C49CE24156AF08\"..., 135) = 135\n  write(1, \"[GNUPG:] TRUST_ULTIMATE\\n\", 24) = 24\n\nThe second line written to stdout there contains the\nsigner's UID, which can be arbitrarily long. If it fills the\npipe buffer, then gpg would block writing to its stdout,\nwhile we are blocked trying to read its stderr.\n\nIn practice, GPG seems to limit UIDs to 2048 bytes, so\nunless your pipe buffer size is quite small, or unless gpg\ndoes not enforce the limit under some conditions, this seems\nunlikely in practice.\n\nStill, it is not hard for us to be cautious and just use\npipe_command.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n gpg-interface.c | 22 +++-------------------\n 1 file changed, 3 insertions(+), 19 deletions(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 854c1e5..c98035d 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -229,29 +229,13 @@ int verify_signed_buffer(const char *payload, size_t payload_size,\n \t\t\t \"--status-fd=1\",\n \t\t\t \"--verify\", temp.filename.buf, \"-\",\n \t\t\t NULL);\n-\tgpg.in = -1;\n-\tgpg.out = -1;\n-\tif (gpg_output)\n-\t\tgpg.err = -1;\n-\tif (start_command(&gpg)) {\n-\t\tdelete_tempfile(&temp);\n-\t\treturn error(_(\"could not run gpg.\"));\n-\t}\n \n-\tsigchain_push(SIGPIPE, SIG_IGN);\n-\twrite_in_full(gpg.in, payload, payload_size);\n-\tclose(gpg.in);\n-\n-\tif (gpg_output) {\n-\t\tstrbuf_read(gpg_output, gpg.err, 0);\n-\t\tclose(gpg.err);\n-\t}\n \tif (!gpg_status)\n \t\tgpg_status = &buf;\n-\tstrbuf_read(gpg_status, gpg.out, 0);\n-\tclose(gpg.out);\n \n-\tret = finish_command(&gpg);\n+\tsigchain_push(SIGPIPE, SIG_IGN);\n+\tret = pipe_command(&gpg, payload, payload_size,\n+\t\t\t   gpg_status, 0, gpg_output, 0);\n \tsigchain_pop(SIGPIPE);\n \n \tdelete_tempfile(&temp);\n-- \n2.9.0.160.g4984cba\n\n"},{"id":"289344","messageId":"20160616093931.GF15851@sigill.intra.peff.net","threadId":"42626","inReplyTo":"20160616093248.GA15130@sigill.intra.peff.net","subject":"[PATCH 6/7] sign_buffer: use pipe_command","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-16T09:39:31Z","receivedAt":"2016-06-16T09:39:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Similar to the prior commit for verify_signed_buffer, the\nmotivation here is both to make the code simpler, and to\navoid any possible deadlocks with gpg.\n\nIn this case we have the same \"write to stdin, then read\nfrom stdout\" that the verify case had. This is unlikely to\nbe a problem in practice, since stdout has the detached\nsignature, which it cannot compute until it has read all of\nstdin (if it were a non-detached signature, that would be a\nproblem, though).\n\nWe don't read from stderr at all currently. However, we will\nwant to in a future patch, so this also prepares us there\n(and in that case gpg _does_ write before reading all of the\ninput, though again, it is unlikely that a key uid will fill\nup a pipe buffer).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n gpg-interface.c | 24 +++++-------------------\n 1 file changed, 5 insertions(+), 19 deletions(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex c98035d..74f08a2 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -151,40 +151,26 @@ const char *get_signing_key(void)\n int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key)\n {\n \tstruct child_process gpg = CHILD_PROCESS_INIT;\n-\tssize_t len;\n+\tint ret;\n \tsize_t i, j, bottom;\n \n-\tgpg.in = -1;\n-\tgpg.out = -1;\n \targv_array_pushl(&gpg.args,\n \t\t\t gpg_program,\n \t\t\t \"-bsau\", signing_key,\n \t\t\t NULL);\n \n-\tif (start_command(&gpg))\n-\t\treturn error(_(\"could not run gpg.\"));\n+\tbottom = signature->len;\n \n \t/*\n \t * When the username signingkey is bad, program could be terminated\n \t * because gpg exits without reading and then write gets SIGPIPE.\n \t */\n \tsigchain_push(SIGPIPE, SIG_IGN);\n-\n-\tif (write_in_full(gpg.in, buffer->buf, buffer->len) != buffer->len) {\n-\t\tclose(gpg.in);\n-\t\tclose(gpg.out);\n-\t\tfinish_command(&gpg);\n-\t\treturn error(_(\"gpg did not accept the data\"));\n-\t}\n-\tclose(gpg.in);\n-\n-\tbottom = signature->len;\n-\tlen = strbuf_read(signature, gpg.out, 1024);\n-\tclose(gpg.out);\n-\n+\tret = pipe_command(&gpg, buffer->buf, buffer->len,\n+\t\t\t   signature, 1024, NULL, 0);\n \tsigchain_pop(SIGPIPE);\n \n-\tif (finish_command(&gpg) || !len || len < 0)\n+\tif (ret || signature->len == bottom)\n \t\treturn error(_(\"gpg failed to sign the data\"));\n \n \t/* Strip CR from the line endings, in case we are on Windows. */\n-- \n2.9.0.160.g4984cba\n\n"},{"id":"289346","messageId":"20160616094239.GG15851@sigill.intra.peff.net","threadId":"42626","inReplyTo":"20160616093248.GA15130@sigill.intra.peff.net","subject":"[PATCH 7/7] gpg-interface: check gpg signature creation status","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-16T09:42:39Z","receivedAt":"2016-06-16T09:42:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we create a signature, it may happen that gpg returns with\n\"success\" but not with an actual detached signature on stdout.\n\nCheck for the correct signature creation status to catch these cases\nbetter. Really, --status-fd parsing is the only way to check gpg status\nreliably. We do the same for verify already.\n\nSigned-off-by: Michael J Gruber <git@drmicha.warpmail.net>\nSigned-off-by: Jeff King <peff@peff.net>\n---\nObviously the big change from your v3 is that this just passes the\nbuffer to pipe_command() instead of doing the read itself.\n\nI changed the name of your \"err\" strbuf to \"gpg_status\" to be more\ndescriptive, and to match the similar usage on the verification side.\n\nIn the test, I bumped the config-setting inside the test, and used\ntest_config to get the automatic cleanup (using \"git -c\" would also have\nworked).\n\nI left the test description as-is, though it is a bit misleading. We are\nnot really checking for bad input anymore, but rather \"did gpg send us\nthe special stats\". Your fake \"echo\" fails on both counts, so the test\nis still effective. A more robust test would be to output something that\n_looked_ like real gpg output, but not to output the correct status\ntoken.\n\n\n gpg-interface.c | 8 ++++++--\n t/t7004-tag.sh  | 9 ++++++++-\n 2 files changed, 14 insertions(+), 3 deletions(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 74f08a2..08356f9 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -153,9 +153,11 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig\n \tstruct child_process gpg = CHILD_PROCESS_INIT;\n \tint ret;\n \tsize_t i, j, bottom;\n+\tstruct strbuf gpg_status = STRBUF_INIT;\n \n \targv_array_pushl(&gpg.args,\n \t\t\t gpg_program,\n+\t\t\t \"--status-fd=2\",\n \t\t\t \"-bsau\", signing_key,\n \t\t\t NULL);\n \n@@ -167,10 +169,12 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig\n \t */\n \tsigchain_push(SIGPIPE, SIG_IGN);\n \tret = pipe_command(&gpg, buffer->buf, buffer->len,\n-\t\t\t   signature, 1024, NULL, 0);\n+\t\t\t   signature, 1024, &gpg_status, 0);\n \tsigchain_pop(SIGPIPE);\n \n-\tif (ret || signature->len == bottom)\n+\tret |= !strstr(gpg_status.buf, \"\\n[GNUPG:] SIG_CREATED \");\n+\tstrbuf_release(&gpg_status);\n+\tif (ret)\n \t\treturn error(_(\"gpg failed to sign the data\"));\n \n \t/* Strip CR from the line endings, in case we are on Windows. */\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex f9b7d79..8b0f71a 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1202,10 +1202,17 @@ test_expect_success GPG,RFC1991 \\\n # try to sign with bad user.signingkey\n git config user.signingkey BobTheMouse\n test_expect_success GPG \\\n-\t'git tag -s fails if gpg is misconfigured' \\\n+\t'git tag -s fails if gpg is misconfigured (bad key)' \\\n \t'test_must_fail git tag -s -m tail tag-gpg-failure'\n git config --unset user.signingkey\n \n+# try to produce invalid signature\n+test_expect_success GPG \\\n+\t'git tag -s fails if gpg is misconfigured (bad signature format)' \\\n+\t'test_config gpg.program echo &&\n+\t test_must_fail git tag -s -m tail tag-gpg-failure'\n+\n+\n # try to verify without gpg:\n \n rm -rf gpghome\n-- \n2.9.0.160.g4984cba\n"},{"id":"289379","messageId":"xmqqy4653si6.fsf@gitster.mtv.corp.google.com","threadId":"42626","inReplyTo":"20160616093248.GA15130@sigill.intra.peff.net","subject":"Re: [PATCH 0/7] gpg-interface cleanups","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-16T18:20:33Z","receivedAt":"2016-06-16T18:20:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> This started off with Michael's patch to sign_buffer, which is at the\n> tip, and then me trying to address the possible deadlocks there and in\n> verify_signed_buffer. While I was in the area, I took the opportunity to\n> do a few cleanups.\n>\n> It's unclear to me whether the deadlocks are possible in practice; see\n> patch 5 for discussion.\n\nI do recall thinking about the verification side and coming up with\nthe same conclusion as yours when we queued that code (i.e. they\nneed to read the whole thing before checking).\n\n> My guess is probably not, but the amount of code\n> to support doing it right is not all that much. But if we don't like it,\n> we can drop 4-6.\n\nLet's keep all of them; they all looked reasonable.\n\n> Patch 7 is still authored by Michael, but has been massaged greatly by\n> me. I'll comment more directly on the changes there.\n>\n>   [1/7]: gpg-interface: use child_process.args\n>   [2/7]: verify_signed_buffer: drop pbuf variable\n>   [3/7]: verify_signed_buffer: use tempfile object\n>   [4/7]: run-command: add pipe_command helper\n>   [5/7]: verify_signed_buffer: use pipe_command\n>   [6/7]: sign_buffer: use pipe_command\n>   [7/7]: gpg-interface: check gpg signature creation status\n\nThanks.\n"},{"id":"289431","messageId":"11a07248-ddf5-1424-2197-ca3ed117e200@drmicha.warpmail.net","threadId":"42626","inReplyTo":"xmqqy4653si6.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/7] gpg-interface cleanups","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2016-06-17T09:20:09Z","receivedAt":"2016-06-17T09:20:37Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 16.06.2016 20:20:\n> Jeff King <peff@peff.net> writes:\n> \n>> This started off with Michael's patch to sign_buffer, which is at the\n>> tip, and then me trying to address the possible deadlocks there and in\n>> verify_signed_buffer. While I was in the area, I took the opportunity to\n>> do a few cleanups.\n>>\n>> It's unclear to me whether the deadlocks are possible in practice; see\n>> patch 5 for discussion.\n> \n> I do recall thinking about the verification side and coming up with\n> the same conclusion as yours when we queued that code (i.e. they\n> need to read the whole thing before checking).\n> \n>> My guess is probably not, but the amount of code\n>> to support doing it right is not all that much. But if we don't like it,\n>> we can drop 4-6.\n> \n> Let's keep all of them; they all looked reasonable.\n> \n>> Patch 7 is still authored by Michael, but has been massaged greatly by\n>> me. I'll comment more directly on the changes there.\n>>\n>>   [1/7]: gpg-interface: use child_process.args\n>>   [2/7]: verify_signed_buffer: drop pbuf variable\n>>   [3/7]: verify_signed_buffer: use tempfile object\n>>   [4/7]: run-command: add pipe_command helper\n>>   [5/7]: verify_signed_buffer: use pipe_command\n>>   [6/7]: sign_buffer: use pipe_command\n>>   [7/7]: gpg-interface: check gpg signature creation status\n> \n> Thanks.\n\nThanks, too. I approve of and enjoyed all of Jeff's massaging.\n\n(With tests, I'm always unsure whether to follow surrounding style or\nthe style guide; I did the former, Jeff improved the config handling by\ndoing the latter.)\n\nMichael\n\n"},{"id":"289432","messageId":"20160617092134.GA25299@sigill.intra.peff.net","threadId":"42626","inReplyTo":"11a07248-ddf5-1424-2197-ca3ed117e200@drmicha.warpmail.net","subject":"Re: [PATCH 0/7] gpg-interface cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-17T09:21:35Z","receivedAt":"2016-06-17T09:21:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 17, 2016 at 11:20:09AM +0200, Michael J Gruber wrote:\n\n> (With tests, I'm always unsure whether to follow surrounding style or\n> the style guide; I did the former, Jeff improved the config handling by\n> doing the latter.)\n\nYes, but I kept the horrendous indentation as a nod to consistency. :)\n\n-Peff\n"},{"id":"289470","messageId":"CAPig+cR-Z+dpKN0dG_BG7JWRBZeo4Xtpa1HQAU1Tt3WRunsrXg@mail.gmail.com","threadId":"42626","inReplyTo":"20160616093745.GD15851@sigill.intra.peff.net","subject":"Re: [PATCH 4/7] run-command: add pipe_command helper","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-06-17T20:03:18Z","receivedAt":"2016-06-17T20:03:35Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Jun 16, 2016 at 5:37 AM, Jeff King <peff@peff.net> wrote:\n> We already have capture_command(), which captures the stdout\n> of a command in a way that avoids deadlocks. But sometimes\n> we need to do more I/O, like capturing stderr as well, or\n> sending data to stdin. It's easy to write code that\n> deadlocks racily in these situations depending on how fast\n> the command reads its input, or in which order it writes its\n> output.\n>\n> Let's give callers an easy interface for doing this the\n> right way, similar to what capture_command() did for the\n> simple case.\n>\n> The whole thing is backed by a generic poll() loop that can\n> feed an arbitrary number of buffers to descriptors, and fill\n> an arbitrary number of strbufs from other descriptors. This\n> seems like overkill, but the resulting code is actually a\n> bit cleaner than just handling the three descriptors\n> (because the output code for stdout/stderr is effectively\n> duplicated, so being able to loop is a benefit).\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> diff --git a/run-command.h b/run-command.h\n> @@ -79,17 +79,34 @@ int run_command_v_opt(const char **argv, int opt);\n>  /**\n> - * Execute the given command, capturing its stdout in the given strbuf.\n> + * Execute the given command, sending \"in\" to its stdin, and capturing its\n> + * stdout and stderr in the \"out\" and \"err\" strbufs. Any of the three may\n> + * be NULL to skip processing.\n> + *\n>   * Returns -1 if starting the command fails or reading fails, and otherwise\n> - * returns the exit code of the command. The output collected in the\n> - * buffer is kept even if the command returns a non-zero exit. The hint field\n> - * gives a starting size for the strbuf allocation.\n> + * returns the exit code of the command. Any output collected in the\n\nDid you mean s/returns/Returns/ ?\n\n> + * buffers is kept even if the command returns a non-zero exit. The hint fields\n> + * gives starting sizes for the strbuf allocations.\n>   *\n>   * The fields of \"cmd\" should be set up as they would for a normal run_command\n> - * invocation. But note that there is no need to set cmd->out; the function\n> - * sets it up for the caller.\n> + * invocation. But note that there is no need to set the in, out, or err\n> + * fields; capture_command handles that automatically.\n\ns/capture_command/pipe_command/\n\n> + */\n> +int pipe_command(struct child_process *cmd,\n> +                const char *in, size_t in_len,\n> +                struct strbuf *out, size_t out_hint,\n> +                struct strbuf *err, size_t err_hint);\n> +\n"},{"id":"289529","messageId":"20160617232857.GA26564@sigill.intra.peff.net","threadId":"42626","inReplyTo":"CAPig+cR-Z+dpKN0dG_BG7JWRBZeo4Xtpa1HQAU1Tt3WRunsrXg@mail.gmail.com","subject":"Re: [PATCH 4/7] run-command: add pipe_command helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-17T23:28:58Z","receivedAt":"2016-06-17T23:29:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 17, 2016 at 04:03:18PM -0400, Eric Sunshine wrote:\n\n> > diff --git a/run-command.h b/run-command.h\n> > @@ -79,17 +79,34 @@ int run_command_v_opt(const char **argv, int opt);\n> >  /**\n> > - * Execute the given command, capturing its stdout in the given strbuf.\n> > + * Execute the given command, sending \"in\" to its stdin, and capturing its\n> > + * stdout and stderr in the \"out\" and \"err\" strbufs. Any of the three may\n> > + * be NULL to skip processing.\n> > + *\n> >   * Returns -1 if starting the command fails or reading fails, and otherwise\n> > - * returns the exit code of the command. The output collected in the\n> > - * buffer is kept even if the command returns a non-zero exit. The hint field\n> > - * gives a starting size for the strbuf allocation.\n> > + * returns the exit code of the command. Any output collected in the\n> \n> Did you mean s/returns/Returns/ ?\n\nI don't think so. This is a continuatino of \"...and otherwise\" from the\ncontext line above.\n\n> > + * buffers is kept even if the command returns a non-zero exit. The hint fields\n> > + * gives starting sizes for the strbuf allocations.\n> >   *\n> >   * The fields of \"cmd\" should be set up as they would for a normal run_command\n> > - * invocation. But note that there is no need to set cmd->out; the function\n> > - * sets it up for the caller.\n> > + * invocation. But note that there is no need to set the in, out, or err\n> > + * fields; capture_command handles that automatically.\n> \n> s/capture_command/pipe_command/\n\nWhoops, yes.\n\n-Peff\n"},{"id":"289530","messageId":"CAPig+cQkFSDNHLXBPzycm98AROcBQYCwgffSC0ditrqXpd1NEA@mail.gmail.com","threadId":"42626","inReplyTo":"20160617232857.GA26564@sigill.intra.peff.net","subject":"Re: [PATCH 4/7] run-command: add pipe_command helper","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-06-17T23:31:07Z","receivedAt":"2016-06-17T23:31:13Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Jun 17, 2016 at 7:28 PM, Jeff King <peff@peff.net> wrote:\n> On Fri, Jun 17, 2016 at 04:03:18PM -0400, Eric Sunshine wrote:\n>> > diff --git a/run-command.h b/run-command.h\n>> > @@ -79,17 +79,34 @@ int run_command_v_opt(const char **argv, int opt);\n>> >  /**\n>> > - * Execute the given command, capturing its stdout in the given strbuf.\n>> > + * Execute the given command, sending \"in\" to its stdin, and capturing its\n>> > + * stdout and stderr in the \"out\" and \"err\" strbufs. Any of the three may\n>> > + * be NULL to skip processing.\n>> > + *\n>> >   * Returns -1 if starting the command fails or reading fails, and otherwise\n>> > - * returns the exit code of the command. The output collected in the\n>> > - * buffer is kept even if the command returns a non-zero exit. The hint field\n>> > - * gives a starting size for the strbuf allocation.\n>> > + * returns the exit code of the command. Any output collected in the\n>>\n>> Did you mean s/returns/Returns/ ?\n>\n> I don't think so. This is a continuatino of \"...and otherwise\" from the\n> context line above.\n\nIndeed. I somehow imagined a leading '-' on the line containing\n\"...and otherwise\".\n"},{"id":"289532","messageId":"20160617233819.GA31909@sigill.intra.peff.net","threadId":"42626","inReplyTo":"20160616093248.GA15130@sigill.intra.peff.net","subject":"[PATCH v2 0/7] gpg-interface cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-17T23:38:20Z","receivedAt":"2016-06-17T23:38:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 16, 2016 at 05:32:48AM -0400, Jeff King wrote:\n\n>   [1/7]: gpg-interface: use child_process.args\n>   [2/7]: verify_signed_buffer: drop pbuf variable\n>   [3/7]: verify_signed_buffer: use tempfile object\n>   [4/7]: run-command: add pipe_command helper\n>   [5/7]: verify_signed_buffer: use pipe_command\n>   [6/7]: sign_buffer: use pipe_command\n>   [7/7]: gpg-interface: check gpg signature creation status\n\nHere's a re-roll, fixing a few things in 4/7:\n\n  - s/capture/pipe/ in the pipe_command docstring, from Eric\n\n  - make internal pump_io functions static, from Ramsay\n\nand more importantly:\n\n  - I screwed up the in-body \"From:\" in the final patch; Michael should\n    be the author\n\n-Peff\n"},{"id":"289533","messageId":"20160617233835.GA31958@sigill.intra.peff.net","threadId":"42626","inReplyTo":"20160617233819.GA31909@sigill.intra.peff.net","subject":"[PATCH v2 1/7] gpg-interface: use child_process.args","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-17T23:38:35Z","receivedAt":"2016-06-17T23:38:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Our argv allocations are relatively straightforward, but\nthis avoids us having to manually keep the count up to date\n(or create new to-be-replaced slots in the declaration) when\nwe add new arguments.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n gpg-interface.c | 19 +++++++++----------\n 1 file changed, 9 insertions(+), 10 deletions(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex c4b1e8c..0ed9fa7 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -150,17 +150,15 @@ const char *get_signing_key(void)\n int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key)\n {\n \tstruct child_process gpg = CHILD_PROCESS_INIT;\n-\tconst char *args[4];\n \tssize_t len;\n \tsize_t i, j, bottom;\n \n-\tgpg.argv = args;\n \tgpg.in = -1;\n \tgpg.out = -1;\n-\targs[0] = gpg_program;\n-\targs[1] = \"-bsau\";\n-\targs[2] = signing_key;\n-\targs[3] = NULL;\n+\targv_array_pushl(&gpg.args,\n+\t\t\t gpg_program,\n+\t\t\t \"-bsau\", signing_key,\n+\t\t\t NULL);\n \n \tif (start_command(&gpg))\n \t\treturn error(_(\"could not run gpg.\"));\n@@ -210,13 +208,11 @@ int verify_signed_buffer(const char *payload, size_t payload_size,\n \t\t\t struct strbuf *gpg_output, struct strbuf *gpg_status)\n {\n \tstruct child_process gpg = CHILD_PROCESS_INIT;\n-\tconst char *args_gpg[] = {NULL, \"--status-fd=1\", \"--verify\", \"FILE\", \"-\", NULL};\n \tchar path[PATH_MAX];\n \tint fd, ret;\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct strbuf *pbuf = &buf;\n \n-\targs_gpg[0] = gpg_program;\n \tfd = git_mkstemp(path, PATH_MAX, \".git_vtag_tmpXXXXXX\");\n \tif (fd < 0)\n \t\treturn error_errno(_(\"could not create temporary file '%s'\"), path);\n@@ -224,12 +220,15 @@ int verify_signed_buffer(const char *payload, size_t payload_size,\n \t\treturn error_errno(_(\"failed writing detached signature to '%s'\"), path);\n \tclose(fd);\n \n-\tgpg.argv = args_gpg;\n+\targv_array_pushl(&gpg.args,\n+\t\t\t gpg_program,\n+\t\t\t \"--status-fd=1\",\n+\t\t\t \"--verify\", path, \"-\",\n+\t\t\t NULL);\n \tgpg.in = -1;\n \tgpg.out = -1;\n \tif (gpg_output)\n \t\tgpg.err = -1;\n-\targs_gpg[3] = path;\n \tif (start_command(&gpg)) {\n \t\tunlink(path);\n \t\treturn error(_(\"could not run gpg.\"));\n-- \n2.9.0.165.g4aacdc3\n\n"},{"id":"289534","messageId":"20160617233839.GB31958@sigill.intra.peff.net","threadId":"42626","inReplyTo":"20160617233819.GA31909@sigill.intra.peff.net","subject":"[PATCH v2 2/7] verify_signed_buffer: drop pbuf variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-17T23:38:39Z","receivedAt":"2016-06-17T23:38:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If our caller gave us a non-NULL gpg_status parameter, we\nwrite the gpg status into their strbuf. If they didn't, then\nwe write it to a temporary local strbuf (since we still need\nto look at it).  The variable \"pbuf\" adds an extra layer of\nindirection so that the rest of the function can just access\nwhichever is appropriate.\n\nHowever, the name \"pbuf\" isn't very descriptive, and it's\neasy to get confused about what is supposed to be in it\n(especially because we are reading both \"status\" and\n\"output\" from gpg).\n\nRather than give it a more descriptive name, we can just use\ngpg_status as our indirection pointer. Either it points to\nthe caller's input, or we can point it directly to our\ntemporary buffer.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n gpg-interface.c | 9 ++++-----\n 1 file changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 0ed9fa7..216cad8 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -211,7 +211,6 @@ int verify_signed_buffer(const char *payload, size_t payload_size,\n \tchar path[PATH_MAX];\n \tint fd, ret;\n \tstruct strbuf buf = STRBUF_INIT;\n-\tstruct strbuf *pbuf = &buf;\n \n \tfd = git_mkstemp(path, PATH_MAX, \".git_vtag_tmpXXXXXX\");\n \tif (fd < 0)\n@@ -242,9 +241,9 @@ int verify_signed_buffer(const char *payload, size_t payload_size,\n \t\tstrbuf_read(gpg_output, gpg.err, 0);\n \t\tclose(gpg.err);\n \t}\n-\tif (gpg_status)\n-\t\tpbuf = gpg_status;\n-\tstrbuf_read(pbuf, gpg.out, 0);\n+\tif (!gpg_status)\n+\t\tgpg_status = &buf;\n+\tstrbuf_read(gpg_status, gpg.out, 0);\n \tclose(gpg.out);\n \n \tret = finish_command(&gpg);\n@@ -252,7 +251,7 @@ int verify_signed_buffer(const char *payload, size_t payload_size,\n \n \tunlink_or_warn(path);\n \n-\tret |= !strstr(pbuf->buf, \"\\n[GNUPG:] GOODSIG \");\n+\tret |= !strstr(gpg_status->buf, \"\\n[GNUPG:] GOODSIG \");\n \tstrbuf_release(&buf); /* no matter it was used or not */\n \n \treturn ret;\n-- \n2.9.0.165.g4aacdc3\n\n"},{"id":"289535","messageId":"20160617233843.GC31958@sigill.intra.peff.net","threadId":"42626","inReplyTo":"20160617233819.GA31909@sigill.intra.peff.net","subject":"[PATCH v2 3/7] verify_signed_buffer: use tempfile object","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-17T23:38:43Z","receivedAt":"2016-06-17T23:38:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We use git_mkstemp to create a temporary file, and try to\nclean it up in all exit paths from the function. But that\nmisses any cases where we die by signal, or by calling die()\nin a sub-function. In addition, we missed one of the exit\npaths.\n\nLet's convert to using a tempfile object, which handles the\nhard cases for us, and add the missing cleanup call. Note\nthat we would not simply want to rely on program exit to\ncatch our missed cleanup, as this function may be called\nmany times in a single program (for the same reason, we use\na static tempfile instead of heap-allocating a new one; that\ngives an upper bound on our memory usage).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n gpg-interface.c | 21 +++++++++++++--------\n 1 file changed, 13 insertions(+), 8 deletions(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 216cad8..854c1e5 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -3,6 +3,7 @@\n #include \"strbuf.h\"\n #include \"gpg-interface.h\"\n #include \"sigchain.h\"\n+#include \"tempfile.h\"\n \n static char *configured_signing_key;\n static const char *gpg_program = \"gpg\";\n@@ -208,28 +209,32 @@ int verify_signed_buffer(const char *payload, size_t payload_size,\n \t\t\t struct strbuf *gpg_output, struct strbuf *gpg_status)\n {\n \tstruct child_process gpg = CHILD_PROCESS_INIT;\n-\tchar path[PATH_MAX];\n+\tstatic struct tempfile temp;\n \tint fd, ret;\n \tstruct strbuf buf = STRBUF_INIT;\n \n-\tfd = git_mkstemp(path, PATH_MAX, \".git_vtag_tmpXXXXXX\");\n+\tfd = mks_tempfile_t(&temp, \".git_vtag_tmpXXXXXX\");\n \tif (fd < 0)\n-\t\treturn error_errno(_(\"could not create temporary file '%s'\"), path);\n-\tif (write_in_full(fd, signature, signature_size) < 0)\n-\t\treturn error_errno(_(\"failed writing detached signature to '%s'\"), path);\n+\t\treturn error_errno(_(\"could not create temporary file\"));\n+\tif (write_in_full(fd, signature, signature_size) < 0) {\n+\t\terror_errno(_(\"failed writing detached signature to '%s'\"),\n+\t\t\t    temp.filename.buf);\n+\t\tdelete_tempfile(&temp);\n+\t\treturn -1;\n+\t}\n \tclose(fd);\n \n \targv_array_pushl(&gpg.args,\n \t\t\t gpg_program,\n \t\t\t \"--status-fd=1\",\n-\t\t\t \"--verify\", path, \"-\",\n+\t\t\t \"--verify\", temp.filename.buf, \"-\",\n \t\t\t NULL);\n \tgpg.in = -1;\n \tgpg.out = -1;\n \tif (gpg_output)\n \t\tgpg.err = -1;\n \tif (start_command(&gpg)) {\n-\t\tunlink(path);\n+\t\tdelete_tempfile(&temp);\n \t\treturn error(_(\"could not run gpg.\"));\n \t}\n \n@@ -249,7 +254,7 @@ int verify_signed_buffer(const char *payload, size_t payload_size,\n \tret = finish_command(&gpg);\n \tsigchain_pop(SIGPIPE);\n \n-\tunlink_or_warn(path);\n+\tdelete_tempfile(&temp);\n \n \tret |= !strstr(gpg_status->buf, \"\\n[GNUPG:] GOODSIG \");\n \tstrbuf_release(&buf); /* no matter it was used or not */\n-- \n2.9.0.165.g4aacdc3\n\n"},{"id":"289536","messageId":"20160617233846.GD31958@sigill.intra.peff.net","threadId":"42626","inReplyTo":"20160617233819.GA31909@sigill.intra.peff.net","subject":"[PATCH v2 4/7] run-command: add pipe_command helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-17T23:38:47Z","receivedAt":"2016-06-17T23:38:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We already have capture_command(), which captures the stdout\nof a command in a way that avoids deadlocks. But sometimes\nwe need to do more I/O, like capturing stderr as well, or\nsending data to stdin. It's easy to write code that\ndeadlocks racily in these situations depending on how fast\nthe command reads its input, or in which order it writes its\noutput.\n\nLet's give callers an easy interface for doing this the\nright way, similar to what capture_command() did for the\nsimple case.\n\nThe whole thing is backed by a generic poll() loop that can\nfeed an arbitrary number of buffers to descriptors, and fill\nan arbitrary number of strbufs from other descriptors. This\nseems like overkill, but the resulting code is actually a\nbit cleaner than just handling the three descriptors\n(because the output code for stdout/stderr is effectively\nduplicated, so being able to loop is a benefit).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n run-command.c | 152 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--\n run-command.h |  31 +++++++++---\n 2 files changed, 171 insertions(+), 12 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex af0c8a1..33bc63a 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -864,19 +864,161 @@ int run_hook_le(const char *const *env, const char *name, ...)\n \treturn ret;\n }\n \n-int capture_command(struct child_process *cmd, struct strbuf *buf, size_t hint)\n+struct io_pump {\n+\t/* initialized by caller */\n+\tint fd;\n+\tint type; /* POLLOUT or POLLIN */\n+\tunion {\n+\t\tstruct {\n+\t\t\tconst char *buf;\n+\t\t\tsize_t len;\n+\t\t} out;\n+\t\tstruct {\n+\t\t\tstruct strbuf *buf;\n+\t\t\tsize_t hint;\n+\t\t} in;\n+\t} u;\n+\n+\t/* returned by pump_io */\n+\tint error; /* 0 for success, otherwise errno */\n+\n+\t/* internal use */\n+\tstruct pollfd *pfd;\n+};\n+\n+static int pump_io_round(struct io_pump *slots, int nr, struct pollfd *pfd)\n+{\n+\tint pollsize = 0;\n+\tint i;\n+\n+\tfor (i = 0; i < nr; i++) {\n+\t\tstruct io_pump *io = &slots[i];\n+\t\tif (io->fd < 0)\n+\t\t\tcontinue;\n+\t\tpfd[pollsize].fd = io->fd;\n+\t\tpfd[pollsize].events = io->type;\n+\t\tio->pfd = &pfd[pollsize++];\n+\t}\n+\n+\tif (!pollsize)\n+\t\treturn 0;\n+\n+\tif (poll(pfd, pollsize, -1) < 0) {\n+\t\tif (errno == EINTR)\n+\t\t\treturn 1;\n+\t\tdie_errno(\"poll failed\");\n+\t}\n+\n+\tfor (i = 0; i < nr; i++) {\n+\t\tstruct io_pump *io = &slots[i];\n+\n+\t\tif (io->fd < 0)\n+\t\t\tcontinue;\n+\n+\t\tif (!(io->pfd->revents & (POLLOUT|POLLIN|POLLHUP|POLLERR|POLLNVAL)))\n+\t\t\tcontinue;\n+\n+\t\tif (io->type == POLLOUT) {\n+\t\t\tssize_t len = xwrite(io->fd,\n+\t\t\t\t\t     io->u.out.buf, io->u.out.len);\n+\t\t\tif (len < 0) {\n+\t\t\t\tio->error = errno;\n+\t\t\t\tclose(io->fd);\n+\t\t\t\tio->fd = -1;\n+\t\t\t} else {\n+\t\t\t\tio->u.out.buf += len;\n+\t\t\t\tio->u.out.len -= len;\n+\t\t\t\tif (!io->u.out.len) {\n+\t\t\t\t\tclose(io->fd);\n+\t\t\t\t\tio->fd = -1;\n+\t\t\t\t}\n+\t\t\t}\n+\t\t}\n+\n+\t\tif (io->type == POLLIN) {\n+\t\t\tssize_t len = strbuf_read_once(io->u.in.buf,\n+\t\t\t\t\t\t       io->fd, io->u.in.hint);\n+\t\t\tif (len < 0)\n+\t\t\t\tio->error = errno;\n+\t\t\tif (len <= 0) {\n+\t\t\t\tclose(io->fd);\n+\t\t\t\tio->fd = -1;\n+\t\t\t}\n+\t\t}\n+\t}\n+\n+\treturn 1;\n+}\n+\n+static int pump_io(struct io_pump *slots, int nr)\n+{\n+\tstruct pollfd *pfd;\n+\tint i;\n+\n+\tfor (i = 0; i < nr; i++)\n+\t\tslots[i].error = 0;\n+\n+\tALLOC_ARRAY(pfd, nr);\n+\twhile (pump_io_round(slots, nr, pfd))\n+\t\t; /* nothing */\n+\tfree(pfd);\n+\n+\t/* There may be multiple errno values, so just pick the first. */\n+\tfor (i = 0; i < nr; i++) {\n+\t\tif (slots[i].error) {\n+\t\t\terrno = slots[i].error;\n+\t\t\treturn -1;\n+\t\t}\n+\t}\n+\treturn 0;\n+}\n+\n+\n+int pipe_command(struct child_process *cmd,\n+\t\t const char *in, size_t in_len,\n+\t\t struct strbuf *out, size_t out_hint,\n+\t\t struct strbuf *err, size_t err_hint)\n {\n-\tcmd->out = -1;\n+\tstruct io_pump io[3];\n+\tint nr = 0;\n+\n+\tif (in)\n+\t\tcmd->in = -1;\n+\tif (out)\n+\t\tcmd->out = -1;\n+\tif (err)\n+\t\tcmd->err = -1;\n+\n \tif (start_command(cmd) < 0)\n \t\treturn -1;\n \n-\tif (strbuf_read(buf, cmd->out, hint) < 0) {\n-\t\tclose(cmd->out);\n+\tif (in) {\n+\t\tio[nr].fd = cmd->in;\n+\t\tio[nr].type = POLLOUT;\n+\t\tio[nr].u.out.buf = in;\n+\t\tio[nr].u.out.len = in_len;\n+\t\tnr++;\n+\t}\n+\tif (out) {\n+\t\tio[nr].fd = cmd->out;\n+\t\tio[nr].type = POLLIN;\n+\t\tio[nr].u.in.buf = out;\n+\t\tio[nr].u.in.hint = out_hint;\n+\t\tnr++;\n+\t}\n+\tif (err) {\n+\t\tio[nr].fd = cmd->err;\n+\t\tio[nr].type = POLLIN;\n+\t\tio[nr].u.in.buf = err;\n+\t\tio[nr].u.in.hint = err_hint;\n+\t\tnr++;\n+\t}\n+\n+\tif (pump_io(io, nr) < 0) {\n \t\tfinish_command(cmd); /* throw away exit code */\n \t\treturn -1;\n \t}\n \n-\tclose(cmd->out);\n \treturn finish_command(cmd);\n }\n \ndiff --git a/run-command.h b/run-command.h\nindex 11f76b0..5066649 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -79,17 +79,34 @@ int run_command_v_opt(const char **argv, int opt);\n int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const char *const *env);\n \n /**\n- * Execute the given command, capturing its stdout in the given strbuf.\n+ * Execute the given command, sending \"in\" to its stdin, and capturing its\n+ * stdout and stderr in the \"out\" and \"err\" strbufs. Any of the three may\n+ * be NULL to skip processing.\n+ *\n  * Returns -1 if starting the command fails or reading fails, and otherwise\n- * returns the exit code of the command. The output collected in the\n- * buffer is kept even if the command returns a non-zero exit. The hint field\n- * gives a starting size for the strbuf allocation.\n+ * returns the exit code of the command. Any output collected in the\n+ * buffers is kept even if the command returns a non-zero exit. The hint fields\n+ * gives starting sizes for the strbuf allocations.\n  *\n  * The fields of \"cmd\" should be set up as they would for a normal run_command\n- * invocation. But note that there is no need to set cmd->out; the function\n- * sets it up for the caller.\n+ * invocation. But note that there is no need to set the in, out, or err\n+ * fields; pipe_command handles that automatically.\n+ */\n+int pipe_command(struct child_process *cmd,\n+\t\t const char *in, size_t in_len,\n+\t\t struct strbuf *out, size_t out_hint,\n+\t\t struct strbuf *err, size_t err_hint);\n+\n+/**\n+ * Convenience wrapper around pipe_command for the common case\n+ * of capturing only stdout.\n  */\n-int capture_command(struct child_process *cmd, struct strbuf *buf, size_t hint);\n+static inline int capture_command(struct child_process *cmd,\n+\t\t\t\t  struct strbuf *out,\n+\t\t\t\t  size_t hint)\n+{\n+\treturn pipe_command(cmd, NULL, 0, out, hint, NULL, 0);\n+}\n \n /*\n  * The purpose of the following functions is to feed a pipe by running\n-- \n2.9.0.165.g4aacdc3\n\n"},{"id":"289537","messageId":"20160617233851.GE31958@sigill.intra.peff.net","threadId":"42626","inReplyTo":"20160617233819.GA31909@sigill.intra.peff.net","subject":"[PATCH v2 5/7] verify_signed_buffer: use pipe_command","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-17T23:38:52Z","receivedAt":"2016-06-17T23:38:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is shorter and should make the function easier to\nfollow. But more importantly, it removes the possibility of\nany deadlocks based on reading or writing to gpg.\n\nIt's not clear if such a deadlock is possible in practice.\n\nWe do write the whole payload before reading anything, so we\ncould deadlock there. However, in practice gpg will need to\nread our whole input to verify the signature, so it will\ndrain our payload first. It could write an error to stderr\nbefore reading, but it's unlikely that such an error\nwouldn't be followed by it immediately exiting, or that the\nerror would actually be larger than a pipe buffer.\n\nOn the writing side, we drain stderr (with the\nhuman-readable output) in its entirety before reading stdout\n(with the status-fd data). Running strace on \"gpg --verify\"\ndoes show interleaved output on the two descriptors:\n\n  write(2, \"gpg: \", 5)                    = 5\n  write(2, \"Signature made Thu 16 Jun 2016 0\"..., 73) = 73\n  write(1, \"[GNUPG:] SIG_ID tQw8KGcs9rBfLvAj\"..., 66) = 66\n  write(1, \"[GNUPG:] GOODSIG 69808639F9430ED\"..., 60) = 60\n  write(2, \"gpg: \", 5)                    = 5\n  write(2, \"Good signature from \\\"Jeff King <\"..., 47) = 47\n  write(2, \"\\n\", 1)                       = 1\n  write(2, \"gpg: \", 5)                    = 5\n  write(2, \"                aka \\\"Jeff King <\"..., 49) = 49\n  write(2, \"\\n\", 1)                       = 1\n  write(1, \"[GNUPG:] VALIDSIG C49CE24156AF08\"..., 135) = 135\n  write(1, \"[GNUPG:] TRUST_ULTIMATE\\n\", 24) = 24\n\nThe second line written to stdout there contains the\nsigner's UID, which can be arbitrarily long. If it fills the\npipe buffer, then gpg would block writing to its stdout,\nwhile we are blocked trying to read its stderr.\n\nIn practice, GPG seems to limit UIDs to 2048 bytes, so\nunless your pipe buffer size is quite small, or unless gpg\ndoes not enforce the limit under some conditions, this seems\nunlikely in practice.\n\nStill, it is not hard for us to be cautious and just use\npipe_command.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n gpg-interface.c | 22 +++-------------------\n 1 file changed, 3 insertions(+), 19 deletions(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 854c1e5..c98035d 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -229,29 +229,13 @@ int verify_signed_buffer(const char *payload, size_t payload_size,\n \t\t\t \"--status-fd=1\",\n \t\t\t \"--verify\", temp.filename.buf, \"-\",\n \t\t\t NULL);\n-\tgpg.in = -1;\n-\tgpg.out = -1;\n-\tif (gpg_output)\n-\t\tgpg.err = -1;\n-\tif (start_command(&gpg)) {\n-\t\tdelete_tempfile(&temp);\n-\t\treturn error(_(\"could not run gpg.\"));\n-\t}\n \n-\tsigchain_push(SIGPIPE, SIG_IGN);\n-\twrite_in_full(gpg.in, payload, payload_size);\n-\tclose(gpg.in);\n-\n-\tif (gpg_output) {\n-\t\tstrbuf_read(gpg_output, gpg.err, 0);\n-\t\tclose(gpg.err);\n-\t}\n \tif (!gpg_status)\n \t\tgpg_status = &buf;\n-\tstrbuf_read(gpg_status, gpg.out, 0);\n-\tclose(gpg.out);\n \n-\tret = finish_command(&gpg);\n+\tsigchain_push(SIGPIPE, SIG_IGN);\n+\tret = pipe_command(&gpg, payload, payload_size,\n+\t\t\t   gpg_status, 0, gpg_output, 0);\n \tsigchain_pop(SIGPIPE);\n \n \tdelete_tempfile(&temp);\n-- \n2.9.0.165.g4aacdc3\n\n"},{"id":"289538","messageId":"20160617233855.GF31958@sigill.intra.peff.net","threadId":"42626","inReplyTo":"20160617233819.GA31909@sigill.intra.peff.net","subject":"[PATCH v2 6/7] sign_buffer: use pipe_command","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-17T23:38:55Z","receivedAt":"2016-06-17T23:39:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Similar to the prior commit for verify_signed_buffer, the\nmotivation here is both to make the code simpler, and to\navoid any possible deadlocks with gpg.\n\nIn this case we have the same \"write to stdin, then read\nfrom stdout\" that the verify case had. This is unlikely to\nbe a problem in practice, since stdout has the detached\nsignature, which it cannot compute until it has read all of\nstdin (if it were a non-detached signature, that would be a\nproblem, though).\n\nWe don't read from stderr at all currently. However, we will\nwant to in a future patch, so this also prepares us there\n(and in that case gpg _does_ write before reading all of the\ninput, though again, it is unlikely that a key uid will fill\nup a pipe buffer).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n gpg-interface.c | 24 +++++-------------------\n 1 file changed, 5 insertions(+), 19 deletions(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex c98035d..74f08a2 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -151,40 +151,26 @@ const char *get_signing_key(void)\n int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key)\n {\n \tstruct child_process gpg = CHILD_PROCESS_INIT;\n-\tssize_t len;\n+\tint ret;\n \tsize_t i, j, bottom;\n \n-\tgpg.in = -1;\n-\tgpg.out = -1;\n \targv_array_pushl(&gpg.args,\n \t\t\t gpg_program,\n \t\t\t \"-bsau\", signing_key,\n \t\t\t NULL);\n \n-\tif (start_command(&gpg))\n-\t\treturn error(_(\"could not run gpg.\"));\n+\tbottom = signature->len;\n \n \t/*\n \t * When the username signingkey is bad, program could be terminated\n \t * because gpg exits without reading and then write gets SIGPIPE.\n \t */\n \tsigchain_push(SIGPIPE, SIG_IGN);\n-\n-\tif (write_in_full(gpg.in, buffer->buf, buffer->len) != buffer->len) {\n-\t\tclose(gpg.in);\n-\t\tclose(gpg.out);\n-\t\tfinish_command(&gpg);\n-\t\treturn error(_(\"gpg did not accept the data\"));\n-\t}\n-\tclose(gpg.in);\n-\n-\tbottom = signature->len;\n-\tlen = strbuf_read(signature, gpg.out, 1024);\n-\tclose(gpg.out);\n-\n+\tret = pipe_command(&gpg, buffer->buf, buffer->len,\n+\t\t\t   signature, 1024, NULL, 0);\n \tsigchain_pop(SIGPIPE);\n \n-\tif (finish_command(&gpg) || !len || len < 0)\n+\tif (ret || signature->len == bottom)\n \t\treturn error(_(\"gpg failed to sign the data\"));\n \n \t/* Strip CR from the line endings, in case we are on Windows. */\n-- \n2.9.0.165.g4aacdc3\n\n"},{"id":"289539","messageId":"20160617233859.GG31958@sigill.intra.peff.net","threadId":"42626","inReplyTo":"20160617233819.GA31909@sigill.intra.peff.net","subject":"[PATCH v2 7/7] gpg-interface: check gpg signature creation status","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-06-17T23:38:59Z","receivedAt":"2016-06-17T23:39:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: Michael J Gruber <git@drmicha.warpmail.net>\n\nWhen we create a signature, it may happen that gpg returns with\n\"success\" but not with an actual detached signature on stdout.\n\nCheck for the correct signature creation status to catch these cases\nbetter. Really, --status-fd parsing is the only way to check gpg status\nreliably. We do the same for verify already.\n\nSigned-off-by: Michael J Gruber <git@drmicha.warpmail.net>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n gpg-interface.c | 8 ++++++--\n t/t7004-tag.sh  | 9 ++++++++-\n 2 files changed, 14 insertions(+), 3 deletions(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 74f08a2..08356f9 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -153,9 +153,11 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig\n \tstruct child_process gpg = CHILD_PROCESS_INIT;\n \tint ret;\n \tsize_t i, j, bottom;\n+\tstruct strbuf gpg_status = STRBUF_INIT;\n \n \targv_array_pushl(&gpg.args,\n \t\t\t gpg_program,\n+\t\t\t \"--status-fd=2\",\n \t\t\t \"-bsau\", signing_key,\n \t\t\t NULL);\n \n@@ -167,10 +169,12 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig\n \t */\n \tsigchain_push(SIGPIPE, SIG_IGN);\n \tret = pipe_command(&gpg, buffer->buf, buffer->len,\n-\t\t\t   signature, 1024, NULL, 0);\n+\t\t\t   signature, 1024, &gpg_status, 0);\n \tsigchain_pop(SIGPIPE);\n \n-\tif (ret || signature->len == bottom)\n+\tret |= !strstr(gpg_status.buf, \"\\n[GNUPG:] SIG_CREATED \");\n+\tstrbuf_release(&gpg_status);\n+\tif (ret)\n \t\treturn error(_(\"gpg failed to sign the data\"));\n \n \t/* Strip CR from the line endings, in case we are on Windows. */\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex f9b7d79..8b0f71a 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1202,10 +1202,17 @@ test_expect_success GPG,RFC1991 \\\n # try to sign with bad user.signingkey\n git config user.signingkey BobTheMouse\n test_expect_success GPG \\\n-\t'git tag -s fails if gpg is misconfigured' \\\n+\t'git tag -s fails if gpg is misconfigured (bad key)' \\\n \t'test_must_fail git tag -s -m tail tag-gpg-failure'\n git config --unset user.signingkey\n \n+# try to produce invalid signature\n+test_expect_success GPG \\\n+\t'git tag -s fails if gpg is misconfigured (bad signature format)' \\\n+\t'test_config gpg.program echo &&\n+\t test_must_fail git tag -s -m tail tag-gpg-failure'\n+\n+\n # try to verify without gpg:\n \n rm -rf gpghome\n-- \n2.9.0.165.g4aacdc3\n"}]}