{"thread":{"id":"59633","subject":"[PATCH 3/3] parse_commit(): handle broken whitespace-only timestamp","startedAt":"2023-04-22T13:50:06Z","lastAt":"2023-04-25T05:56:26Z","messageCount":6,"participants":["Jeff King","René Scharfe","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"475837","messageId":"20230422135001.GA3942563@coredump.intra.peff.net","threadId":"59633","inReplyTo":null,"subject":"[PATCH 3/3] parse_commit(): handle broken whitespace-only timestamp","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-22T13:50:01Z","receivedAt":"2023-04-22T13:50:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The comment in parse_commit_date() claims that parse_timestamp() will\nnot walk past the end of the buffer we've been given, since it will hit\nthe newline at \"eol\" and stop. This is usually true, when dateptr\ncontains actual numbers to parse. But with a line like:\n\n   committer name <email>   \\n\n\nwith just whitespace, and no numbers, parse_timestamp() will consume\nthat newline as part of the leading whitespace, and we may walk past our\n\"tail\" pointer (which itself is set from the \"size\" parameter passed in\nto parse_commit_buffer()).\n\nIn practice this can't cause us to walk off the end of an array, because\nwe always add an extra NUL byte to the end of objects we load from disk\n(as a defense against exactly this kind of bug). However, you can see\nthe behavior in action when \"committer\" is the final header (which it\nusually is, unless there's an encoding) and the subject line can be\nparsed as an integer. We walk right past the newline on the committer\nline, as well as the \"\\n\\n\" separator, and mistake the subject for the\ntimestamp.\n\nThe new test demonstrates such a case. I also added a test to check this\ncase against the pretty-print formatter, which uses split_ident_line().\nIt's not subject to the same bug, because it insists that there be one\nor more digits in the timestamp.\n\nWe can use the same logic here. If there's a non-whitespace but\nnon-digit value (say \"committer name <email> foo\"), then\nparse_timestamp() would already have returned 0 anyway. So the only\nchange should be for this \"whitespace only\" case.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n commit.c               | 10 ++++++++++\n t/t4212-log-corrupt.sh | 29 +++++++++++++++++++++++++++++\n 2 files changed, 39 insertions(+)\n\ndiff --git a/commit.c b/commit.c\nindex ede810ac1c..56877322d3 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -120,6 +120,16 @@ static timestamp_t parse_commit_date(const char *buf, const char *tail)\n \tif (dateptr == buf || dateptr == eol)\n \t\treturn 0;\n \n+\t/*\n+\t * trim leading whitespace; parse_timestamp() will do this itself, but\n+\t * it will walk past the newline at eol while doing so. So we insist\n+\t * that there is at least one digit here.\n+\t */\n+\twhile (dateptr < eol && isspace(*dateptr))\n+\t\tdateptr++;\n+\tif (!strchr(\"0123456789\", *dateptr))\n+\t\treturn 0;\n+\n \t/* dateptr < eol && *eol == '\\n', so parsing will stop at eol */\n \treturn parse_timestamp(dateptr, NULL, 10);\n }\ndiff --git a/t/t4212-log-corrupt.sh b/t/t4212-log-corrupt.sh\nindex af4b35ff56..d4ef48d646 100755\n--- a/t/t4212-log-corrupt.sh\n+++ b/t/t4212-log-corrupt.sh\n@@ -92,4 +92,33 @@ test_expect_success 'absurdly far-in-future date' '\n \tgit log -1 --format=%ad $commit\n '\n \n+test_expect_success 'create commit with whitespace committer date' '\n+\t# It is important that this subject line is numeric, since we want to\n+\t# be sure we are not confused by skipping whitespace and accidentally\n+\t# parsing the subject as a timestamp.\n+\t#\n+\t# Do not use munge_author_date here. Besides not hitting the committer\n+\t# line, it leaves the timezone intact, and we want nothing but\n+\t# whitespace.\n+\ttest_commit 1234567890 &&\n+\tgit cat-file commit HEAD >commit.orig &&\n+\tsed \"s/>.*/>    /\" <commit.orig >commit.munge &&\n+\tws_commit=$(git hash-object --literally -w -t commit commit.munge)\n+'\n+\n+test_expect_success '--until treats whitespace date as sentinel' '\n+\techo $ws_commit >expect &&\n+\tgit rev-list --until=1980-01-01 $ws_commit >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'pretty-printer handles whitespace date' '\n+\t# as with the %ad test above, we will show these as the empty string,\n+\t# not the 1970 epoch date. This is intentional; see 7d9a281941 (t4212:\n+\t# test bogus timestamps with git-log, 2014-02-24) for more discussion.\n+\techo : >expect &&\n+\tgit log -1 --format=\"%at:%ct\" $ws_commit >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.40.0.653.g15ca972062\n"},{"id":"475845","messageId":"7f48188b-2e06-9f01-de8d-a24812fb20fe@web.de","threadId":"59633","inReplyTo":"20230422135001.GA3942563@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] parse_commit(): handle broken whitespace-only timestamp","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-04-22T15:53:10Z","receivedAt":"2023-04-22T15:53:34Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 22.04.23 um 15:50 schrieb Jeff King:\n> diff --git a/commit.c b/commit.c\n> index ede810ac1c..56877322d3 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -120,6 +120,16 @@ static timestamp_t parse_commit_date(const char *buf, const char *tail)\n>  \tif (dateptr == buf || dateptr == eol)\n>  \t\treturn 0;\n>\n> +\t/*\n> +\t * trim leading whitespace; parse_timestamp() will do this itself, but\n> +\t * it will walk past the newline at eol while doing so. So we insist\n> +\t * that there is at least one digit here.\n> +\t */\n> +\twhile (dateptr < eol && isspace(*dateptr))\n> +\t\tdateptr++;\n> +\tif (!strchr(\"0123456789\", *dateptr))\n\nYou could use (our own) isdigit() here instead.  It's more concise and\nefficient.\n\nRené\n\n"},{"id":"475887","messageId":"20230423003715.GB3953216@coredump.intra.peff.net","threadId":"59633","inReplyTo":"7f48188b-2e06-9f01-de8d-a24812fb20fe@web.de","subject":"Re: [PATCH 3/3] parse_commit(): handle broken whitespace-only timestamp","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-23T00:37:15Z","receivedAt":"2023-04-23T00:37:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 22, 2023 at 05:53:10PM +0200, René Scharfe wrote:\n\n> Am 22.04.23 um 15:50 schrieb Jeff King:\n> > diff --git a/commit.c b/commit.c\n> > index ede810ac1c..56877322d3 100644\n> > --- a/commit.c\n> > +++ b/commit.c\n> > @@ -120,6 +120,16 @@ static timestamp_t parse_commit_date(const char *buf, const char *tail)\n> >  \tif (dateptr == buf || dateptr == eol)\n> >  \t\treturn 0;\n> >\n> > +\t/*\n> > +\t * trim leading whitespace; parse_timestamp() will do this itself, but\n> > +\t * it will walk past the newline at eol while doing so. So we insist\n> > +\t * that there is at least one digit here.\n> > +\t */\n> > +\twhile (dateptr < eol && isspace(*dateptr))\n> > +\t\tdateptr++;\n> > +\tif (!strchr(\"0123456789\", *dateptr))\n> \n> You could use (our own) isdigit() here instead.  It's more concise and\n> efficient.\n\nHeh, yes, that is much better. I had strspn() on the mind since that is\nwhat split_ident_line() uses.\n\nI think it could even just be:\n\n  if (dateptr != eol)\n\nwhich implies that we found some non-whitespace character, and then we\nrely on parse_timestamp() to return 0 (which is what the current code is\neffectively doing).\n\n-Peff\n"},{"id":"475953","messageId":"xmqqy1mhdurt.fsf@gitster.g","threadId":"59633","inReplyTo":"20230422135001.GA3942563@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] parse_commit(): handle broken whitespace-only timestamp","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-24T18:01:26Z","receivedAt":"2023-04-24T18:01:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> The comment in parse_commit_date() claims that parse_timestamp() will\n> not walk past the end of the buffer we've been given, since it will hit\n> the newline at \"eol\" and stop. This is usually true, when dateptr\n> contains actual numbers to parse. But with a line like:\n>\n>    committer name <email>   \\n\n\nI was wondering of this case while reading [2/3] ;-)\n\n> ...\n> In practice this can't cause us to walk off the end of an array, because\n> we always add an extra NUL byte to the end of objects we load from disk\n> (as a defense against exactly this kind of bug). However, you can see\n> the behavior in action when \"committer\" is the final header (which it\n> usually is, unless there's an encoding) and the subject line can be\n> parsed as an integer. We walk right past the newline on the committer\n> line, as well as the \"\\n\\n\" separator, and mistake the subject for the\n> timestamp.\n\n\n> +\t/*\n> +\t * trim leading whitespace; parse_timestamp() will do this itself, but\n> +\t * it will walk past the newline at eol while doing so. So we insist\n> +\t * that there is at least one digit here.\n> +\t */\n\n\"one digit\" -> \"one non-whitespace\".\n\n> +\twhile (dateptr < eol && isspace(*dateptr))\n> +\t\tdateptr++;\n\nThis is an expected change, but\n\n> +\tif (!strchr(\"0123456789\", *dateptr))\n> +\t\treturn 0;\n\nthis is not.  Isn't the only problematic case that dateptr being at\neol?  That is what the proposed log message argued.\n\n>  \t/* dateptr < eol && *eol == '\\n', so parsing will stop at eol */\n\nThis comment is slightly stale.  dateptr < eol, *eol == '\\n', and we\nknow the string starting at dateptr is not a run of whitespace and\nthat is what makes the parsing stop at eol.\n\n> diff --git a/t/t4212-log-corrupt.sh b/t/t4212-log-corrupt.sh\n> index af4b35ff56..d4ef48d646 100755\n> --- a/t/t4212-log-corrupt.sh\n> +++ b/t/t4212-log-corrupt.sh\n> @@ -92,4 +92,33 @@ test_expect_success 'absurdly far-in-future date' '\n>  \tgit log -1 --format=%ad $commit\n>  '\n>  \n> +test_expect_success 'create commit with whitespace committer date' '\n> +\t# It is important that this subject line is numeric, since we want to\n> +\t# be sure we are not confused by skipping whitespace and accidentally\n> +\t# parsing the subject as a timestamp.\n\nNice.\n\n> +\t# Do not use munge_author_date here. Besides not hitting the committer\n> +\t# line, it leaves the timezone intact, and we want nothing but\n> +\t# whitespace.\n> +\ttest_commit 1234567890 &&\n> +\tgit cat-file commit HEAD >commit.orig &&\n> +\tsed \"s/>.*/>    /\" <commit.orig >commit.munge &&\n> +\tws_commit=$(git hash-object --literally -w -t commit commit.munge)\n> +'\n> +\n> +test_expect_success '--until treats whitespace date as sentinel' '\n> +\techo $ws_commit >expect &&\n> +\tgit rev-list --until=1980-01-01 $ws_commit >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'pretty-printer handles whitespace date' '\n> +\t# as with the %ad test above, we will show these as the empty string,\n> +\t# not the 1970 epoch date. This is intentional; see 7d9a281941 (t4212:\n> +\t# test bogus timestamps with git-log, 2014-02-24) for more discussion.\n> +\techo : >expect &&\n> +\tgit log -1 --format=\"%at:%ct\" $ws_commit >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n>  test_done\n"},{"id":"476003","messageId":"20230425052739.GB4007491@coredump.intra.peff.net","threadId":"59633","inReplyTo":"xmqqy1mhdurt.fsf@gitster.g","subject":"Re: [PATCH 3/3] parse_commit(): handle broken whitespace-only timestamp","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-25T05:27:39Z","receivedAt":"2023-04-25T05:27:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 24, 2023 at 11:01:26AM -0700, Junio C Hamano wrote:\n\n> > +\t/*\n> > +\t * trim leading whitespace; parse_timestamp() will do this itself, but\n> > +\t * it will walk past the newline at eol while doing so. So we insist\n> > +\t * that there is at least one digit here.\n> > +\t */\n> \n> \"one digit\" -> \"one non-whitespace\".\n> \n> > +\twhile (dateptr < eol && isspace(*dateptr))\n> > +\t\tdateptr++;\n> \n> This is an expected change, but\n> \n> > +\tif (!strchr(\"0123456789\", *dateptr))\n> > +\t\treturn 0;\n> \n> this is not.  Isn't the only problematic case that dateptr being at\n> eol?  That is what the proposed log message argued.\n\nYes, that would be sufficient. I was moving things slightly closer to\nwhat split_ident_line() does by actually checking for numbers. But that\nled to the final paragraph in the commit message explaining how it all\nends up the same either way.\n\nSo I'll swap this out for:\n\n  if (dateptr == eol)\n\nwhich I think requires less explanation, as it leaves the function more\nlike it was originally (and the behavior is the same either way).\n\n> >  \t/* dateptr < eol && *eol == '\\n', so parsing will stop at eol */\n> \n> This comment is slightly stale.  dateptr < eol, *eol == '\\n', and we\n> know the string starting at dateptr is not a run of whitespace and\n> that is what makes the parsing stop at eol.\n\nYeah, I hoped the extra context of the earlier comment would be enough. ;)\nBut it is probably better to spell it out by expanding this comment.\nThe code is certainly tricky enough.\n\n-Peff\n"},{"id":"476010","messageId":"20230425055620.GA4015761@coredump.intra.peff.net","threadId":"59633","inReplyTo":"20230423003715.GB3953216@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] parse_commit(): handle broken whitespace-only timestamp","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-04-25T05:56:20Z","receivedAt":"2023-04-25T05:56:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 22, 2023 at 08:37:15PM -0400, Jeff King wrote:\n\n> > You could use (our own) isdigit() here instead.  It's more concise and\n> > efficient.\n> \n> Heh, yes, that is much better. I had strspn() on the mind since that is\n> what split_ident_line() uses.\n> \n> I think it could even just be:\n> \n>   if (dateptr != eol)\n> \n> which implies that we found some non-whitespace character, and then we\n> rely on parse_timestamp() to return 0 (which is what the current code is\n> effectively doing).\n\nThis should be \"dateptr == eol\" of course, because the body of the\nconditional is \"return 0\" to signal an error. It's correct in the v2 of\nthe series I just sent out.\n\n-Peff\n"}]}