From 0d2b664efd815a3f6432723adb41732d90cc9be1 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 17 Jun 2016 19:38:52 -0400 Subject: 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(-) (limited to 'gpg-interface.c') 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); -- cgit v1.2.1