threads / patch / 36932

patchpretty: avoid reading past end-of-string with "%G"

Subject: [PATCH] pretty: avoid reading past end-of-string with "%G"

## tl;dr

10 messages between Jun 16, 2014 and Jun 17, 2014. Diffs are folded; open one to read it.

replies: 9people: 2as markdown or json

Jeff King· Jun 16, 2014, 20:13 UTC · lore

If the user asks for --format=%G with nothing else, we correctly realize that "%G" is not a valid placeholder (it should be "%G?", "%GK", etc). But we still tell the strbuf_expand code that we consumed 2 characters, causing it to jump over the trailing NUL and output garbage.

This also fixes the case where "%GX" would be consumed (and produce no output). In other cases, we pass unrecognized placeholders through to the final string.

Signed-off-by: Jeff King <peff@peff.net>
---
Noticed while experimenting with "%G" placeholders in the nearby thread.
It doesn't look like we have any tests of "%G*" and friends at all. :(
 pretty.c | 2 ++
 1 file changed, 2 insertions(+)
Show changes to pretty.c +2 −0
diff --git a/pretty.c b/pretty.c
index e1e2cad..70d8776 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1267,6 +1267,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 			if (c->signature_check.key)
 				strbuf_addstr(sb, c->signature_check.key);
 			break;
+		default:
+			return 0;
 		}
 		return 2;
 	}
-- 
2.0.0.566.gfe3e6b2
Jeff King· Jun 16, 2014, 20:26 UTC · re: Jeff King · lore

[PATCH] t7510: check %G* pretty-format output

On Mon, Jun 16, 2014 at 04:13:11PM -0400, Jeff King wrote:
> It doesn't look like we have any tests of "%G*" and friends at all. :(
Maybe we can add this:
-- >8 --
Subject: t7510: check %G* pretty-format output

We do not check these along with the other pretty-format placeholders in t6006, because we need signed commits to make them interesting. t7510 has such commits, and can easily exercise them in addition to the regular --show-signature code path.

Signed-off-by: Jeff King <peff@peff.net>
---
I explicitly avoided "%GG" here, as its exact format is dependent on
gpg (and the current date). I don't know that it is worth the
complexity to test, as the interesting parts are already parsed from it
and exposed in the other placeholders.
 t/t7510-signed-commit.sh | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)
Show changes to t/t7510-signed-commit.sh +36 −0
diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index 5ddac1a..2f96937 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -76,6 +76,42 @@ test_expect_success GPG 'detect fudged signature' '
 	! grep "Good signature from" actual1
 '
 
+test_expect_success GPG 'show good signature with custom format' '
+	cat >expect <<-\EOF
+	G
+	13B6F51ECDDE430D
+	C O Mitter <committer@example.com>
+	EOF
+	git log -1 --format="%G?%n%GK%n%GS" master >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success GPG 'show bad signature with custom format' '
+	cat >expect <<-\EOF
+	B
+	13B6F51ECDDE430D
+	C O Mitter <committer@example.com>
+	EOF
+	git log -1 --format="%G?%n%GK%n%GS" $(cat forged1.commit) >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success GPG 'show lack of signature with custom format' '
+	cat >expect <<-\EOF
+	N
+
+
+	EOF
+	git log -1 --format="%G?%n%GK%n%GS" seventh-unsigned >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'unused %G placeholders are passed through' '
+	echo "%GX %G" >expect &&
+	git log -1 --format="%GX %G" >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success GPG 'detect fudged signature with NUL' '
 	git cat-file commit master >raw &&
 	cat raw >forged2 &&
-- 
2.0.0.566.gfe3e6b2
Eric Sunshine· Jun 16, 2014, 21:50 UTC · re: Jeff King · lore

Re: [PATCH] t7510: check %G* pretty-format output

On Mon, Jun 16, 2014 at 4:26 PM, Jeff King <peff@peff.net> wrote:
Show 35 quoted lines
> On Mon, Jun 16, 2014 at 04:13:11PM -0400, Jeff King wrote:
>
>> It doesn't look like we have any tests of "%G*" and friends at all. :(
>
> Maybe we can add this:
>
> -- >8 --
> Subject: t7510: check %G* pretty-format output
>
> We do not check these along with the other pretty-format
> placeholders in t6006, because we need signed commits to
> make them interesting. t7510 has such commits, and can
> easily exercise them in addition to the regular
> --show-signature code path.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I explicitly avoided "%GG" here, as its exact format is dependent on
> gpg (and the current date). I don't know that it is worth the
> complexity to test, as the interesting parts are already parsed from it
> and exposed in the other placeholders.
>
>  t/t7510-signed-commit.sh | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
>
> diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
> index 5ddac1a..2f96937 100755
> --- a/t/t7510-signed-commit.sh
> +++ b/t/t7510-signed-commit.sh
> @@ -76,6 +76,42 @@ test_expect_success GPG 'detect fudged signature' '
>         ! grep "Good signature from" actual1
>  '
>
> +test_expect_success GPG 'show good signature with custom format' '
> +       cat >expect <<-\EOF
Broken &&-chain (and in tests below).
Show 44 quoted lines
> +       G
> +       13B6F51ECDDE430D
> +       C O Mitter <committer@example.com>
> +       EOF
> +       git log -1 --format="%G?%n%GK%n%GS" master >actual &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success GPG 'show bad signature with custom format' '
> +       cat >expect <<-\EOF
> +       B
> +       13B6F51ECDDE430D
> +       C O Mitter <committer@example.com>
> +       EOF
> +       git log -1 --format="%G?%n%GK%n%GS" $(cat forged1.commit) >actual &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success GPG 'show lack of signature with custom format' '
> +       cat >expect <<-\EOF
> +       N
> +
> +
> +       EOF
> +       git log -1 --format="%G?%n%GK%n%GS" seventh-unsigned >actual &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success 'unused %G placeholders are passed through' '
> +       echo "%GX %G" >expect &&
> +       git log -1 --format="%GX %G" >actual &&
> +       test_cmp expect actual
> +'
> +
>  test_expect_success GPG 'detect fudged signature with NUL' '
>         git cat-file commit master >raw &&
>         cat raw >forged2 &&
> --
> 2.0.0.566.gfe3e6b2
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff King· Jun 16, 2014, 23:36 UTC · re: Eric Sunshine · lore

Re: [PATCH] t7510: check %G* pretty-format output

On Mon, Jun 16, 2014 at 05:50:14PM -0400, Eric Sunshine wrote:
> > +test_expect_success GPG 'show good signature with custom format' '
> > +       cat >expect <<-\EOF
> 
> Broken &&-chain (and in tests below).

Whoops, thanks. Re-roll coming in a minute. I'm also reorganizing the two patches a bit and adding a test for an unvalidated key.

-Peff
Jeff King· Jun 16, 2014, 23:59 UTC · re: Jeff King · lore

[PATCH 0/5] --format=%G tests and fixes

On Mon, Jun 16, 2014 at 07:36:49PM -0400, Jeff King wrote:
Show 9 quoted lines
> On Mon, Jun 16, 2014 at 05:50:14PM -0400, Eric Sunshine wrote:
> 
> > > +test_expect_success GPG 'show good signature with custom format' '
> > > +       cat >expect <<-\EOF
> > 
> > Broken &&-chain (and in tests below).
> 
> Whoops, thanks. Re-roll coming in a minute. I'm also reorganizing the
> two patches a bit and adding a test for an unvalidated key.

OK, here they are. Somehow it multiplied into 5 patches. I must have gotten them wet.

  [1/5]: t7510: stop referring to master in later tests
  [2/5]: t7510: use consistent &&-chains in loop
  [3/5]: t7510: test a commit signed by an unknown key
  [4/5]: t7510: check %G* pretty-format output
  [5/5]: pretty: avoid reading past end-of-string with "%G"
-Peff
Jeff King· Jun 16, 2014, 23:59 UTC · re: Jeff King · lore

[PATCH 1/5] t7510: stop referring to master in later tests

Our setup creates a sequence of commits, each with its own tag. However, we sometimes refer to "seventh-signed" as "master". This works, since it is at the tip of the created branch, but is brittle if new tests need to add more commits. Let's use its tag name to be unambiguous.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t7510-signed-commit.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
Show changes to t/t7510-signed-commit.sh +3 −3
diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index 5ddac1a..37c3778 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -47,7 +47,7 @@ test_expect_success GPG 'create signed commits' '
 
 test_expect_success GPG 'show signatures' '
 	(
-		for commit in initial second merge fourth-signed fifth-signed sixth-signed master
+		for commit in initial second merge fourth-signed fifth-signed sixth-signed seventh-signed
 		do
 			git show --pretty=short --show-signature $commit >actual &&
 			grep "Good signature from" actual || exit 1
@@ -67,7 +67,7 @@ test_expect_success GPG 'show signatures' '
 '
 
 test_expect_success GPG 'detect fudged signature' '
-	git cat-file commit master >raw &&
+	git cat-file commit seventh-signed >raw &&
 
 	sed -e "s/seventh/7th forged/" raw >forged1 &&
 	git hash-object -w -t commit forged1 >forged1.commit &&
@@ -77,7 +77,7 @@ test_expect_success GPG 'detect fudged signature' '
 '
 
 test_expect_success GPG 'detect fudged signature with NUL' '
-	git cat-file commit master >raw &&
+	git cat-file commit seventh-signed >raw &&
 	cat raw >forged2 &&
 	echo Qwik | tr "Q" "\000" >>forged2 &&
 	git hash-object -w -t commit forged2 >forged2.commit &&
-- 
2.0.0.566.gfe3e6b2
Jeff King· Jun 17, 2014, 00:03 UTC · re: Jeff King · lore

[PATCH 2/5] t7510: use consistent &&-chains in loop

From: Michael J Gruber <git@drmicha.warpmail.net>

We check multiple commits in a loop. Because we want to break out of the loop if any single iteration fails, we use a subshell/exit like:

  (
	for i in $stuff
	do
		do-something $i || exit 1
	done
  )

However, we are inconsistent in our loop body. Some commands get their own "|| exit 1", and others try to chain to the next command with "&&", like:

  X &&
  Y || exit 1
  Z || exit 1

This is a little hard to read and follow, because X and Y are treated differently for no good reason. But much worse, the second loop follows a similar pattern and gets it wrong. "Y" is expected to fail, so we use "&& exit 1", giving us:

  X &&
  Y && exit 1
  Z || exit 1

That gets the test for X wrong (we do not exit unless both X fails and Y unexpectedly succeeds, but we would want to exit if _either_ is wrong). We can write this clearly and correctly by consistently using "&&", followed by a single "|| exit 1", and negating Y with "!" (as we would in a normal &&-chain). Like:

  X &&
  ! Y &&
  Z || exit 1
Signed-off-by: Jeff King <peff@peff.net>
---
I listed Michael as the author here, because this is the patch that I
expected to come in his next re-roll, based on our discussion. I'm
including it here because I'm about to touch the same area (and Michael,
you'd probably want to rebase on top of this series anyway. Assuming I
haven't screwed it up :) ).
 t/t7510-signed-commit.sh | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
Show changes to t/t7510-signed-commit.sh +6 −6
diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index 37c3778..cdffcbd 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -50,18 +50,18 @@ test_expect_success GPG 'show signatures' '
 		for commit in initial second merge fourth-signed fifth-signed sixth-signed seventh-signed
 		do
 			git show --pretty=short --show-signature $commit >actual &&
-			grep "Good signature from" actual || exit 1
-			! grep "BAD signature from" actual || exit 1
-			echo $commit OK
+			grep "Good signature from" actual &&
+			! grep "BAD signature from" actual &&
+			echo $commit OK || exit 1
 		done
 	) &&
 	(
 		for commit in merge^2 fourth-unsigned sixth-unsigned seventh-unsigned
 		do
 			git show --pretty=short --show-signature $commit >actual &&
-			grep "Good signature from" actual && exit 1
-			! grep "BAD signature from" actual || exit 1
-			echo $commit OK
+			! grep "Good signature from" actual &&
+			! grep "BAD signature from" actual &&
+			echo $commit OK || exit 1
 		done
 	)
 '
-- 
2.0.0.566.gfe3e6b2
Jeff King· Jun 17, 2014, 00:05 UTC · re: Jeff King · lore

[PATCH 3/5] t7510: test a commit signed by an unknown key

We tested both good and bad signatures, but not ones made correctly but with a key for which we have no trust.

Signed-off-by: Jeff King <peff@peff.net>
---
I'm not happy about grepping more gpg output, but perhaps this "not
certified" is no worse than the current 'Good signature from" greps we
have?

The internal code uses --status-fd, which is presumably more robust to changes. I dunno. I'd be inclined to go with this, and if it becomes a problem in a future gpg release, we can deal with it then.

 t/t7510-signed-commit.sh | 13 +++++++++++++
 1 file changed, 13 insertions(+)
Show changes to t/t7510-signed-commit.sh +13 −0
diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index cdffcbd..04fc2c5 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -43,6 +43,9 @@ test_expect_success GPG 'create signed commits' '
 
 	test_tick && git rebase -f HEAD^^ && git tag sixth-signed HEAD^ &&
 	git tag seventh-signed
+
+	echo 8 >file && test_tick && git commit -a -m eighth -SB7227189 &&
+	git tag eighth-signed-alt
 '
 
 test_expect_success GPG 'show signatures' '
@@ -63,6 +66,16 @@ test_expect_success GPG 'show signatures' '
 			! grep "BAD signature from" actual &&
 			echo $commit OK || exit 1
 		done
+	) &&
+	(
+		for commit in eighth-signed-alt
+		do
+			git show --pretty=short --show-signature $commit >actual &&
+			grep "Good signature from" actual &&
+			! grep "BAD signature from" actual &&
+			grep "not certified" actual &&
+			echo $commit OK || exit 1
+		done
 	)
 '
 
-- 
2.0.0.566.gfe3e6b2
Jeff King· Jun 17, 2014, 00:06 UTC · re: Jeff King · lore

[PATCH 4/5] t7510: check %G* pretty-format output

We do not check these along with the other pretty-format placeholders in t6006, because we need signed commits to make them interesting. t7510 has such commits, and can easily exercise them in addition to the regular --show-signature code path.

Signed-off-by: Jeff King <peff@peff.net>
---
Similar to before, but with the 'U' test added in.
 t/t7510-signed-commit.sh | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)
Show changes to t/t7510-signed-commit.sh +40 −0
diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index 04fc2c5..e97477a 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -107,4 +107,44 @@ test_expect_success GPG 'amending already signed commit' '
 	! grep "BAD signature from" actual
 '
 
+test_expect_success GPG 'show good signature with custom format' '
+	cat >expect <<-\EOF &&
+	G
+	13B6F51ECDDE430D
+	C O Mitter <committer@example.com>
+	EOF
+	git log -1 --format="%G?%n%GK%n%GS" sixth-signed >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success GPG 'show bad signature with custom format' '
+	cat >expect <<-\EOF &&
+	B
+	13B6F51ECDDE430D
+	C O Mitter <committer@example.com>
+	EOF
+	git log -1 --format="%G?%n%GK%n%GS" $(cat forged1.commit) >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success GPG 'show unknown signature with custom format' '
+	cat >expect <<-\EOF &&
+	U
+	61092E85B7227189
+	Eris Discordia <discord@example.net>
+	EOF
+	git log -1 --format="%G?%n%GK%n%GS" eighth-signed-alt >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success GPG 'show lack of signature with custom format' '
+	cat >expect <<-\EOF &&
+	N
+
+
+	EOF
+	git log -1 --format="%G?%n%GK%n%GS" seventh-unsigned >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.0.0.566.gfe3e6b2
Jeff King· Jun 17, 2014, 00:07 UTC · re: Jeff King · lore

[PATCH 5/5] pretty: avoid reading past end-of-string with "%G"

If the user asks for --format=%G with nothing else, we correctly realize that "%G" is not a valid placeholder (it should be "%G?", "%GK", etc). But we still tell the strbuf_expand code that we consumed 2 characters, causing it to jump over the trailing NUL and output garbage.

This also fixes the case where "%GX" would be consumed (and produce no output). In other cases, we pass unrecognized placeholders through to the final string.

Signed-off-by: Jeff King <peff@peff.net>
---
Same as before, but with the test included along with the fix, rather
than in another commit.
 pretty.c                 | 2 ++
 t/t7510-signed-commit.sh | 6 ++++++
 2 files changed, 8 insertions(+)
Show changes to 2 files +8 −0

pretty.c, t/t7510-signed-commit.sh

diff --git a/pretty.c b/pretty.c
index e1e2cad..70d8776 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1267,6 +1267,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 			if (c->signature_check.key)
 				strbuf_addstr(sb, c->signature_check.key);
 			break;
+		default:
+			return 0;
 		}
 		return 2;
 	}
diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index e97477a..9810242 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -147,4 +147,10 @@ test_expect_success GPG 'show lack of signature with custom format' '
 	test_cmp expect actual
 '
 
+test_expect_success 'unused %G placeholders are passed through' '
+	echo "%GX %G" >expect &&
+	git log -1 --format="%GX %G" >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.0.0.566.gfe3e6b2

← back to recent threads