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

[Michael J Gruber]

Jeff King venit, vidit, dixit 16.06.2016 11:25:
> On Wed, Jun 15, 2016 at 09:17:54AM +0200, Michael J Gruber wrote:
> 
>> As for the flexibility:
>> We do code specifically for gpg, which happens to work for gpg2 also.
>> The patch doesn't add any gpg ui requirements that we don't require
>> elsewhere already.
>> More flexibility requires a completely pluggable approach - gpgsm
>> already fails to meet the gpg command line ui.
> 
> Does it? I haven't run it in production, but I did some tests with gpgsm
> a few months ago and found it to be a drop-in replacement, at least with
> respect to git. Though I don't think that matters one way or the other
> for the current discussion; it _does_ do --status-fd.

You are right!

I solely trusted in the documentation rather than simply trying it out.
gpgsm even supports the undocumented short options.

Only the resulting header is different (plus the fact that I have to
turn of crl checks for whatever reason).

>> In any case, "status-fd" is *the* way to interface with gpg reliably
>> just like plumbing commands are *the* way to interface with git reliably.
> 
> Fair enough. I've generally found its exit code pretty reliable. It's
> unclear to me if the problem you saw was because gpg was exiting 0 but
> producing no signature, or if your debugging was masking its exit code.
> 
> Either way, I do not mind using --status-fd; it seems like it should be
> more robust in general. It's the tip commit in the series I'm about to
> post.
> 
>> As for the read locking:
>> I'm sorry I have no idea about that area at all. I thought that I'm
>> doing the same that we do for verify, but apparently not. My
>> strbuf_read-fu is not that strong either (read: copy). I trust
>> your assessment completely, though.
> 
> Yeah, you're right that the deadlock thing is also a possibility on the
> verification side. I'm not sure whether it's possible to trigger in
> practice or not. See the analysis in the series.

With great admiration I've looked at your series which solves this
problem at the root. Way above my head (the solution, not the root,
fortunately).

>> As for the original problem:
>> That had a different cause, as we know now (rebase dropping signatures
>> without hint). I still think we should check gpg status codes properly.
> 
> Yep, I agree.
> 
> -Peff
> 

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jeff King]

On Wed, Jun 15, 2016 at 09:17:54AM +0200, Michael J Gruber wrote:

> As for the flexibility:
> We do code specifically for gpg, which happens to work for gpg2 also.
> The patch doesn't add any gpg ui requirements that we don't require
> elsewhere already.
> More flexibility requires a completely pluggable approach - gpgsm
> already fails to meet the gpg command line ui.

Does it? I haven't run it in production, but I did some tests with gpgsm
a few months ago and found it to be a drop-in replacement, at least with
respect to git. Though I don't think that matters one way or the other
for the current discussion; it _does_ do --status-fd.

> In any case, "status-fd" is *the* way to interface with gpg reliably
> just like plumbing commands are *the* way to interface with git reliably.

Fair enough. I've generally found its exit code pretty reliable. It's
unclear to me if the problem you saw was because gpg was exiting 0 but
producing no signature, or if your debugging was masking its exit code.

Either way, I do not mind using --status-fd; it seems like it should be
more robust in general. It's the tip commit in the series I'm about to
post.

> As for the read locking:
> I'm sorry I have no idea about that area at all. I thought that I'm
> doing the same that we do for verify, but apparently not. My
> strbuf_read-fu is not that strong either (read: copy). I trust
> your assessment completely, though.

Yeah, you're right that the deadlock thing is also a possibility on the
verification side. I'm not sure whether it's possible to trigger in
practice or not. See the analysis in the series.

> As for the original problem:
> That had a different cause, as we know now (rebase dropping signatures
> without hint). I still think we should check gpg status codes properly.

Yep, I agree.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Michael J Gruber]

Jeff King venit, vidit, dixit 15.06.2016 02:56:
> On Tue, Jun 14, 2016 at 04:47:35PM -0700, Junio C Hamano wrote:
> 
>> Jeff King  writes:
>>
>>> 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.
>>
>> I share the same thinking, but at the same time, it already is a
>> requirement to give --status-fd output that is close enough on the
>> signature verification side, isn't it?
> 
> Yeah, though I could see somebody wanting to sit amidst the signing side
> but not verification (e.g., if your keys are elsewhere from the machine
> running git). Of course such a case could probably ferry --status-fd
> from the other side anyway.
> 
> I admit I don't know of such a case in practice, though, and
> implementing a rudimentary --status-fd to say "SIG OK" or whatever on
> the signing side is not _that_ big a deal. So if we think this approach
> is a more robust solution in the normal case, let's not hold it up over
> what-ifs.
> 
> -Peff

As for the flexibility:
We do code specifically for gpg, which happens to work for gpg2 also.
The patch doesn't add any gpg ui requirements that we don't require
elsewhere already.
More flexibility requires a completely pluggable approach - gpgsm
already fails to meet the gpg command line ui.

In any case, "status-fd" is *the* way to interface with gpg reliably
just like plumbing commands are *the* way to interface with git reliably.

As for the read locking:
I'm sorry I have no idea about that area at all. I thought that I'm
doing the same that we do for verify, but apparently not. My
strbuf_read-fu is not that strong either (read: copy). I trust
your assessment completely, though.

As for the original problem:
That had a different cause, as we know now (rebase dropping signatures
without hint). I still think we should check gpg status codes properly.

Michael
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jeff King]

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

> > > > bottom = signature->len;
> > > > -   len = strbuf_read(signature, gpg.out, 1024);
> > > > +   strbuf_read(signature, gpg.out, 1024);
> > > > +   strbuf_read(, gpg.err, 0);
> > > 
> > > H, 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jeff King]

On Tue, Jun 14, 2016 at 04:47:35PM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > 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.
> 
> I share the same thinking, but at the same time, it already is a
> requirement to give --status-fd output that is close enough on the
> signature verification side, isn't it?

Yeah, though I could see somebody wanting to sit amidst the signing side
but not verification (e.g., if your keys are elsewhere from the machine
running git). Of course such a case could probably ferry --status-fd
from the other side anyway.

I admit I don't know of such a case in practice, though, and
implementing a rudimentary --status-fd to say "SIG OK" or whatever on
the signing side is not _that_ big a deal. So if we think this approach
is a more robust solution in the normal case, let's not hold it up over
what-ifs.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Jeff King  writes:

> 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.

I share the same thinking, but at the same time, it already is a
requirement to give --status-fd output that is close enough on the
signature verification side, isn't it?

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jeff King]

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  writes:
> > 
> > >   bottom = signature->len;
> > > - len = strbuf_read(signature, gpg.out, 1024);
> > > + strbuf_read(signature, gpg.out, 1024);
> > > + strbuf_read(, gpg.err, 0);
> > 
> > H, 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(, gpg.err, 0);
+
+   readers[0].buf = signature;
+   readers[0].fd = gpg.out;
+   readers[0].hint = 1024;
+   readers[1].buf = 
+   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 = [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;
+   

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

[Jeff King]

On Tue, Jun 14, 2016 at 11:13:54AM -0700, Junio C Hamano wrote:

> Michael J Gruber  writes:
> 
> > bottom = signature->len;
> > -   len = strbuf_read(signature, gpg.out, 1024);
> > +   strbuf_read(signature, gpg.out, 1024);
> > +   strbuf_read(, gpg.err, 0);
> 
> H, 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).

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Michael J Gruber  writes:

>   bottom = signature->len;
> - len = strbuf_read(signature, gpg.out, 1024);
> + strbuf_read(signature, gpg.out, 1024);
> + strbuf_read(, gpg.err, 0);

H, 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?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html