{"thread":{"id":"47322","subject":"[PATCH] pretty: fix buffer over-read with %> and %<","startedAt":"2017-11-26T02:55:03Z","lastAt":"2017-11-28T02:10:27Z","messageCount":4,"participants":["mwnx","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"333510","messageId":"20171126025222.7831-1-mwnx@gmx.com","threadId":"47322","inReplyTo":null,"subject":"[PATCH] pretty: fix buffer over-read with %> and %<","fromName":"mwnx","fromEmail":"mwnx@gmx.com","sentAt":"2017-11-26T02:52:22Z","receivedAt":"2017-11-26T02:55:03Z","isPatch":true,"sender":{"key":"mwnx@gmx.com","avatar":null},"body":"A buffer over-read of the format string would occur with unterminated\nformats of the form '%>(#' and '%<(#', where '#' represents a number.\n\nThis error can be witnessed by running git log under valgrind like so:\n\n valgrind git log -n1 --format='%<(42'\n\nThis was due to the fact that the \"not found\" case for strcspn() was\nbeing checked in the same way that one checks the \"not found\" case for\nstrchr(), i.e. by checking for a NULL pointer return value. Instead, one\nmust check for the end of the string since strcspn() points to the\ncharacter where it finished its search (which will be a '\\0' if\nunsuccessful).\n\nSigned-off-by: mwnx \n---\n pretty.c | 2 +-\n t/t4205-log-pretty-formats.sh | 6 ++++++\n 2 files changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex 2f6b0ae6c..4c70bad45 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1021,7 +1021,7 @@ static size_t parse_padding_placeholder(struct strbuf *sb,\n \t\tconst char *end = start + strcspn(start, \",)\");\n \t\tchar *next;\n \t\tint width;\n-\t\tif (!end || end == start)\n+\t\tif (!*end || end == start)\n \t\t\treturn 0;\n \t\twidth = strtol(start, &next, 10);\n \t\tif (next == start || width == 0)\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex 591f35daa..4d9555962 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -598,4 +598,10 @@ test_expect_success ':only and :unfold work together' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'unterminated alignment formatting' '\n+\tgit log -n1 --format=\"%<(42\" >actual &&\n+\techo \"%<(42\" >expected &&\n+\ttest_cmp expected actual\n+'\n+\n test_done\n-- \n2.15.0.90.g6559daec7\n\n"},{"id":"333565","messageId":"xmqqh8tgfq8g.fsf@gitster.mtv.corp.google.com","threadId":"47322","inReplyTo":"20171126025222.7831-1-mwnx@gmx.com","subject":"Re: [PATCH] pretty: fix buffer over-read with %> and %<","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-27T01:46:23Z","receivedAt":"2017-11-27T01:46:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"mwnx writes:\n\n> diff --git a/pretty.c b/pretty.c\n> index 2f6b0ae6c..4c70bad45 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -1021,7 +1021,7 @@ static size_t parse_padding_placeholder(struct strbuf *sb,\n> \t\tconst char *end = start + strcspn(start, \",)\");\n> \t\tchar *next;\n> \t\tint width;\n> -\t\tif (!end || end == start)\n> +\t\tif (!*end || end == start)\n\nYuck. This is so obvious a typo as it is quite clear that a few\nlines above will never give us !end. Well spotted.\n\nBy the way, Documentation/SubmittingPatches has this in \"(5) Certify\nyour work\" section:\n\n Also notice that a real name is used in the Signed-off-by: line. Please\n don't hide your real name.\n\n\n> \t\t\treturn 0;\n> \t\twidth = strtol(start, &next, 10);\n> \t\tif (next == start || width == 0)\n> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\n> index 591f35daa..4d9555962 100755\n> --- a/t/t4205-log-pretty-formats.sh\n> +++ b/t/t4205-log-pretty-formats.sh\n> @@ -598,4 +598,10 @@ test_expect_success ':only and :unfold work together' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'unterminated alignment formatting' '\n> +\tgit log -n1 --format=\"%<(42\" >actual &&\n> +\techo \"%<(42\" >expected &&\n> +\ttest_cmp expected actual\n> +'\n> +\n> test_done\n"},{"id":"333651","messageId":"20171127232927.x5iamt5rlxrafkhe@debian","threadId":"47322","inReplyTo":"xmqqh8tgfq8g.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] pretty: fix buffer over-read with %> and %<","fromName":"mwnx","fromEmail":"mwnx@gmx.com","sentAt":"2017-11-27T23:29:27Z","receivedAt":"2017-11-27T23:29:38Z","isPatch":true,"sender":{"key":"mwnx@gmx.com","avatar":null},"body":"On Mon, Nov 27, 2017 at 10:46:23AM +0900, Junio C Hamano wrote:\n> By the way, Documentation/SubmittingPatches has this in \"(5) Certify\n> your work\" section:\n> \n> Also notice that a real name is used in the Signed-off-by: line. Please\n> don't hide your real name.\n\nI did read that document, but I saw a few commits which were signed\noff with an alias so I figured the rule might be flexible\n(especially since I'm not quite sure what the rationale behind it\nis). I'd rather not give my full name if I can avoid it, however if\nI really can't convince you to accept this patch any other way, I\nguess I'll comply. What are your thoughts on this issue?\n\n-- \nmwnx\nGPG: AEC9 554B 07BD F60D 75A3 AF6A 44E8 E4D4 0312 C726\n"},{"id":"333667","messageId":"xmqqtvxf886r.fsf@gitster.mtv.corp.google.com","threadId":"47322","inReplyTo":"20171127232927.x5iamt5rlxrafkhe@debian","subject":"Re: [PATCH] pretty: fix buffer over-read with %> and %<","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-28T02:10:20Z","receivedAt":"2017-11-28T02:10:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"mwnx writes:\n\n> On Mon, Nov 27, 2017 at 10:46:23AM +0900, Junio C Hamano wrote:\n>> By the way, Documentation/SubmittingPatches has this in \"(5) Certify\n>> your work\" section:\n>> \n>> Also notice that a real name is used in the Signed-off-by: line. Please\n>> don't hide your real name.\n>\n> (especially since I'm not quite sure what the rationale behind it\n> is).\n\nAs DCO is something we'd need to present to the court when the next\nSCO comes after us, we'd prefer to see that we can refer to, and\ncontact if necessary, a real person when we need to say \"this is not\na stolen code, here is the person who presented it to us and we'll\nlet him or her explain\".\n\n> What are your thoughts on this issue?\n\nNot limited to this, but our stance for things are that previous\nmistakes do not justify repeating and spreading the same.\n"}]}