git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH] pretty: fix buffer over-read with %> and %<
@ 2017-11-26  2:52 mwnx
  2017-11-27  1:46 ` Junio C Hamano
  0 siblings, 1 reply; 5+ messages in thread
From: mwnx @ 2017-11-26  2:52 UTC (permalink / raw)
  To: git; +Cc: Nguyễn Thái Ngọc Duy, mwnx

A buffer over-read of the format string would occur with unterminated
formats of the form '%>(#' and '%<(#', where '#' represents a number.

This error can be witnessed by running git log under valgrind like so:

    valgrind git log -n1 --format='%<(42'

This was due to the fact that the "not found" case for strcspn() was
being checked in the same way that one checks the "not found" case for
strchr(), i.e. by checking for a NULL pointer return value. Instead, one
must check for the end of the string since strcspn() points to the
character where it finished its search (which will be a '\0' if
unsuccessful).

Signed-off-by: mwnx <mwnx@gmx.com>
---
 pretty.c                      | 2 +-
 t/t4205-log-pretty-formats.sh | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/pretty.c b/pretty.c
index 2f6b0ae6c..4c70bad45 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1021,7 +1021,7 @@ static size_t parse_padding_placeholder(struct strbuf *sb,
 		const char *end = start + strcspn(start, ",)");
 		char *next;
 		int width;
-		if (!end || end == start)
+		if (!*end || end == start)
 			return 0;
 		width = strtol(start, &next, 10);
 		if (next == start || width == 0)
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 591f35daa..4d9555962 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -598,4 +598,10 @@ test_expect_success ':only and :unfold work together' '
 	test_cmp expect actual
 '
 
+test_expect_success 'unterminated alignment formatting' '
+	git log -n1 --format="%<(42" >actual &&
+	echo "%<(42" >expected &&
+	test_cmp expected actual
+'
+
 test_done
-- 
2.15.0.90.g6559daec7


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] pretty: fix buffer over-read with %> and %<
  2017-11-26  2:52 mwnx
@ 2017-11-27  1:46 ` Junio C Hamano
  2017-11-27 23:29   ` mwnx
  0 siblings, 1 reply; 5+ messages in thread
From: Junio C Hamano @ 2017-11-27  1:46 UTC (permalink / raw)
  To: mwnx; +Cc: git, Nguyễn Thái Ngọc Duy

mwnx <mwnx@gmx.com> writes:

> diff --git a/pretty.c b/pretty.c
> index 2f6b0ae6c..4c70bad45 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1021,7 +1021,7 @@ static size_t parse_padding_placeholder(struct strbuf *sb,
>  		const char *end = start + strcspn(start, ",)");
>  		char *next;
>  		int width;
> -		if (!end || end == start)
> +		if (!*end || end == start)

Yuck.  This is so obvious a typo as it is quite clear that a few
lines above will never give us !end.  Well spotted.

By the way, Documentation/SubmittingPatches has this in "(5) Certify
your work" section:

    Also notice that a real name is used in the Signed-off-by: line. Please
    don't hide your real name.


>  			return 0;
>  		width = strtol(start, &next, 10);
>  		if (next == start || width == 0)
> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
> index 591f35daa..4d9555962 100755
> --- a/t/t4205-log-pretty-formats.sh
> +++ b/t/t4205-log-pretty-formats.sh
> @@ -598,4 +598,10 @@ test_expect_success ':only and :unfold work together' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'unterminated alignment formatting' '
> +	git log -n1 --format="%<(42" >actual &&
> +	echo "%<(42" >expected &&
> +	test_cmp expected actual
> +'
> +
>  test_done

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] pretty: fix buffer over-read with %> and %<
  2017-11-27  1:46 ` Junio C Hamano
@ 2017-11-27 23:29   ` mwnx
  2017-11-28  2:10     ` Junio C Hamano
  0 siblings, 1 reply; 5+ messages in thread
From: mwnx @ 2017-11-27 23:29 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

On Mon, Nov 27, 2017 at 10:46:23AM +0900, Junio C Hamano wrote:
> By the way, Documentation/SubmittingPatches has this in "(5) Certify
> your work" section:
> 
>     Also notice that a real name is used in the Signed-off-by: line. Please
>     don't hide your real name.

I did read that document, but I saw a few commits which were signed
off with an alias so I figured the rule might be flexible
(especially since I'm not quite sure what the rationale behind it
is). I'd rather not give my full name if I can avoid it, however if
I really can't convince you to accept this patch any other way, I
guess I'll comply. What are your thoughts on this issue?

-- 
mwnx
GPG: AEC9 554B 07BD F60D 75A3  AF6A 44E8 E4D4 0312 C726

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] pretty: fix buffer over-read with %> and %<
  2017-11-27 23:29   ` mwnx
@ 2017-11-28  2:10     ` Junio C Hamano
  0 siblings, 0 replies; 5+ messages in thread
From: Junio C Hamano @ 2017-11-28  2:10 UTC (permalink / raw)
  To: mwnx; +Cc: git

mwnx <mwnx@gmx.com> writes:

> On Mon, Nov 27, 2017 at 10:46:23AM +0900, Junio C Hamano wrote:
>> By the way, Documentation/SubmittingPatches has this in "(5) Certify
>> your work" section:
>> 
>>     Also notice that a real name is used in the Signed-off-by: line. Please
>>     don't hide your real name.
>
> (especially since I'm not quite sure what the rationale behind it
> is).

As DCO is something we'd need to present to the court when the next
SCO comes after us, we'd prefer to see that we can refer to, and
contact if necessary, a real person when we need to say "this is not
a stolen code, here is the person who presented it to us and we'll
let him or her explain".

> What are your thoughts on this issue?

Not limited to this, but our stance for things are that previous
mistakes do not justify repeating and spreading the same.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH] pretty: fix buffer over-read with %> and %<
@ 2017-11-29  0:32 mwnx
  0 siblings, 0 replies; 5+ messages in thread
From: mwnx @ 2017-11-29  0:32 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, mwnx

A buffer over-read of the format string would occur with unterminated
formats of the form '%>(#' and '%<(#', where '#' represents a number.

This error can be witnessed by running git log under valgrind like so:

    valgrind git log -n1 --format='%<(42'

This was due to the fact that the "not found" case for strcspn() was
being checked in the same way that one checks the "not found" case for
strchr(), i.e. by checking for a NULL pointer return value. Instead, one
must check for the end of the string since strcspn() points to the
character where it finished its search (which will be a '\0' if
unsuccessful).

Signed-off-by: Maxwell Nixie <mwnx@gmx.com>
---
Hope I got everything right.

 pretty.c                      | 2 +-
 t/t4205-log-pretty-formats.sh | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/pretty.c b/pretty.c
index 2f6b0ae6c..4c70bad45 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1021,7 +1021,7 @@ static size_t parse_padding_placeholder(struct strbuf *sb,
 		const char *end = start + strcspn(start, ",)");
 		char *next;
 		int width;
-		if (!end || end == start)
+		if (!*end || end == start)
 			return 0;
 		width = strtol(start, &next, 10);
 		if (next == start || width == 0)
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 591f35daa..4d9555962 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -598,4 +598,10 @@ test_expect_success ':only and :unfold work together' '
 	test_cmp expect actual
 '
 
+test_expect_success 'unterminated alignment formatting' '
+	git log -n1 --format="%<(42" >actual &&
+	echo "%<(42" >expected &&
+	test_cmp expected actual
+'
+
 test_done
-- 
2.15.0.319.gb3398b820


^ permalink raw reply related	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2017-11-29  0:33 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-29  0:32 [PATCH] pretty: fix buffer over-read with %> and %< mwnx
  -- strict thread matches above, loose matches on Subject: below --
2017-11-26  2:52 mwnx
2017-11-27  1:46 ` Junio C Hamano
2017-11-27 23:29   ` mwnx
2017-11-28  2:10     ` Junio C Hamano

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).