threads / patch / 41821

patchtag.c: move PGP verification code from plumbing

Subject: [PATCH] tag.c: move PGP verification code from plumbing

## tl;dr

5 messages between Mar 25, 2016 and Mar 26, 2016. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

santiago@nyu.edu· Mar 25, 2016, 00:33 UTC · lore
From: Santiago Torres <santiago@nyu.edu>

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

To do this, the run_pgp_verify() and verify_tag() functions are moved to tag.c. The definition of verify_tag was changed to support extra arguments that match the builtin/tag and builtin/verify-tag modules. The SIGPIPE ignore call in tag-verify was also moved to a more sensible place, now that both modules need it.

The function name was also changed to pgp_verify_tag to avoid conflicts with mktag.c's.

Signed-off-by: Santiago Torres <santiago@nyu.edu>
---
 builtin/tag.c        | 26 ++++++++---------------
 builtin/verify-tag.c | 60 ++++++----------------------------------------------
 gpg-interface.c      |  6 ++++++
 tag.c                | 48 +++++++++++++++++++++++++++++++++++++++++
 tag.h                |  2 ++
 5 files changed, 72 insertions(+), 70 deletions(-)
Show changes to 5 files +72 −70

builtin/tag.c, builtin/verify-tag.c, gpg-interface.c, tag.c, tag.h

diff --git a/builtin/tag.c b/builtin/tag.c
index 1705c94..af8f3ba 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);
 
-static int for_each_tag_name(const char **argv, each_tag_name_fn fn)
+static int for_each_tag_name(const char **argv, each_tag_name_fn fn,
+		unsigned flags)
 {
 	const char **p;
 	char ref[PATH_MAX];
@@ -86,32 +87,22 @@ static int for_each_tag_name(const char **argv, each_tag_name_fn fn)
 			had_error = 1;
 			continue;
 		}
-		if (fn(*p, ref, sha1))
+		if (fn(*p, ref, sha1, flags))
 			had_error = 1;
 	}
 	return had_error;
 }
 
 static int delete_tag(const char *name, const char *ref,
-				const unsigned char *sha1)
+				const unsigned char *sha1, unsigned flags)
 {
-	if (delete_ref(ref, sha1, 0))
+	if (delete_ref(ref, sha1, flags))
 		return 1;
 	printf(_("Deleted tag '%s' (was %s)\n"), name, find_unique_abbrev(sha1, DEFAULT_ABBREV));
 	return 0;
 }
 
-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;
-}
 
 static int do_sign(struct strbuf *buffer)
 {
@@ -424,9 +415,10 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	if (filter.merge_commit)
 		die(_("--merged and --no-merged option are only allowed with -l"));
 	if (cmdmode == 'd')
-		return for_each_tag_name(argv, delete_tag);
+		return for_each_tag_name(argv, delete_tag, 0);
 	if (cmdmode == 'v')
-		return for_each_tag_name(argv, verify_tag);
+		return for_each_tag_name(argv, pgp_verify_tag,
+				GPG_VERIFY_VERBOSE);
 
 	if (msg.given || msgfile) {
 		if (msg.given && msgfile)
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index 00663f6..2e6a175 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -18,55 +18,6 @@ static const char * const verify_tag_usage[] = {
 		NULL
 };
 
-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) {
-		if (flags & GPG_VERIFY_VERBOSE)
-			write_in_full(1, buf, len);
-		return error("no signature found");
-	}
-
-	ret = check_signature(buf, len, buf + len, size - len, &sigc);
-	print_signature_buffer(&sigc, flags);
-
-	signature_check_clear(&sigc);
-	return ret;
-}
-
-static int verify_tag(const char *name, unsigned flags)
-{
-	enum object_type type;
-	unsigned char sha1[20];
-	char *buf;
-	unsigned long size;
-	int ret;
-
-	if (get_sha1(name, sha1))
-		return error("tag '%s' not found.", name);
-
-	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, flags);
-
-	free(buf);
-	return ret;
-}
-
 static int git_verify_tag_config(const char *var, const char *value, void *cb)
 {
 	int status = git_gpg_config(var, value, cb);
@@ -78,6 +29,8 @@ static int git_verify_tag_config(const char *var, const char *value, void *cb)
 int cmd_verify_tag(int argc, const char **argv, const char *prefix)
 {
 	int i = 1, verbose = 0, had_error = 0;
+	unsigned char sha1[20];
+	const char *name;
 	unsigned flags = 0;
 	const struct option verify_tag_options[] = {
 		OPT__VERBOSE(&verbose, N_("print tag contents")),
@@ -95,11 +48,12 @@ 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))
+		name = argv[i++];
+		if (get_sha1(name, sha1))
+			return error("tag '%s' not found.", name);
+
+		if (pgp_verify_tag(NULL, NULL, sha1, flags))
 			had_error = 1;
 	return had_error;
 }
diff --git a/gpg-interface.c b/gpg-interface.c
index 3dc2fe3..1b421b7 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -232,6 +232,11 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
 	if (gpg_output)
 		gpg.err = -1;
 	args_gpg[3] = path;
+
+	/* sometimes the program was terminated because this signal
+	 * was received in the process of writing the gpg input.
+	 * We ignore it for this call and restore it afterwards */
+	sigchain_push(SIGPIPE, SIG_IGN);
 	if (start_command(&gpg)) {
 		unlink(path);
 		return error(_("could not run gpg."));
@@ -250,6 +255,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);
 
diff --git a/tag.c b/tag.c
index d72f742..7097a84 100644
--- a/tag.c
+++ b/tag.c
@@ -3,9 +3,57 @@
 #include "commit.h"
 #include "tree.h"
 #include "blob.h"
+#include "sigchain.h"
 
 const char *tag_type = "tag";
 
+
+static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
+{
+	struct signature_check sigc;
+	int payload_size;
+	int ret;
+
+	memset(&sigc, 0, sizeof(sigc));
+
+	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!");
+	}
+
+	ret = check_signature(buf, payload_size, buf + payload_size,
+			size - payload_size, &sigc);
+	print_signature_buffer(&sigc, flags);
+
+	signature_check_clear(&sigc);
+	return ret;
+}
+
+int pgp_verify_tag(const char *name, const char *ref,
+		const unsigned char *sha1, unsigned flags)
+{
+
+	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, flags);
+
+	return ret;
+}
+
 struct object *deref_tag(struct object *o, const char *warn, int warnlen)
 {
 	while (o && o->type == OBJ_TAG)
diff --git a/tag.h b/tag.h
index f4580ae..22289a5 100644
--- a/tag.h
+++ b/tag.h
@@ -17,5 +17,7 @@ extern int parse_tag_buffer(struct tag *item, const void *data, unsigned long si
 extern int parse_tag(struct tag *item);
 extern struct object *deref_tag(struct object *, const char *, int);
 extern struct object *deref_tag_noverify(struct object *);
+extern int pgp_verify_tag(const char *name, const char *ref,
+		const unsigned char *sha1, unsigned flags);
 
 #endif /* TAG_H */
-- 
2.7.3
Eric Sunshine· Mar 25, 2016, 05:23 UTC · re: santiago@nyu.edu · lore

Re: [PATCH] tag.c: move PGP verification code from plumbing

On Thu, Mar 24, 2016 at 8:33 PM,  <santiago@nyu.edu> wrote:
> The verify tag function is just a thin wrapper around the verify-tag
> command. We can avoid one fork call by doing the verification inside
> the tag builtin instead.

Hopefully, the below review comments are meaningful, however, aside from having just read Peff's review of the previous version of this patch, I haven't been following this discussion, so it's possible some comments may be off the mark. Caveat emptor.

Show 5 quoted lines
> To do this, the run_pgp_verify() and verify_tag() functions are moved to
> tag.c. The definition of verify_tag was changed to support extra
> arguments that match the builtin/tag and builtin/verify-tag modules. The
> SIGPIPE ignore call in tag-verify was also moved to a more sensible
> place, now that both modules need it.

This patch is doing too much, thus making it difficult to review and reason about. Based upon this paragraph alone, you may want to split the patch into three or more patches. At the very least, have a patch which does the code movement (and nothing else); one which adjusts the argument lists; and one which adjusts SIGPIPE handling. More generally, figure out the distinct conceptual changes being made here, and give each such change its own patch.

Show 5 quoted lines
> The function name was also changed to pgp_verify_tag to avoid conflicts with
> mktag.c's.
>
> Signed-off-by: Santiago Torres <santiago@nyu.edu>
> ---

Right here below the "---" line is where, for the sake of reviewers, you would explain what changed since the previous version. Also, as a reviewer aid, provide a link to the previous discussion, like this[1]. Finally, indicate the version number of the patch in the subject, like this [PATCH v3] (see git-format-patch's -v option).

More below...
[1]: http://thread.gmane.org/gmane.comp.version-control.git/289803
Show 17 quoted lines
> diff --git a/builtin/tag.c b/builtin/tag.c
> @@ -86,32 +87,22 @@ static int for_each_tag_name(const char **argv, each_tag_name_fn fn)
>         return 0;
>  }
>
> -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;
> -}
>

This deletion inconsistently leaves an extra blank line between functions; thus you'd want to delete one more (blank) line.

Show 18 quoted lines
>  static int do_sign(struct strbuf *buffer)
>  {
> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
> @@ -95,11 +48,12 @@ 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))
> +               name = argv[i++];
> +               if (get_sha1(name, sha1))
> +                       return error("tag '%s' not found.", name);
> +
> +               if (pgp_verify_tag(NULL, NULL, sha1, flags))
>                         had_error = 1;

Meh, this isn't Python. Due to the missing braces, the only thing inside the while() loop is the assignment to 'name'; all the other indented code is outside the while().

Did you run the test suite following this change? Did it all pass? If so, perhaps an additional test or two to catch this sort of error would be warranted.

Show 11 quoted lines
>         return had_error;
>  }
> diff --git a/gpg-interface.c b/gpg-interface.c
> @@ -232,6 +232,11 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
>         if (gpg_output)
>                 gpg.err = -1;
>         args_gpg[3] = path;
> +
> +       /* sometimes the program was terminated because this signal
> +        * was received in the process of writing the gpg input.
> +        * We ignore it for this call and restore it afterwards */

I realize that the bulk of this comment block was merely relocated from builtin/verify-tag.c, however, now would be a good time to fix its style violation and format it like this:

    /*
     * This is a multi-line
     * comment.
     */

Also, the last line of the comment, which you added when relocating it, merely repeats what the code itself already says clearly, thus is not particularly useful and should be dropped.

Show 11 quoted lines
> +       sigchain_push(SIGPIPE, SIG_IGN);
>         if (start_command(&gpg)) {
>                 unlink(path);
>                 return error(_("could not run gpg."));
> @@ -250,6 +255,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);
Jeff King· Mar 25, 2016, 05:31 UTC · re: Eric Sunshine · lore

Re: [PATCH] tag.c: move PGP verification code from plumbing

On Fri, Mar 25, 2016 at 01:23:57AM -0400, Eric Sunshine wrote:
Show 9 quoted lines
> On Thu, Mar 24, 2016 at 8:33 PM,  <santiago@nyu.edu> wrote:
> > The verify tag function is just a thin wrapper around the verify-tag
> > command. We can avoid one fork call by doing the verification inside
> > the tag builtin instead.
> 
> Hopefully, the below review comments are meaningful, however, aside
> from having just read Peff's review of the previous version of this
> patch, I haven't been following this discussion, so it's possible some
> comments may be off the mark. Caveat emptor.

Thanks for reviewing. I agree with all of the comments you made, but I'd add a little more to the last one:

Show 17 quoted lines
> > +
> > +       /* sometimes the program was terminated because this signal
> > +        * was received in the process of writing the gpg input.
> > +        * We ignore it for this call and restore it afterwards */
> 
> I realize that the bulk of this comment block was merely relocated
> from builtin/verify-tag.c, however, now would be a good time to fix
> its style violation and format it like this:
> 
>     /*
>      * This is a multi-line
>      * comment.
>      */
> 
> Also, the last line of the comment, which you added when relocating
> it, merely repeats what the code itself already says clearly, thus is
> not particularly useful and should be dropped.

I actually think we can drop this comment entirely. Pushing and popping SIGPIPE when piping to a sub-program like this is not that exotic in our code base. And if the SIGPIPE handling here is done in its own patch, then if somebody wants to see more discussion or reasoning, they can go to its commit message (which can then go into more detail about why we might see SIGPIPE, and not just a vague "sometimes...").

-Peff
Santiago Torres· Mar 25, 2016, 14:45 UTC · re: Eric Sunshine · lore

Re: [PATCH] tag.c: move PGP verification code from plumbing

Show 17 quoted lines
> > -       signal(SIGPIPE, SIG_IGN);
> >         while (i < argc)
> > -               if (verify_tag(argv[i++], flags))
> > +               name = argv[i++];
> > +               if (get_sha1(name, sha1))
> > +                       return error("tag '%s' not found.", name);
> > +
> > +               if (pgp_verify_tag(NULL, NULL, sha1, flags))
> >                         had_error = 1;
> 
> Meh, this isn't Python. Due to the missing braces, the only thing
> inside the while() loop is the assignment to 'name'; all the other
> indented code is outside the while().
> 
> Did you run the test suite following this change? Did it all pass? If
> so, perhaps an additional test or two to catch this sort of error
> would be warranted.

Wow, you're right! I just re-ran the tests again to make sure I didn't miss anything. All the tests pass for me, so I'll write an extra case to avoid this. Just to be sure, I should include it in t7030-verify-tag.sh right?

All your other comments seem straightforward and on point. I'll apply them right away :)

Thanks! -Santiago.

Eric Sunshine· Mar 26, 2016, 06:33 UTC · re: Santiago Torres · lore

Re: [PATCH] tag.c: move PGP verification code from plumbing

On Fri, Mar 25, 2016 at 10:45 AM, Santiago Torres <santiago@nyu.edu> wrote:
Show 21 quoted lines
>> >         while (i < argc)
>> > -               if (verify_tag(argv[i++], flags))
>> > +               name = argv[i++];
>> > +               if (get_sha1(name, sha1))
>> > +                       return error("tag '%s' not found.", name);
>> > +
>> > +               if (pgp_verify_tag(NULL, NULL, sha1, flags))
>> >                         had_error = 1;
>>
>> Meh, this isn't Python. Due to the missing braces, the only thing
>> inside the while() loop is the assignment to 'name'; all the other
>> indented code is outside the while().
>>
>> Did you run the test suite following this change? Did it all pass? If
>> so, perhaps an additional test or two to catch this sort of error
>> would be warranted.
>
> Wow, you're right! I just re-ran the tests again to make sure I didn't
> miss anything. All the tests pass for me, so I'll write an extra case to
> avoid this. Just to be sure, I should include it in t7030-verify-tag.sh
> right?

Generally speaking, it is desirable to have test coverage for all functionality you're refactoring to ensure that the refactoring doesn't break that functionality.

t7030-verify-tag.sh indeed seems like a good place to add a new test for catching this sort of regression.

t7004-tag.sh is also of interest since, in an earlier version of this patch, if I recall correctly, Peff caught a regression where "git tag -v" failed to pass the GPG_VERIFY_VERBOSE flag, and I don't think t7004-tag.sh would have caught that problem either, so a new test for that would be good, as well.

Speaking of GPG_VERIFY_VERBOSE, now that I'm examining the changes more closely, it appears that this version of the patch no longer respects GPG_VERIFY_VERBOSE at all but is instead hard-coded to be verbose. Is that desirable or intentional?

← back to recent threads