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

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

[Jeff King]

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(+)

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

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

[Jeff King]

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
+	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

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

[Eric Sunshine]

On Mon, Jun 16, 2014 at 4:26 PM, Jeff King <peff@peff.net> wrote:
> 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).

> +       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

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

[Jeff King]

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

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

[Jeff King]

On Mon, Jun 16, 2014 at 07:36:49PM -0400, Jeff King wrote:

> 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

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

[Jeff King]

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(-)

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

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

[Jeff King]

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(-)

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

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

[Jeff King]

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(+)

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

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

[Jeff King]

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(+)

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

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

[Jeff King]

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(+)

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