Re: [PATCHv3] gpg-interface: check gpg signature creation status

[Jeff King <peff@peff.net>]

On Tue, Jun 14, 2016 at 05:50:19PM -0400, Jeff King wrote:

> On Tue, Jun 14, 2016 at 11:13:54AM -0700, Junio C Hamano wrote:
> 
> > Michael J Gruber <git@drmicha.warpmail.net> writes:
> > 
> > >  	bottom = signature->len;
> > > -	len = strbuf_read(signature, gpg.out, 1024);
> > > +	strbuf_read(signature, gpg.out, 1024);
> > > +	strbuf_read(&err, gpg.err, 0);
> > 
> > Hmmmm, isn't this asking for a deadlock?  When GPG spews more than
> > what would fit in a pipe buffer to its standard error (hence gets
> > blocked), its standard output may not complete, and the we would get
> > stuck by attempting to read from gpg.out, failing to reach the other
> > strbuf_read() that would unblock GPG by reading from gpg.err?
> 
> Yeah, it definitely is a deadlock. I think we'd need a select loop to
> read into multiple strbufs at once (we can't use "struct async" because
> that might happen in another process).

Something like this on top of Michael's patch (only lightly tested).

I'm still undecided on whether it is a better approach than making sure
the stdout we got looks sane. In particular I'd worry that it would make
things harder for somebody trying to plug in something gpg-like (e.g.,
if you wanted to do something exotic like call a program which fetched
the signature from a remote device or something). But it's probably not
_that_ hard for such a script to emulate --status-fd.

---
diff --git a/gpg-interface.c b/gpg-interface.c
index 850dc81..576e462 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -153,6 +153,7 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig
 	const char *args[5];
 	size_t i, j, bottom;
 	struct strbuf err = STRBUF_INIT;
+	struct strbuf_reader readers[2];
 
 	gpg.argv = args;
 	gpg.in = -1;
@@ -183,8 +184,15 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig
 	close(gpg.in);
 
 	bottom = signature->len;
-	strbuf_read(signature, gpg.out, 1024);
-	strbuf_read(&err, gpg.err, 0);
+
+	readers[0].buf = signature;
+	readers[0].fd = gpg.out;
+	readers[0].hint = 1024;
+	readers[1].buf = &err;
+	readers[1].fd = gpg.err;
+	readers[1].hint = 1024;
+	strbuf_read_parallel(readers, 2);
+
 	close(gpg.out);
 	close(gpg.err);
 
diff --git a/strbuf.c b/strbuf.c
index 1ba600b..f674b23 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -395,6 +395,58 @@ ssize_t strbuf_read_once(struct strbuf *sb, int fd, size_t hint)
 	return cnt;
 }
 
+void strbuf_read_parallel(struct strbuf_reader *readers, int nr)
+{
+	int i;
+	struct pollfd *pfd;
+
+	ALLOC_ARRAY(pfd, nr);
+	for (i = 0; i < nr; i++)
+		readers[i].error = 0;
+
+	while (1) {
+		int pollsize = 0;
+		int ret;
+
+		for (i = 0; i < nr; i++) {
+			if (readers[i].fd < 0)
+				continue;
+			pfd[pollsize].fd = readers[i].fd;
+			pfd[pollsize].events = POLLIN;
+			readers[i].pfd = &pfd[pollsize];
+			pollsize++;
+		}
+
+		if (!pollsize)
+			break;
+
+		ret = poll(pfd, pollsize, -1);
+		if (ret < 0) {
+			if (errno == EINTR)
+				continue;
+			/* should never happen? */
+			die_errno("poll failed");
+		}
+
+		for (i = 0; i < nr; i++) {
+			if (readers[i].fd < 0)
+				continue;
+			if (readers[i].pfd->revents &
+			    (POLLIN|POLLHUP|POLLERR|POLLNVAL)) {
+				ret = strbuf_read_once(readers[i].buf,
+						       readers[i].fd,
+						       readers[i].hint);
+				if (ret < 0)
+					readers[i].error = errno;
+				if (ret <= 0)
+					readers[i].fd = -1;
+			}
+		}
+	}
+
+	free(pfd);
+}
+
 ssize_t strbuf_write(struct strbuf *sb, FILE *f)
 {
 	return sb->len ? fwrite(sb->buf, 1, sb->len, f) : 0;
diff --git a/strbuf.h b/strbuf.h
index 7987405..b93822e 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -374,6 +374,25 @@ extern ssize_t strbuf_read(struct strbuf *, int fd, size_t hint);
  */
 extern ssize_t strbuf_read_once(struct strbuf *, int fd, size_t hint);
 
+/**
+ * Read from several descriptors in parallel, each into its own strbuf.
+ * This can be used, for example, to capture stdout and stderr from a
+ * sub-process without worrying about deadlocks.
+ */
+struct strbuf_reader {
+	/* Initialized by caller */
+	struct strbuf *buf;
+	int fd;
+	size_t hint;
+
+	/* Returned by strbuf_read_parallel */
+	int error; /* 0 for success, otherwise errno */
+
+	/* Internal use */
+	struct pollfd *pfd;
+};
+void strbuf_read_parallel(struct strbuf_reader *readers, int nr);
+
 /**
  * Read the contents of a file, specified by its path. The third argument
  * can be used to give a hint about the file size, to avoid reallocs.