[v2,18/24] gpg-interface: improve interface for parsing tags
diff mbox series

Message ID 20200222201749.937983-19-sandals@crustytoothpaste.net
State New
Headers show
Series
  • SHA-256 stage 4 implementation, part 1/3
Related show

Commit Message

brian m. carlson Feb. 22, 2020, 8:17 p.m. UTC
We have a function which parses a buffer with a signature at the end,
parse_signature, and this function is used for signed tags.  However,
the transition plan has SHA-256 tags using a header, which is a
materially different syntax.  The current interface is not suitable for
parsing such tags.

Adjust the parse_signature interface to store the parsed data in two
strbufs and turn the existing function into parse_signed_buffer.  The
latter is still used in places where we want to strip off the signature
in a SHA-1 tag or in places where we know we always have a signed
buffer, such as push certs.

Adjust all the callers to deal with this new interface.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 builtin/fmt-merge-msg.c | 26 ++++++++++++++++++--------
 builtin/receive-pack.c  |  4 ++--
 builtin/tag.c           | 16 ++++++++++++----
 commit.c                |  9 ++++++---
 gpg-interface.c         | 13 ++++++++++++-
 gpg-interface.h         |  9 ++++++++-
 log-tree.c              | 14 ++++++++------
 ref-filter.c            | 18 ++++++++++++++----
 tag.c                   | 15 ++++++++-------
 9 files changed, 88 insertions(+), 36 deletions(-)

Comments

Junio C Hamano Feb. 24, 2020, 6:26 p.m. UTC | #1
"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> We have a function which parses a buffer with a signature at the end,
> parse_signature, and this function is used for signed tags.  However,
> the transition plan has SHA-256 tags using a header, which is a
> materially different syntax.  The current interface is not suitable for
> parsing such tags.
>
> Adjust the parse_signature interface to store the parsed data in two
> strbufs and turn the existing function into parse_signed_buffer.  The
> latter is still used in places where we want to strip off the signature
> in a SHA-1 tag or in places where we know we always have a signed
> buffer, such as push certs.
>
> Adjust all the callers to deal with this new interface.
>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
>  builtin/fmt-merge-msg.c | 26 ++++++++++++++++++--------
>  builtin/receive-pack.c  |  4 ++--
>  builtin/tag.c           | 16 ++++++++++++----
>  commit.c                |  9 ++++++---
>  gpg-interface.c         | 13 ++++++++++++-
>  gpg-interface.h         |  9 ++++++++-
>  log-tree.c              | 14 ++++++++------
>  ref-filter.c            | 18 ++++++++++++++----
>  tag.c                   | 15 ++++++++-------
>  9 files changed, 88 insertions(+), 36 deletions(-)
>
> diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
> index 05a92c59d8..29f647e2d9 100644
> --- a/builtin/fmt-merge-msg.c
> +++ b/builtin/fmt-merge-msg.c
> @@ -472,6 +472,7 @@ static void fmt_tag_signature(struct strbuf *tagbuf,
>  			      const char *buf,
>  			      unsigned long len)
>  {
> +
>  	const char *tag_body = strstr(buf, "\n\n");
>  	if (tag_body) {
>  		tag_body += 2;

Is this hunk a pun to rhyme with the strstr ;-)?

> diff --git a/gpg-interface.c b/gpg-interface.c
> index 2d538bcd6e..b25f5c21d8 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -345,7 +345,7 @@ void print_signature_buffer(const struct signature_check *sigc, unsigned flags)
>  		fputs(output, stderr);
>  }
>  
> -size_t parse_signature(const char *buf, size_t size)
> +size_t parse_signed_buffer(const char *buf, size_t size)
>  {
>  	size_t len = 0;
>  	size_t match = size;
> @@ -361,6 +361,17 @@ size_t parse_signature(const char *buf, size_t size)
>  	return match;
>  }
>  
> +int parse_signature(const char *buf, size_t size, struct strbuf *payload, struct strbuf *signature)
> +{
> +	size_t match = parse_signed_buffer(buf, size);
> +	if (match != size) {
> +		strbuf_add(payload, buf, match);
> +		strbuf_add(signature, buf + match, size - match);
> +		return 1;
> +	}
> +	return 0;
> +}
> +

Makes sense.
Johannes Schindelin Feb. 25, 2020, 10:29 a.m. UTC | #2
Hi brian,

On Sat, 22 Feb 2020, brian m. carlson wrote:

> diff --git a/ref-filter.c b/ref-filter.c
> index 6867e33648..212f1165bb 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1161,7 +1161,13 @@ static void find_subpos(const char *buf,
>  			unsigned long *nonsiglen,
>  			const char **sig, unsigned long *siglen)
>  {
> +	struct strbuf payload = STRBUF_INIT;
> +	struct strbuf signature = STRBUF_INIT;
>  	const char *eol;
> +	const char *end = buf + strlen(buf);
> +	const char *sigstart;
> +
> +
>  	/* skip past header until we hit empty line */
>  	while (*buf && *buf != '\n') {
>  		eol = strchrnul(buf, '\n');
> @@ -1174,13 +1180,14 @@ static void find_subpos(const char *buf,
>  		buf++;
>
>  	/* parse signature first; we might not even have a subject line */
> -	*sig = buf + parse_signature(buf, strlen(buf));
> -	*siglen = strlen(*sig);
> +	parse_signature(buf, end - buf, &payload, &signature);
> +	*sig = strbuf_detach(&signature, siglen);

While I like the spirit of this patch, it makes the Windows build fail. I
put this on top of Git for Windows' `shears/pu` branch to fix it (maybe
you could adopt a variation of it?):

-- snipsnap --
Subject: [PATCH] fixup??? gpg-interface: improve interface for parsing tags

In 3f69139fa39 (gpg-interface: improve interface for parsing tags,
2020-02-22), we introduce a call to `strbuf_detach()`. The second
parameter of that function takes a pointer to the variable where the
length of the string should be stored.

However, `strbuf_detach()` uses the 21st century data type `size_t`,
while the existing code in `find_subpos()` uses the `unsigned long` data
type that the 1980/1980 so desperately want back.

Unsurprisingly, this causes problems with the Windows build, where
`sizeof(unsigned long) == 4` even in 64-bit builds (which, I feel the
need to add, is totally legitimate).

We should probably change the data type to the correct one in a
preparatory patch, before improving the interface for parsing tags.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 ref-filter.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 164ab62d15e..bab34b9ef74 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1156,10 +1156,10 @@ static void grab_person(const char *who, struct atom_value *val, int deref, void
 }

 static void find_subpos(const char *buf,
-			const char **sub, unsigned long *sublen,
-			const char **body, unsigned long *bodylen,
-			unsigned long *nonsiglen,
-			const char **sig, unsigned long *siglen)
+			const char **sub, size_t *sublen,
+			const char **body, size_t *bodylen,
+			size_t *nonsiglen,
+			const char **sig, size_t *siglen)
 {
 	struct strbuf payload = STRBUF_INIT;
 	struct strbuf signature = STRBUF_INIT;
@@ -1234,7 +1234,7 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf)
 {
 	int i;
 	const char *subpos = NULL, *bodypos = NULL, *sigpos = NULL;
-	unsigned long sublen = 0, bodylen = 0, nonsiglen = 0, siglen = 0;
+	size_t sublen = 0, bodylen = 0, nonsiglen = 0, siglen = 0;

 	for (i = 0; i < used_atom_cnt; i++) {
 		struct used_atom *atom = &used_atom[i];
--
2.25.1.windows.1
Junio C Hamano Feb. 25, 2020, 7:25 p.m. UTC | #3
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>>  	/* parse signature first; we might not even have a subject line */
>> -	*sig = buf + parse_signature(buf, strlen(buf));
>> -	*siglen = strlen(*sig);
>> +	parse_signature(buf, end - buf, &payload, &signature);
>> +	*sig = strbuf_detach(&signature, siglen);
>
> While I like the spirit of this patch, it makes the Windows build fail. I
> put this on top of Git for Windows' `shears/pu` branch to fix it (maybe
> you could adopt a variation of it?):

FWIW, it's of course not just Windows, but on anything whose size_t
!= ulong, e.g. https://travis-ci.org/git/git/jobs/654661788 (Linux32)
brian m. carlson Feb. 26, 2020, 2:23 a.m. UTC | #4
On 2020-02-25 at 10:29:26, Johannes Schindelin wrote:
> Hi brian,
> 
> On Sat, 22 Feb 2020, brian m. carlson wrote:
> 
> > diff --git a/ref-filter.c b/ref-filter.c
> > index 6867e33648..212f1165bb 100644
> > --- a/ref-filter.c
> > +++ b/ref-filter.c
> > @@ -1161,7 +1161,13 @@ static void find_subpos(const char *buf,
> >  			unsigned long *nonsiglen,
> >  			const char **sig, unsigned long *siglen)
> >  {
> > +	struct strbuf payload = STRBUF_INIT;
> > +	struct strbuf signature = STRBUF_INIT;
> >  	const char *eol;
> > +	const char *end = buf + strlen(buf);
> > +	const char *sigstart;
> > +
> > +
> >  	/* skip past header until we hit empty line */
> >  	while (*buf && *buf != '\n') {
> >  		eol = strchrnul(buf, '\n');
> > @@ -1174,13 +1180,14 @@ static void find_subpos(const char *buf,
> >  		buf++;
> >
> >  	/* parse signature first; we might not even have a subject line */
> > -	*sig = buf + parse_signature(buf, strlen(buf));
> > -	*siglen = strlen(*sig);
> > +	parse_signature(buf, end - buf, &payload, &signature);
> > +	*sig = strbuf_detach(&signature, siglen);
> 
> While I like the spirit of this patch, it makes the Windows build fail. I
> put this on top of Git for Windows' `shears/pu` branch to fix it (maybe
> you could adopt a variation of it?):

I'm happy to squash this in.  Sorry for the breakage, and thanks for
catching this.
brian m. carlson Feb. 26, 2020, 3:05 a.m. UTC | #5
On 2020-02-25 at 19:25:46, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >>  	/* parse signature first; we might not even have a subject line */
> >> -	*sig = buf + parse_signature(buf, strlen(buf));
> >> -	*siglen = strlen(*sig);
> >> +	parse_signature(buf, end - buf, &payload, &signature);
> >> +	*sig = strbuf_detach(&signature, siglen);
> >
> > While I like the spirit of this patch, it makes the Windows build fail. I
> > put this on top of Git for Windows' `shears/pu` branch to fix it (maybe
> > you could adopt a variation of it?):
> 
> FWIW, it's of course not just Windows, but on anything whose size_t
> != ulong, e.g. https://travis-ci.org/git/git/jobs/654661788 (Linux32)

I will likely not get to a reroll before Thursday at the earliest due to
other commitments, so if it's more convenient for you to eject this from
pu and just pick up v3 when I get around to it, please feel free.
Junio C Hamano Feb. 26, 2020, 3:11 a.m. UTC | #6
"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> I will likely not get to a reroll before Thursday at the earliest due to
> other commitments, so if it's more convenient for you to eject this from
> pu and just pick up v3 when I get around to it, please feel free.

Thanks, but I think a minimal SQUASH??? I added while queuing to
'pu' seems to have made it pass so that would have to do for now.
Johannes Schindelin Feb. 27, 2020, 1:24 p.m. UTC | #7
Hi brian,

On Wed, 26 Feb 2020, brian m. carlson wrote:

> On 2020-02-25 at 10:29:26, Johannes Schindelin wrote:
> >
> > On Sat, 22 Feb 2020, brian m. carlson wrote:
> >
> > > diff --git a/ref-filter.c b/ref-filter.c
> > > index 6867e33648..212f1165bb 100644
> > > --- a/ref-filter.c
> > > +++ b/ref-filter.c
> > > @@ -1161,7 +1161,13 @@ static void find_subpos(const char *buf,
> > >  			unsigned long *nonsiglen,
> > >  			const char **sig, unsigned long *siglen)
> > >  {
> > > +	struct strbuf payload = STRBUF_INIT;
> > > +	struct strbuf signature = STRBUF_INIT;
> > >  	const char *eol;
> > > +	const char *end = buf + strlen(buf);
> > > +	const char *sigstart;
> > > +
> > > +
> > >  	/* skip past header until we hit empty line */
> > >  	while (*buf && *buf != '\n') {
> > >  		eol = strchrnul(buf, '\n');
> > > @@ -1174,13 +1180,14 @@ static void find_subpos(const char *buf,
> > >  		buf++;
> > >
> > >  	/* parse signature first; we might not even have a subject line */
> > > -	*sig = buf + parse_signature(buf, strlen(buf));
> > > -	*siglen = strlen(*sig);
> > > +	parse_signature(buf, end - buf, &payload, &signature);
> > > +	*sig = strbuf_detach(&signature, siglen);
> >
> > While I like the spirit of this patch, it makes the Windows build fail. I
> > put this on top of Git for Windows' `shears/pu` branch to fix it (maybe
> > you could adopt a variation of it?):
>
> I'm happy to squash this in.  Sorry for the breakage, and thanks for
> catching this.

You're welcome, but credit for catching it should go to Azure Pipelines
;-)

To be honest, I am rather happy how these CI builds help us catch things
already when they are in `pu`. _Quite_ happy.

Ciao,
Dscho
Junio C Hamano Feb. 27, 2020, 3:06 p.m. UTC | #8
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> You're welcome, but credit for catching it should go to Azure Pipelines
> ;-)
>
> To be honest, I am rather happy how these CI builds help us catch things
> already when they are in `pu`. _Quite_ happy.

Oh, you do not have to be honest for that---you show that all the
time ;-)

And I am happy that I can throw a new topic to 'pu' and CI will
catch issues before I even look at it seriously to consider if it is
worth keeping it.

One sad thing about Azure Pipelines thing is that, as far as I
recall, we hear about breakages it discovered only from you and
nobody else, while we hear about breakages discovered by Travis by
at least multiple people, and that is *not* because the coverage is
wider or report is more accesible and easier to read there.  It's
that our CI at Travis forced awareness of its existence on us by
simply being the only one out there for a long time.

We are bound to add issues to the tip of 'pu' (e.g. 64-vs-32,
bytesex, or GNUisms) that are easy to discover by building on
different platforms and in different configurations every once in a
while.  We should advertise more to gain awareness so that other
people can also notice breakages Travis's limited coverage may have
missed even when you are on vacation.

If you want quantitative measure of success ;-) (sorry, I couldn't
resist as it has been Perf period around here), I want to see at
least two people outside Microsoft to notice and report breakages
first found by Azure Pipelines CI in the coming 6 months.  How can
we help make that happen?

Thanks.

Patch
diff mbox series

diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 05a92c59d8..29f647e2d9 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -472,6 +472,7 @@  static void fmt_tag_signature(struct strbuf *tagbuf,
 			      const char *buf,
 			      unsigned long len)
 {
+
 	const char *tag_body = strstr(buf, "\n\n");
 	if (tag_body) {
 		tag_body += 2;
@@ -492,24 +493,31 @@  static void fmt_merge_msg_sigs(struct strbuf *out)
 	for (i = 0; i < origins.nr; i++) {
 		struct object_id *oid = origins.items[i].util;
 		enum object_type type;
-		unsigned long size, len;
-		char *buf = read_object_file(oid, &type, &size);
+		unsigned long size;
+		char *buf = read_object_file(oid, &type, &size), *orig = buf;
 		struct signature_check sigc = { 0 };
+		struct strbuf payload = STRBUF_INIT;
+		struct strbuf signature = STRBUF_INIT;
 		struct strbuf sig = STRBUF_INIT;
+		size_t len = size;
 
 		if (!buf || type != OBJ_TAG)
 			goto next;
-		len = parse_signature(buf, size);
-
-		if (size == len)
-			; /* merely annotated */
-		else if (!check_signature(buf, len, buf + len, size - len,
+		if (!parse_signature(buf, size, &payload, &signature))
+			len = size; /* merely annotated */
+		else if (!check_signature(payload.buf, payload.len,
+					  signature.buf, signature.len,
 					  &sigc)) {
 			strbuf_addstr(&sig, sigc.gpg_output);
 			signature_check_clear(&sigc);
 		} else
 			strbuf_addstr(&sig, "gpg verification failed.\n");
 
+		if (payload.len) {
+			buf = payload.buf;
+			len = payload.len;
+		}
+
 		if (!tag_number++) {
 			fmt_tag_signature(&tagbuf, &sig, buf, len);
 			first_tag = i;
@@ -531,8 +539,10 @@  static void fmt_merge_msg_sigs(struct strbuf *out)
 			fmt_tag_signature(&tagbuf, &sig, buf, len);
 		}
 		strbuf_release(&sig);
+		strbuf_release(&payload);
+		strbuf_release(&signature);
 	next:
-		free(buf);
+		free(orig);
 	}
 	if (tagbuf.len) {
 		strbuf_addch(out, '\n');
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 411e0b4d99..4c814c1963 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -636,7 +636,7 @@  static void prepare_push_cert_sha1(struct child_process *proc)
 
 		memset(&sigcheck, '\0', sizeof(sigcheck));
 
-		bogs = parse_signature(push_cert.buf, push_cert.len);
+		bogs = parse_signed_buffer(push_cert.buf, push_cert.len);
 		check_signature(push_cert.buf, bogs, push_cert.buf + bogs,
 				push_cert.len - bogs, &sigcheck);
 
@@ -1568,7 +1568,7 @@  static void queue_commands_from_cert(struct command **tail,
 		die("malformed push certificate %.*s", 100, push_cert->buf);
 	else
 		boc += 2;
-	eoc = push_cert->buf + parse_signature(push_cert->buf, push_cert->len);
+	eoc = push_cert->buf + parse_signed_buffer(push_cert->buf, push_cert->len);
 
 	while (boc < eoc) {
 		const char *eol = memchr(boc, '\n', eoc - boc);
diff --git a/builtin/tag.c b/builtin/tag.c
index e0a4c25382..6b95c6a2ea 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -174,11 +174,17 @@  static void write_tag_body(int fd, const struct object_id *oid)
 {
 	unsigned long size;
 	enum object_type type;
-	char *buf, *sp;
+	char *buf, *sp, *orig;
+	struct strbuf payload = STRBUF_INIT;
+	struct strbuf signature = STRBUF_INIT;
 
-	buf = read_object_file(oid, &type, &size);
+	orig = buf = read_object_file(oid, &type, &size);
 	if (!buf)
 		return;
+	if (parse_signature(buf, size, &payload, &signature)) {
+		buf = payload.buf;
+		size = payload.len;
+	}
 	/* skip header */
 	sp = strstr(buf, "\n\n");
 
@@ -187,9 +193,11 @@  static void write_tag_body(int fd, const struct object_id *oid)
 		return;
 	}
 	sp += 2; /* skip the 2 LFs */
-	write_or_die(fd, sp, parse_signature(sp, buf + size - sp));
+	write_or_die(fd, sp, buf + size - sp);
 
-	free(buf);
+	free(orig);
+	strbuf_release(&payload);
+	strbuf_release(&signature);
 }
 
 static int build_tag_object(struct strbuf *buf, int sign, struct object_id *result)
diff --git a/commit.c b/commit.c
index 534e14f22a..d39ce5076d 100644
--- a/commit.c
+++ b/commit.c
@@ -1097,8 +1097,10 @@  static void handle_signed_tag(struct commit *parent, struct commit_extra_header
 	struct merge_remote_desc *desc;
 	struct commit_extra_header *mergetag;
 	char *buf;
-	unsigned long size, len;
+	unsigned long size;
 	enum object_type type;
+	struct strbuf payload = STRBUF_INIT;
+	struct strbuf signature = STRBUF_INIT;
 
 	desc = merge_remote_util(parent);
 	if (!desc || !desc->obj)
@@ -1106,8 +1108,7 @@  static void handle_signed_tag(struct commit *parent, struct commit_extra_header
 	buf = read_object_file(&desc->obj->oid, &type, &size);
 	if (!buf || type != OBJ_TAG)
 		goto free_return;
-	len = parse_signature(buf, size);
-	if (size == len)
+	if (!parse_signature(buf, size, &payload, &signature))
 		goto free_return;
 	/*
 	 * We could verify this signature and either omit the tag when
@@ -1126,6 +1127,8 @@  static void handle_signed_tag(struct commit *parent, struct commit_extra_header
 
 	**tail = mergetag;
 	*tail = &mergetag->next;
+	strbuf_release(&payload);
+	strbuf_release(&signature);
 	return;
 
 free_return:
diff --git a/gpg-interface.c b/gpg-interface.c
index 2d538bcd6e..b25f5c21d8 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -345,7 +345,7 @@  void print_signature_buffer(const struct signature_check *sigc, unsigned flags)
 		fputs(output, stderr);
 }
 
-size_t parse_signature(const char *buf, size_t size)
+size_t parse_signed_buffer(const char *buf, size_t size)
 {
 	size_t len = 0;
 	size_t match = size;
@@ -361,6 +361,17 @@  size_t parse_signature(const char *buf, size_t size)
 	return match;
 }
 
+int parse_signature(const char *buf, size_t size, struct strbuf *payload, struct strbuf *signature)
+{
+	size_t match = parse_signed_buffer(buf, size);
+	if (match != size) {
+		strbuf_add(payload, buf, match);
+		strbuf_add(signature, buf + match, size - match);
+		return 1;
+	}
+	return 0;
+}
+
 void set_signing_key(const char *key)
 {
 	free(configured_signing_key);
diff --git a/gpg-interface.h b/gpg-interface.h
index f4e9b4f371..80567e4894 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -37,13 +37,20 @@  struct signature_check {
 
 void signature_check_clear(struct signature_check *sigc);
 
+/*
+ * Look at a GPG signed tag object.  If such a signature exists, store it in
+ * signature and the signed content in payload.  Return 1 if a signature was
+ * found, and 0 otherwise.
+ */
+int parse_signature(const char *buf, size_t size, struct strbuf *payload, struct strbuf *signature);
+
 /*
  * Look at GPG signed content (e.g. a signed tag object), whose
  * payload is followed by a detached signature on it.  Return the
  * offset where the embedded detached signature begins, or the end of
  * the data when there is no such signature.
  */
-size_t parse_signature(const char *buf, size_t size);
+size_t parse_signed_buffer(const char *buf, size_t size);
 
 /*
  * Create a detached signature for the contents of "buffer" and append
diff --git a/log-tree.c b/log-tree.c
index cae38dcc66..75bd61a531 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -499,7 +499,9 @@  static int show_one_mergetag(struct commit *commit,
 	struct strbuf verify_message;
 	struct signature_check sigc = { 0 };
 	int status, nth;
-	size_t payload_size, gpg_message_offset;
+	size_t gpg_message_offset;
+	struct strbuf payload = STRBUF_INIT;
+	struct strbuf signature = STRBUF_INIT;
 
 	hash_object_file(the_hash_algo, extra->value, extra->len,
 			 type_name(OBJ_TAG), &oid);
@@ -523,13 +525,11 @@  static int show_one_mergetag(struct commit *commit,
 			    "parent #%d, tagged '%s'\n", nth + 1, tag->tag);
 	gpg_message_offset = verify_message.len;
 
-	payload_size = parse_signature(extra->value, extra->len);
 	status = -1;
-	if (extra->len > payload_size) {
+	if (parse_signature(extra->value, extra->len, &payload, &signature)) {
 		/* could have a good signature */
-		if (!check_signature(extra->value, payload_size,
-				     extra->value + payload_size,
-				     extra->len - payload_size, &sigc)) {
+		if (!check_signature(payload.buf, payload.len,
+				     signature.buf, signature.len, &sigc)) {
 			strbuf_addstr(&verify_message, sigc.gpg_output);
 			signature_check_clear(&sigc);
 			status = 0; /* good */
@@ -540,6 +540,8 @@  static int show_one_mergetag(struct commit *commit,
 
 	show_sig_lines(opt, status, verify_message.buf);
 	strbuf_release(&verify_message);
+	strbuf_release(&payload);
+	strbuf_release(&signature);
 	return 0;
 }
 
diff --git a/ref-filter.c b/ref-filter.c
index 6867e33648..212f1165bb 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1161,7 +1161,13 @@  static void find_subpos(const char *buf,
 			unsigned long *nonsiglen,
 			const char **sig, unsigned long *siglen)
 {
+	struct strbuf payload = STRBUF_INIT;
+	struct strbuf signature = STRBUF_INIT;
 	const char *eol;
+	const char *end = buf + strlen(buf);
+	const char *sigstart;
+
+
 	/* skip past header until we hit empty line */
 	while (*buf && *buf != '\n') {
 		eol = strchrnul(buf, '\n');
@@ -1174,13 +1180,14 @@  static void find_subpos(const char *buf,
 		buf++;
 
 	/* parse signature first; we might not even have a subject line */
-	*sig = buf + parse_signature(buf, strlen(buf));
-	*siglen = strlen(*sig);
+	parse_signature(buf, end - buf, &payload, &signature);
+	*sig = strbuf_detach(&signature, siglen);
+	sigstart = buf + parse_signed_buffer(buf, strlen(buf));
 
 	/* subject is first non-empty line */
 	*sub = buf;
 	/* subject goes to first empty line */
-	while (buf < *sig && *buf && *buf != '\n') {
+	while (buf < sigstart && *buf && *buf != '\n') {
 		eol = strchrnul(buf, '\n');
 		if (*eol)
 			eol++;
@@ -1196,7 +1203,7 @@  static void find_subpos(const char *buf,
 		buf++;
 	*body = buf;
 	*bodylen = strlen(buf);
-	*nonsiglen = *sig - buf;
+	*nonsiglen = sigstart - buf;
 }
 
 /*
@@ -1234,6 +1241,7 @@  static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf)
 		struct used_atom *atom = &used_atom[i];
 		const char *name = atom->name;
 		struct atom_value *v = &val[i];
+
 		if (!!deref != (*name == '*'))
 			continue;
 		if (deref)
@@ -1273,6 +1281,8 @@  static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf)
 			v->s = strbuf_detach(&s, NULL);
 		} else if (atom->u.contents.option == C_BARE)
 			v->s = xstrdup(subpos);
+
+		free((void *)sigpos);
 	}
 }
 
diff --git a/tag.c b/tag.c
index 71b544467e..5d04506d10 100644
--- a/tag.c
+++ b/tag.c
@@ -13,26 +13,27 @@  const char *tag_type = "tag";
 static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
 {
 	struct signature_check sigc;
-	size_t payload_size;
+	struct strbuf payload = STRBUF_INIT;
+	struct strbuf signature = STRBUF_INIT;
 	int ret;
 
 	memset(&sigc, 0, sizeof(sigc));
 
-	payload_size = parse_signature(buf, size);
-
-	if (size == payload_size) {
+	if (!parse_signature(buf, size, &payload, &signature)) {
 		if (flags & GPG_VERIFY_VERBOSE)
-			write_in_full(1, buf, payload_size);
+			write_in_full(1, buf, size);
 		return error("no signature found");
 	}
 
-	ret = check_signature(buf, payload_size, buf + payload_size,
-				size - payload_size, &sigc);
+	ret = check_signature(payload.buf, payload.len, signature.buf,
+				signature.len, &sigc);
 
 	if (!(flags & GPG_VERIFY_OMIT_STATUS))
 		print_signature_buffer(&sigc, flags);
 
 	signature_check_clear(&sigc);
+	strbuf_release(&payload);
+	strbuf_release(&signature);
 	return ret;
 }