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

From: Jeff King <peff@peff.net>
Date: Jun 15, 2016, 03:28 UTC
Message-ID: <20160615032804.GA2249@sigill.intra.peff.net>
In-Reply-To: <20160614222633.GA32020@sigill.intra.peff.net>

On Tue, Jun 14, 2016 at 06:26:33PM -0400, Jeff King wrote:

Show 16 quoted lines

> > > >  	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 wondered if this is something we might run into in other places, but just haven't in practice. After grepping existing calls to strbuf_read(), I think the only other case is the one in verify_signed_buffer(), which reads all of stderr before trying to read stdout. I suspect that _could_ deadlock but doesn't tend to in practice.

Interestingly, it also writes in full to gpg's stdin before reading anything. If gpg buffers all of its input before writing, that's fine, but in theory it does not have to. I have no idea if it's possible for it to be a problem in practice.

That can be solved in the same select loop, though obviously it's not just strbuf_read_parallel() at that point, but rather a kind of a feed_and_capture_command().

-Peff