git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Jeff King <peff@peff.net>
Date
Jun 14, 2016, 22:26 UTC
Message-ID
<20160614222633.GA32020@sigill.intra.peff.net>
In-Reply-To
<20160614215019.GB22334@sigill.intra.peff.net>
On Tue, Jun 14, 2016 at 05:50:19PM -0400, Jeff King wrote:
Show 18 quoted lines
> 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.
Previous: Jeff KingNext: Junio C Hamano
Message 17 of 31 in “I lost my commit signature”
  1. ZhenTianJun 14, 2016
  2. Jeff KingJun 14, 2016
  3. ZhenTianJun 14, 2016
  4. Jeff KingJun 14, 2016
  5. ZhenTianJun 14, 2016
  6. Jeff KingJun 14, 2016
  7. ZhenTianJun 14, 2016
  8. Michael J GruberJun 14, 2016
  9. gpg-interface: check gpg signature for correct headerMichael J Gruber, Jun 14, 2016
  10. Jeff KingJun 14, 2016
  11. Michael J GruberJun 14, 2016
  12. Michael J GruberJun 14, 2016
  13. [PATCHv2] gpg-interface: check gpg signature for correct headerMichael J Gruber, Jun 14, 2016
  14. [PATCHv3] gpg-interface: check gpg signature creation statusMichael J Gruber, Jun 14, 2016
  15. Junio C HamanoJun 14, 2016
  16. Jeff KingJun 14, 2016
  17. Jeff KingJun 14, 2016
  18. Junio C HamanoJun 14, 2016
  19. Jeff KingJun 15, 2016
  20. Michael J GruberJun 15, 2016
  21. Jeff KingJun 16, 2016
  22. Michael J GruberJun 16, 2016
  23. Jeff KingJun 15, 2016
  24. ZhenTianJun 15, 2016
  25. Jeff KingJun 15, 2016
  26. Michael J GruberJun 15, 2016
  27. ZhenTianJun 15, 2016
  28. Jeff KingJun 16, 2016
  29. Junio C HamanoJun 16, 2016
  30. Michael J GruberJun 17, 2016
  31. Junio C HamanoJun 17, 2016

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.