v3, 4 partstag: move PGP verification code to tag.c

> From: Santiago Torres <santiago@nyu.edu>
> 
> The verify_signed_buffer comand might cause a SIGPIPE signal when the
> gpg child process terminates early (due to a bad keyid, for example) and
> git tries to write to it afterwards. Previously, ignoring SIGPIPE was
> done on the builtin/gpg-verify.c command to avoid this issue. However,

> From: Santiago Torres <santiago@nyu.edu>
>
> The verify_signed_buffer comand might cause a SIGPIPE signal when the
> gpg child process terminates early (due to a bad keyid, for example) and
> git tries to write to it afterwards. Previously, ignoring SIGPIPE was
> done on the builtin/gpg-verify.c command to avoid this issue. However,
> any other caller who wanted to use the verify_signed_buffer command
> would have to include this signal call.
>
> Instead, we use sigchain_push(SIGPIPE, SIG_IGN) on the
> verify_signed_buffer call (pretty much like in sign_buffer()) so
> that any caller is not required to perform this task. This will avoid
> possible mistakes by further developers using verify_signed_buffer.
>
> Signed-off-by: Santiago Torres <santiago@nyu.edu>
> ---
> Notes:
>   I dropped the multiline comment altogheter.
>
>   builtin/verify-tag.c | 3 ---
>   gpg-interface.c      | 3 +++
>   2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
> index 00663f6..77f070a 100644
> --- a/builtin/verify-tag.c
> +++ b/builtin/verify-tag.c
> @@ -95,9 +95,6 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
>   	if (verbose)
>   		flags |= GPG_VERIFY_VERBOSE;
>
> -	/* sometimes the program was terminated because this signal
> -	 * was received in the process of writing the gpg input: */
> -	signal(SIGPIPE, SIG_IGN);
>   	while (i < argc)
>   		if (verify_tag(argv[i++], flags))
>   			had_error = 1;
> diff --git a/gpg-interface.c b/gpg-interface.c
> index 3dc2fe3..c1f6b2d 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -232,6 +232,8 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
>   	if (gpg_output)
>   		gpg.err = -1;
>   	args_gpg[3] = path;
> +
> +	sigchain_push(SIGPIPE, SIG_IGN);
>   	if (start_command(&gpg)) {
>   		unlink(path);
>   		return error(_("could not run gpg."));

> @@ -250,6 +252,7 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
>   	close(gpg.out);
>
>   	ret = finish_command(&gpg);
> +	sigchain_pop(SIGPIPE);
>
>   	unlink_or_warn(path);
>
>

> Am 03.04.2016 um 01:16 schrieb santiago@nyu.edu:
> > From: Santiago Torres <santiago@nyu.edu>
> > 
> > The verify_signed_buffer comand might cause a SIGPIPE signal when the
> > gpg child process terminates early (due to a bad keyid, for example) and
> > git tries to write to it afterwards. Previously, ignoring SIGPIPE was
> > done on the builtin/gpg-verify.c command to avoid this issue. However,
> > any other caller who wanted to use the verify_signed_buffer command
> > would have to include this signal call.
> > 
> > Instead, we use sigchain_push(SIGPIPE, SIG_IGN) on the
> > verify_signed_buffer call (pretty much like in sign_buffer()) so
> > that any caller is not required to perform this task. This will avoid
> > possible mistakes by further developers using verify_signed_buffer.
> > 
> > Signed-off-by: Santiago Torres <santiago@nyu.edu>
> > ---
> > Notes:
> >   I dropped the multiline comment altogheter.
> > 
> >   builtin/verify-tag.c | 3 ---
> >   gpg-interface.c      | 3 +++
> >   2 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
> > index 00663f6..77f070a 100644
> > --- a/builtin/verify-tag.c
> > +++ b/builtin/verify-tag.c
> > @@ -95,9 +95,6 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
> >   	if (verbose)
> >   		flags |= GPG_VERIFY_VERBOSE;
> > 
> > -	/* sometimes the program was terminated because this signal
> > -	 * was received in the process of writing the gpg input: */
> > -	signal(SIGPIPE, SIG_IGN);
> >   	while (i < argc)
> >   		if (verify_tag(argv[i++], flags))
> >   			had_error = 1;
> > diff --git a/gpg-interface.c b/gpg-interface.c
> > index 3dc2fe3..c1f6b2d 100644
> > --- a/gpg-interface.c
> > +++ b/gpg-interface.c
> > @@ -232,6 +232,8 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
> >   	if (gpg_output)
> >   		gpg.err = -1;
> >   	args_gpg[3] = path;
> > +
> > +	sigchain_push(SIGPIPE, SIG_IGN);
> >   	if (start_command(&gpg)) {
> >   		unlink(path);
> >   		return error(_("could not run gpg."));
> 
> But no sigchain_pop() in the error path that we see here?
> 
> Perhaps you can even defer the sigchain_push() until after start_command()?

> In fact, I suspect you could replace the "GOODSIG" check as well by
> doing something like:
>
>   # verifying 3 tags in one invocation should be exactly like
>   # verifying the 3 separately
>   tags="fourth-signed sixth-signed seventh-signed"
>   for i in $tags; do
>           git verify-tag -v --raw $i || return 1
>   done >expect.stdout 2>expect.stderr &&
>   git verify-tag -v --raw $tags >actual.stdout 2>actual.stderr &&
>   test_cmp expect.stdout actual.stdout &&
>   test_cmp expect.stderr actual.stderr

> On Sun, Apr 3, 2016 at 12:40 AM, Jeff King <peff@peff.net> wrote:
> > In fact, I suspect you could replace the "GOODSIG" check as well by
> > doing something like:
> >
> >   # verifying 3 tags in one invocation should be exactly like
> >   # verifying the 3 separately
> >   tags="fourth-signed sixth-signed seventh-signed"
> >   for i in $tags; do
> >           git verify-tag -v --raw $i || return 1
> >   done >expect.stdout 2>expect.stderr &&
> >   git verify-tag -v --raw $tags >actual.stdout 2>actual.stderr &&
> >   test_cmp expect.stdout actual.stdout &&
> >   test_cmp expect.stderr actual.stderr
> 
> Hmm, does [1] suggest that using test_cmp on stderr here would be
> contraindicated?
> 
> [1]: http://article.gmane.org/gmane.comp.version-control.git/289077

> On Sun, Apr 03, 2016 at 03:59:46AM -0400, Eric Sunshine wrote:
> 
> > On Sun, Apr 3, 2016 at 12:40 AM, Jeff King <peff@peff.net> wrote:
> > > In fact, I suspect you could replace the "GOODSIG" check as well by
> > > doing something like:
> > >
> > >   # verifying 3 tags in one invocation should be exactly like
> > >   # verifying the 3 separately
> > >   tags="fourth-signed sixth-signed seventh-signed"
> > >   for i in $tags; do
> > >           git verify-tag -v --raw $i || return 1
> > >   done >expect.stdout 2>expect.stderr &&
> > >   git verify-tag -v --raw $tags >actual.stdout 2>actual.stderr &&
> > >   test_cmp expect.stdout actual.stdout &&
> > >   test_cmp expect.stderr actual.stderr
> > 
> > Hmm, does [1] suggest that using test_cmp on stderr here would be
> > contraindicated?
> > 
> > [1]: http://article.gmane.org/gmane.comp.version-control.git/289077
> 
> It does, but I am not sure I agree with the advice in that email in the
> general case (I like making "-x" work, too, but not at the cost of
> making the tests harder to read and write). In this case, I suppose you
> could grep for gpg raw-output on stderr, though, and compare only that.
> 
> -Peff

> On Sun, Apr 03, 2016 at 09:07:25AM -0400, Jeff King wrote:
>> On Sun, Apr 03, 2016 at 03:59:46AM -0400, Eric Sunshine wrote:
>> > On Sun, Apr 3, 2016 at 12:40 AM, Jeff King <peff@peff.net> wrote:
>> > > In fact, I suspect you could replace the "GOODSIG" check as well by
>> > > doing something like:
>> > >
>> > >   tags="fourth-signed sixth-signed seventh-signed"
>> > >   for i in $tags; do
>> > >           git verify-tag -v --raw $i || return 1
>> > >   done >expect.stdout 2>expect.stderr &&
>> > >   git verify-tag -v --raw $tags >actual.stdout 2>actual.stderr &&
>> > >   test_cmp expect.stdout actual.stdout &&
>> > >   test_cmp expect.stderr actual.stderr
>> >
>> > Hmm, does [1] suggest that using test_cmp on stderr here would be
>> > contraindicated?
>> >
>> > [1]: http://article.gmane.org/gmane.comp.version-control.git/289077
>>
>> It does, but I am not sure I agree with the advice in that email in the
>> general case (I like making "-x" work, too, but not at the cost of
>> making the tests harder to read and write). In this case, I suppose you
>> could grep for gpg raw-output on stderr, though, and compare only that.
>
> I just read [1], I'll take the later advice and use test_i18ngrep
> instead.

> I think Peff meant that a simple grep would suffice; no need for
> test_i18ngrep. In other words (reproducing Peff's example), something
> like this:
> 
>     tags="fourth-signed sixth-signed seventh-signed" &&
>     for i in $tags; do
>         git verify-tag -v --raw $i || return 1
>     done >expect.stdout 2>expect.stderr.1 &&
>     grep GOODSIG <expect.stderr.1 >expect.stderr &&
>     git verify-tag -v --raw $tags >actual.stdout 2>actual.stderr.1 &&
>     grep GOODSIG <actual.stderr.1 >actual.stderr &&
>     test_cmp expect.stdout actual.stdout &&
>     test_cmp expect.stderr actual.stderr

> From: Santiago Torres <santiago@nyu.edu>
> 
> The PGP verification routine for tags could be accessed by other
> commands that require it. We do this by moving it to the common tag.c
> code. We rename the verify_tag() function to pgp_verify_tag() to avoid
> conflicts with the mktag.c function.

> -	len = parse_signature(buf, size);
> -
> -	if (size == len) {
> -		if (flags & GPG_VERIFY_VERBOSE)
> -			write_in_full(1, buf, len);
> -		return error("no signature found");
> -	}
> [...]
> +	payload_size = parse_signature(buf, size);
> +
> +	if (size == payload_size) {
> +		write_in_full(1, buf, payload_size);
> +		return error("No PGP signature found in this tag!");
> +	}

> On Sat, Apr 02, 2016 at 07:16:14PM -0400, santiago@nyu.edu wrote:
>> -     len = parse_signature(buf, size);
>> -
>> -     if (size == len) {
>> -             if (flags & GPG_VERIFY_VERBOSE)
>> -                     write_in_full(1, buf, len);
>> -             return error("no signature found");
>> -     }
>> [...]
>> +     payload_size = parse_signature(buf, size);
>> +
>> +     if (size == payload_size) {
>> +             write_in_full(1, buf, payload_size);
>> +             return error("No PGP signature found in this tag!");
>> +     }
>
> I'm happy to see the more readable variable name here. I wonder if we
> should leave the error message as-is, though, as this is just supposed
> to be about code movement (and if we are changing it, it should adhere
> to our usual style of not starting with a capital letter, and not ending
> in punctuation).

> The PGP verification routine for tags could be accessed by other
> commands that require it. We do this by moving it to the common tag.c
> code. We rename the verify_tag() function to pgp_verify_tag() to avoid
> conflicts with the mktag.c function.
>
> Signed-off-by: Santiago Torres <santiago@nyu.edu>
> ---
> -       len = parse_signature(buf, size);
> -
> -       if (size == len) {
> -               if (flags & GPG_VERIFY_VERBOSE)
> -                       write_in_full(1, buf, len);
> -               return error("no signature found");
> -       }
> [...]
> +       payload_size = parse_signature(buf, size);
> +
> +       if (size == payload_size) {
> +               write_in_full(1, buf, payload_size);
> +               return error("No PGP signature found in this tag!");
> +       }

> On Sat, Apr 2, 2016 at 7:16 PM,  <santiago@nyu.edu> wrote:
> > The PGP verification routine for tags could be accessed by other
> > commands that require it. We do this by moving it to the common tag.c
> > code. We rename the verify_tag() function to pgp_verify_tag() to avoid
> > conflicts with the mktag.c function.
> >
> > Signed-off-by: Santiago Torres <santiago@nyu.edu>
> > ---
> > -       len = parse_signature(buf, size);
> > -
> > -       if (size == len) {
> > -               if (flags & GPG_VERIFY_VERBOSE)
> > -                       write_in_full(1, buf, len);
> > -               return error("no signature found");
> > -       }
> > [...]
> > +       payload_size = parse_signature(buf, size);
> > +
> > +       if (size == payload_size) {
> > +               write_in_full(1, buf, payload_size);
> > +               return error("No PGP signature found in this tag!");
> > +       }
> 
> Also, [1] asked why the moved code no longer respects
> GPG_VERIFY_VERBOSE, and that question doesn't seem to be answered
> either in the previous review thread or by this patch's commit
> message. It's not clear at a casual glance why this change is
> desirable.
> 

> diff --git a/builtin/tag.c b/builtin/tag.c
> index 1705c94..3dffdff 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -65,9 +65,10 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, con
>  }
>  
>  typedef int (*each_tag_name_fn)(const char *name, const char *ref,
> -				const unsigned char *sha1);
> +				const unsigned char *sha1, unsigned flags);

> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
> index f776778..8abc357 100644
> --- a/builtin/verify-tag.c
> +++ b/builtin/verify-tag.c
> @@ -30,6 +30,8 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
>  {
>  	int i = 1, verbose = 0, had_error = 0;
>  	unsigned flags = 0;
> +	unsigned char sha1[20];
> +	const char *name;
>  	const struct option verify_tag_options[] = {
>  		OPT__VERBOSE(&verbose, N_("print tag contents")),
>  		OPT_BIT(0, "raw", &flags, N_("print raw gpg status output"), GPG_VERIFY_RAW),
> @@ -46,8 +48,16 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
>  	if (verbose)
>  		flags |= GPG_VERIFY_VERBOSE;
>  
> -	while (i < argc)
> -		if (pgp_verify_tag(argv[i++], flags))
> +	while (i < argc) {
> +		name = argv[i++];
> +		if (get_sha1(name, sha1)) {
> +			error("tag '%s' not found.", name);
>  			had_error = 1;
> +		}
> +
> +		if (pgp_verify_tag(name, NULL, sha1, flags))
> +			had_error = 1;
> +
> +	}

> >  	if (cmdmode == 'v')
> > -		return for_each_tag_name(argv, verify_tag);
> > +		return for_each_tag_name(argv, pgp_verify_tag,
> > +				GPG_VERIFY_VERBOSE);
> 
> but I think that is coupling too closely. What happens later when the
> public, multi-file pgp_verify_tag function changes its interface? Or we
> want to change our interface here, and it no longer matches
> pgp_verify_tag? The results ripple a lot further than they should.
> 
> I think you probably want to keep a simple adapter callback in this
> file, like:
> 
>   int verify_tag(const char *name, const char *ref, const unsigned char *sha1)
>   {
> 	return pgp_verify_tag(name, GPG_VERIFY_VERBOSE));
>   }

> > diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
> > index f776778..8abc357 100644
> > --- a/builtin/verify-tag.c
> > +++ b/builtin/verify-tag.c
> > @@ -30,6 +30,8 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
> >  {
> >  	int i = 1, verbose = 0, had_error = 0;
> >  	unsigned flags = 0;
> > +	unsigned char sha1[20];
> > +	const char *name;
> >  	const struct option verify_tag_options[] = {
> >  		OPT__VERBOSE(&verbose, N_("print tag contents")),
> >  		OPT_BIT(0, "raw", &flags, N_("print raw gpg status output"), GPG_VERIFY_RAW),
> > @@ -46,8 +48,16 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
> >  	if (verbose)
> >  		flags |= GPG_VERIFY_VERBOSE;
> >  
> > -	while (i < argc)
> > -		if (pgp_verify_tag(argv[i++], flags))
> > +	while (i < argc) {
> > +		name = argv[i++];
> > +		if (get_sha1(name, sha1)) {
> > +			error("tag '%s' not found.", name);
> >  			had_error = 1;
> > +		}
> > +
> > +		if (pgp_verify_tag(name, NULL, sha1, flags))
> > +			had_error = 1;
> > +
> > +	}
> 
> So this is a good example of the rippling I mentioned earlier.
> 
> As a side note, it might actually be an improvement for pgp_verify_tag
> to take a sha1 (so that git-tag is sure that it is verifying the same
> object that it is printing), but that refactoring should probably come
> separately, I think.
> 
> -Peff

> > As a side note, it might actually be an improvement for pgp_verify_tag
> > to take a sha1 (so that git-tag is sure that it is verifying the same
> > object that it is printing), but that refactoring should probably come
> > separately, I think.
> 
> Just to be sure, this refactoring is something we should still include
> in this set of patches, right? I think that otherwise we'd lose the
> desambigutaion that git tag -v does in this patch.

> On Mon, Apr 04, 2016 at 12:12:04AM -0400, Santiago Torres wrote:
> 
> > > As a side note, it might actually be an improvement for pgp_verify_tag
> > > to take a sha1 (so that git-tag is sure that it is verifying the same
> > > object that it is printing), but that refactoring should probably come
> > > separately, I think.
> > 
> > Just to be sure, this refactoring is something we should still include
> > in this set of patches, right? I think that otherwise we'd lose the
> > desambigutaion that git tag -v does in this patch.
> 
> I think it can be part of this series, but doesn't have to be. As I
> understand it, the current code is just handing the name to the `git
> verify-tag` process, so if we continue to do so, that would be OK.

> 
> > I also think that most of the rippling is gone if we use and adaptor as
> > you suggested. Should I add a patch on top of this to support a sha1 as
> > part for gpg_verify_tag()?
> 
> Yes, though I'd generally advise against a function taking either a name or
> a sha1, and ignoring the other option. That often leads to confusing
> interfaces for the callers. Instead, perhaps just take the sha1, and let
> the caller do the get_sha1() themselves. Or possibly provide two
> functions, one of which is a convenience to translate the name to sha1
> and then call the other.

> > I think it can be part of this series, but doesn't have to be. As I
> > understand it, the current code is just handing the name to the `git
> > verify-tag` process, so if we continue to do so, that would be OK.
> 
> IIRC, the current code for git tag -v hands the hex-representation[1] of
> the sha1 to git verify-tag --- I believe that's related to the
> desamgibuation issue I've seen people discuss.  I think this behavior is
> lost unless we add this on top of the patch.

> > Yes, though I'd generally advise against a function taking either a name or
> > a sha1, and ignoring the other option. That often leads to confusing
> > interfaces for the callers. Instead, perhaps just take the sha1, and let
> > the caller do the get_sha1() themselves. Or possibly provide two
> > functions, one of which is a convenience to translate the name to sha1
> > and then call the other.
> 
> I think the former sounds easier. I can replace the name argument and
> move the sha1-resolution code to in verify-tag. git tag -v already
> resolves the tagname to a sha1, so it is easier there.
> 
> Does this sound reasonable?