threads / rfc / 41818

RFC patchbuiltin/tag.c: move PGP verification inside builtin.

Subject: [PATCH/RFC] builtin/tag.c: move PGP verification inside builtin.

## tl;dr

7 messages between Mar 24, 2016 and Mar 24, 2016. Diffs are folded; open one to read it.

replies: 6people: 3as markdown or json

santiago@nyu.edu· Mar 24, 2016, 21:39 UTC · lore
From: Santiago Torres <torresariass@gmail.com>

The verify tag function is just a thin wrapper around the verify-tag command. We can avoid one fork call by doing the verification instide the tag builtin instead.

Signed-off-by: Santiago Torres <santiago@nyu.edu>
---
 builtin/tag.c | 44 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 38 insertions(+), 6 deletions(-)
Show changes to builtin/tag.c +38 −6
diff --git a/builtin/tag.c b/builtin/tag.c
index 1705c94..be5d7c7 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -30,6 +30,27 @@ static const char * const git_tag_usage[] = {
 
 static unsigned int colopts;
 
+static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
+{
+	struct signature_check sigc;
+	int len;
+	int ret;
+
+	memset(&sigc, 0, sizeof(sigc));
+
+	len = parse_signature(buf, size);
+
+	if (size == len) {
+		write_in_full(1, buf, len);
+	}
+
+	ret = check_signature(buf, len, buf + len, size - len, &sigc);
+	print_signature_buffer(&sigc, flags);
+
+	signature_check_clear(&sigc);
+	return ret;
+}
+
 static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, const char *format)
 {
 	struct ref_array array;
@@ -104,13 +125,24 @@ static int delete_tag(const char *name, const char *ref,
 static int verify_tag(const char *name, const char *ref,
 				const unsigned char *sha1)
 {
-	const char *argv_verify_tag[] = {"verify-tag",
-					"-v", "SHA1_HEX", NULL};
-	argv_verify_tag[2] = sha1_to_hex(sha1);
 
-	if (run_command_v_opt(argv_verify_tag, RUN_GIT_CMD))
-		return error(_("could not verify the tag '%s'"), name);
-	return 0;
+	enum object_type type;
+	unsigned long size;
+	const char* buf;
+	int ret;
+
+	type = sha1_object_info(sha1, NULL);
+	if (type != OBJ_TAG)
+		return error("%s: cannot verify a non-tag object of type %s.",
+				name, typename(type));
+
+	buf = read_sha1_file(sha1, &type, &size);
+	if (!buf)
+		return error("%s: unable to read file.", name);
+
+	ret = run_gpg_verify(buf, size, 0);
+
+	return ret;
 }
 
 static int do_sign(struct strbuf *buffer)

-- 
2.7.3
Santiago Torres· Mar 24, 2016, 21:51 UTC · re: santiago@nyu.edu · lore

Re: [PATCH/RFC] builtin/tag.c: move PGP verification inside builtin.

Hi Jeff.
Sorry for the delay with this, I got caught up with coursework.

This is my first stab at this, in the dumbest/simplest way imaginable. I don't like that there is no code reuse (the run_gpg_verify function is repeated here and in the plumbing command). I would appreciate pointers on what would be the best way to avoid this.

I also spent quite some time figuring out what you meant with
> Do note the trickery with SIGPIPE in verify-tag, though. We probably
> need to do the same here (in fact, I wonder if that should be pushed
> down into the code that calls gpg).
I don't see any explicit SIGPIPE trickery here. Any pointers?

Thanks! -Santiago.

On Thu, Mar 24, 2016 at 05:39:20PM -0400, santiago@nyu.edu wrote:
Show 77 quoted lines
> From: Santiago Torres <torresariass@gmail.com>
> 
> The verify tag function is just a thin wrapper around the verify-tag
> command. We can avoid one fork call by doing the verification instide
> the tag builtin instead.
> 
> Signed-off-by: Santiago Torres <santiago@nyu.edu>
> ---
>  builtin/tag.c | 44 ++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 38 insertions(+), 6 deletions(-)
> 
> diff --git a/builtin/tag.c b/builtin/tag.c
> index 1705c94..be5d7c7 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -30,6 +30,27 @@ static const char * const git_tag_usage[] = {
>  
>  static unsigned int colopts;
>  
> +static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
> +{
> +	struct signature_check sigc;
> +	int len;
> +	int ret;
> +
> +	memset(&sigc, 0, sizeof(sigc));
> +
> +	len = parse_signature(buf, size);
> +
> +	if (size == len) {
> +		write_in_full(1, buf, len);
> +	}
> +
> +	ret = check_signature(buf, len, buf + len, size - len, &sigc);
> +	print_signature_buffer(&sigc, flags);
> +
> +	signature_check_clear(&sigc);
> +	return ret;
> +}
> +
>  static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, const char *format)
>  {
>  	struct ref_array array;
> @@ -104,13 +125,24 @@ static int delete_tag(const char *name, const char *ref,
>  static int verify_tag(const char *name, const char *ref,
>  				const unsigned char *sha1)
>  {
> -	const char *argv_verify_tag[] = {"verify-tag",
> -					"-v", "SHA1_HEX", NULL};
> -	argv_verify_tag[2] = sha1_to_hex(sha1);
>  
> -	if (run_command_v_opt(argv_verify_tag, RUN_GIT_CMD))
> -		return error(_("could not verify the tag '%s'"), name);
> -	return 0;
> +	enum object_type type;
> +	unsigned long size;
> +	const char* buf;
> +	int ret;
> +
> +	type = sha1_object_info(sha1, NULL);
> +	if (type != OBJ_TAG)
> +		return error("%s: cannot verify a non-tag object of type %s.",
> +				name, typename(type));
> +
> +	buf = read_sha1_file(sha1, &type, &size);
> +	if (!buf)
> +		return error("%s: unable to read file.", name);
> +
> +	ret = run_gpg_verify(buf, size, 0);
> +
> +	return ret;
>  }
>  
>  static int do_sign(struct strbuf *buffer)
> -- 
> 2.7.3
>
Jeff King· Mar 24, 2016, 22:14 UTC · re: Santiago Torres · lore

Re: [PATCH/RFC] builtin/tag.c: move PGP verification inside builtin.

On Thu, Mar 24, 2016 at 05:51:05PM -0400, Santiago Torres wrote:
> Sorry for the delay with this, I got caught up with coursework.
No problem. The project moves forward as contributor time permits.
> This is my first stab at this, in the dumbest/simplest way imaginable. I
> don't like that there is no code reuse (the run_gpg_verify function is
> repeated here and in the plumbing command). I would appreciate pointers
> on what would be the best way to avoid this.

It looks to me like you could factor the repeated code into a common verify_tag(), but maybe I am missing something.

Show 6 quoted lines
> I also spent quite some time figuring out what you meant with
> 
> > Do note the trickery with SIGPIPE in verify-tag, though. We probably
> > need to do the same here (in fact, I wonder if that should be pushed
> > down into the code that calls gpg).
> I don't see any explicit SIGPIPE trickery here. Any pointers?

There is a call to ignore SIGPIPE in builtin/verify-tag.c, line 100. Do we need to do be doing the same thing here?

There's some discussion in the thread starting at:
  http://thread.gmane.org/gmane.comp.version-control.git/53878/focus=53904

The claim there is that we get SIGPIPE and die early if we feed gpg a tag which isn't signed. We _should_ be catching that case already via parse_signature(), though I wonder if it can be fooled (e.g., something that looks like a signature, but when gpg parses it, it turns out to be bogus). So we should probably continue ignoring SIGPIPE to be on the safe side.

But I notice that we already handle SIGPIPE explicitly in sign_buffer() for similar reasons. What I was wondering earlier was whether we should teach other functions that call gpg (like verify_signed_buffer()) to ignore SIGPIPE, too, so that we can return a reasonable error value rather than just killing the whole program.

-Peff
Santiago Torres· Mar 24, 2016, 22:32 UTC · re: Jeff King · lore

Re: [PATCH/RFC] builtin/tag.c: move PGP verification inside builtin.

Show 7 quoted lines
> > This is my first stab at this, in the dumbest/simplest way imaginable. I
> > don't like that there is no code reuse (the run_gpg_verify function is
> > repeated here and in the plumbing command). I would appreciate pointers
> > on what would be the best way to avoid this.
> 
> It looks to me like you could factor the repeated code into a common
> verify_tag(), but maybe I am missing something.
I see, yes. This looks like the way forward.
Show 28 quoted lines
> 
> > I also spent quite some time figuring out what you meant with
> > 
> > > Do note the trickery with SIGPIPE in verify-tag, though. We probably
> > > need to do the same here (in fact, I wonder if that should be pushed
> > > down into the code that calls gpg).
> > I don't see any explicit SIGPIPE trickery here. Any pointers?
> 
> There is a call to ignore SIGPIPE in builtin/verify-tag.c, line 100.
> Do we need to do be doing the same thing here?
> 
> There's some discussion in the thread starting at:
> 
>   http://thread.gmane.org/gmane.comp.version-control.git/53878/focus=53904
> 
> The claim there is that we get SIGPIPE and die early if we feed gpg a
> tag which isn't signed. We _should_ be catching that case already via
> parse_signature(), though I wonder if it can be fooled (e.g., something
> that looks like a signature, but when gpg parses it, it turns out to be
> bogus). So we should probably continue ignoring SIGPIPE to be on the
> safe side.
> 
> But I notice that we already handle SIGPIPE explicitly in sign_buffer()
> for similar reasons.  What I was wondering earlier was whether we should
> teach other functions that call gpg (like verify_signed_buffer()) to
> ignore SIGPIPE, too, so that we can return a reasonable error value
> rather than just killing the whole program.
>

Now I get it I think this should be easy to achieve by moving verify_tag() to tag.c, along with the static run_gpg_verify functions. I could move the SIGPIPE call inside the verify-tag command and patch up everything accordingly. Does this sound ok?

Thanks, -Santiago.

Jeff King· Mar 24, 2016, 23:27 UTC · re: Santiago Torres · lore

Re: [PATCH/RFC] builtin/tag.c: move PGP verification inside builtin.

On Thu, Mar 24, 2016 at 06:32:58PM -0400, Santiago Torres wrote:
Show 8 quoted lines
> > But I notice that we already handle SIGPIPE explicitly in sign_buffer()
> > for similar reasons.  What I was wondering earlier was whether we should
> > teach other functions that call gpg (like verify_signed_buffer()) to
> > ignore SIGPIPE, too, so that we can return a reasonable error value
> > rather than just killing the whole program.
> 
> Now I get it  I think this should be easy to achieve by moving
> verify_tag() to tag.c, along with the static run_gpg_verify functions.
Exactly.
> I could move the SIGPIPE call inside the verify-tag command and patch up
> everything accordingly. Does this sound ok?
I think that works, but take note of two things:
  - convert it to sigchain_push(), and make sure you sigchain_pop() it
    when you are done, so that the caller retains their original SIGPIPE
    behavior after the function returns. See the example in
    sign_buffer().
  - you should probably do it as close to the gpg call as possible, so
    as to affect as little code as possible. So probably in
    verify_signed_buffer(), not in verify_tag().
-Peff
Jeff King· Mar 24, 2016, 22:10 UTC · re: santiago@nyu.edu · lore

Re: [PATCH/RFC] builtin/tag.c: move PGP verification inside builtin.

On Thu, Mar 24, 2016 at 05:39:20PM -0400, santiago@nyu.edu wrote:
Show 9 quoted lines
> +static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
> +{
> +	struct signature_check sigc;
> +	int len;
> +	int ret;
> +
> +	memset(&sigc, 0, sizeof(sigc));
> +
> +	len = parse_signature(buf, size);

I know you are just copying this from the one in builtin/verify-tag.c, but I find the use of "size" and "len" for two different purposes confusing. Those words are synonyms, so how do the variables differ?

Perhaps "payload_size", or "signature_offset" would be a better term for "len".

> +	if (size == len) {
> +		write_in_full(1, buf, len);
> +	}

If the two are the same, we have no signature. Should we be returning early, and skipping check_signature() in that case?

Show 6 quoted lines
> +	ret = check_signature(buf, len, buf + len, size - len, &sigc);
> +	print_signature_buffer(&sigc, flags);
> +
> +	signature_check_clear(&sigc);
> +	return ret;
> +}
This part looks OK.
Show 6 quoted lines
> @@ -104,13 +125,24 @@ static int delete_tag(const char *name, const char *ref,
>  static int verify_tag(const char *name, const char *ref,
>  				const unsigned char *sha1)
>  {
> -	const char *argv_verify_tag[] = {"verify-tag",
> -					"-v", "SHA1_HEX", NULL};

So the original was passing "-v" to verify-tag. That should put GPG_VERIFY_VERBOSE into the flags field. But later:

> +	ret = run_gpg_verify(buf, size, 0);

We don't pass any flags. Shouldn't this unconditionally pass GPG_VERIFY_VERBOSE?

Show 17 quoted lines
> +	enum object_type type;
> +	unsigned long size;
> +	const char* buf;
> +	int ret;
> +
> +	type = sha1_object_info(sha1, NULL);
> +	if (type != OBJ_TAG)
> +		return error("%s: cannot verify a non-tag object of type %s.",
> +				name, typename(type));
> +
> +	buf = read_sha1_file(sha1, &type, &size);
> +	if (!buf)
> +		return error("%s: unable to read file.", name);
> +
> +	ret = run_gpg_verify(buf, size, 0);
> +
> +	return ret;

All of this seems like a repetition of verify_tag() in builtin/verify-tag.c (and ditto with run_gpg_verify()). Can we move those functions into tag.c and just call them from both places, or is there some difference that needs to be taken into account (and if the latter, can we refactor them to account for the differences?).

-Peff
Santiago Torres· Mar 24, 2016, 22:24 UTC · re: Jeff King · lore

Re: [PATCH/RFC] builtin/tag.c: move PGP verification inside builtin.

Show 6 quoted lines
> I know you are just copying this from the one in builtin/verify-tag.c,
> but I find the use of "size" and "len" for two different purposes
> confusing. Those words are synonyms, so how do the variables differ?
> 
> Perhaps "payload_size", or "signature_offset" would be a better term for
> "len".
I agree, I'll give this a go.
Show 7 quoted lines
> 
> > +	if (size == len) {
> > +		write_in_full(1, buf, len);
> > +	}
> 
> If the two are the same, we have no signature. Should we be returning
> early, and skipping check_signature() in that case?

This makes sense, for both the builtin and the plumbing. Let me give this a try.
Show 15 quoted lines
> > @@ -104,13 +125,24 @@ static int delete_tag(const char *name, const char *ref,
> >  static int verify_tag(const char *name, const char *ref,
> >  				const unsigned char *sha1)
> >  {
> > -	const char *argv_verify_tag[] = {"verify-tag",
> > -					"-v", "SHA1_HEX", NULL};
> 
> So the original was passing "-v" to verify-tag. That should put
> GPG_VERIFY_VERBOSE into the flags field. But later:
> 
> > +	ret = run_gpg_verify(buf, size, 0);
> 
> We don't pass any flags. Shouldn't this unconditionally pass
> GPG_VERIFY_VERBOSE?
> 
Right, I missed this. Sorry about this.
Show 6 quoted lines
> All of this seems like a repetition of verify_tag() in
> builtin/verify-tag.c (and ditto with run_gpg_verify()). Can we move
> those functions into tag.c and just call them from both places, or is
> there some difference that needs to be taken into account (and if the
> latter, can we refactor them to account for the differences?).
>

Yep, this is what was troubling me (as I mentioned on the followup). I didn't want to remove the "static" classifier for the function (as there could be a major reason for this decision).

If this last chage is ok with you I can send the fixed-up version right away.

Thanks! -Santiago.

← back to recent threads