From: Jeff King <peff@peff.net>
To: Thomas Bock <bockthom@cs.uni-saarland.de>
Cc: Derrick Stolee <derrickstolee@github.com>,
Junio C Hamano <gitster@pobox.com>,
git@vger.kernel.org
Subject: [PATCH 3/3] parse_commit(): handle broken whitespace-only timestamp
Date: Sat, 22 Apr 2023 09:50:01 -0400 [thread overview]
Message-ID: <20230422135001.GA3942563@coredump.intra.peff.net> (raw)
The comment in parse_commit_date() claims that parse_timestamp() will
not walk past the end of the buffer we've been given, since it will hit
the newline at "eol" and stop. This is usually true, when dateptr
contains actual numbers to parse. But with a line like:
committer name <email> \n
with just whitespace, and no numbers, parse_timestamp() will consume
that newline as part of the leading whitespace, and we may walk past our
"tail" pointer (which itself is set from the "size" parameter passed in
to parse_commit_buffer()).
In practice this can't cause us to walk off the end of an array, because
we always add an extra NUL byte to the end of objects we load from disk
(as a defense against exactly this kind of bug). However, you can see
the behavior in action when "committer" is the final header (which it
usually is, unless there's an encoding) and the subject line can be
parsed as an integer. We walk right past the newline on the committer
line, as well as the "\n\n" separator, and mistake the subject for the
timestamp.
The new test demonstrates such a case. I also added a test to check this
case against the pretty-print formatter, which uses split_ident_line().
It's not subject to the same bug, because it insists that there be one
or more digits in the timestamp.
We can use the same logic here. If there's a non-whitespace but
non-digit value (say "committer name <email> foo"), then
parse_timestamp() would already have returned 0 anyway. So the only
change should be for this "whitespace only" case.
Signed-off-by: Jeff King <peff@peff.net>
---
commit.c | 10 ++++++++++
t/t4212-log-corrupt.sh | 29 +++++++++++++++++++++++++++++
2 files changed, 39 insertions(+)
diff --git a/commit.c b/commit.c
index ede810ac1c..56877322d3 100644
--- a/commit.c
+++ b/commit.c
@@ -120,6 +120,16 @@ static timestamp_t parse_commit_date(const char *buf, const char *tail)
if (dateptr == buf || dateptr == eol)
return 0;
+ /*
+ * trim leading whitespace; parse_timestamp() will do this itself, but
+ * it will walk past the newline at eol while doing so. So we insist
+ * that there is at least one digit here.
+ */
+ while (dateptr < eol && isspace(*dateptr))
+ dateptr++;
+ if (!strchr("0123456789", *dateptr))
+ return 0;
+
/* dateptr < eol && *eol == '\n', so parsing will stop at eol */
return parse_timestamp(dateptr, NULL, 10);
}
diff --git a/t/t4212-log-corrupt.sh b/t/t4212-log-corrupt.sh
index af4b35ff56..d4ef48d646 100755
--- a/t/t4212-log-corrupt.sh
+++ b/t/t4212-log-corrupt.sh
@@ -92,4 +92,33 @@ test_expect_success 'absurdly far-in-future date' '
git log -1 --format=%ad $commit
'
+test_expect_success 'create commit with whitespace committer date' '
+ # It is important that this subject line is numeric, since we want to
+ # be sure we are not confused by skipping whitespace and accidentally
+ # parsing the subject as a timestamp.
+ #
+ # Do not use munge_author_date here. Besides not hitting the committer
+ # line, it leaves the timezone intact, and we want nothing but
+ # whitespace.
+ test_commit 1234567890 &&
+ git cat-file commit HEAD >commit.orig &&
+ sed "s/>.*/> /" <commit.orig >commit.munge &&
+ ws_commit=$(git hash-object --literally -w -t commit commit.munge)
+'
+
+test_expect_success '--until treats whitespace date as sentinel' '
+ echo $ws_commit >expect &&
+ git rev-list --until=1980-01-01 $ws_commit >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'pretty-printer handles whitespace date' '
+ # as with the %ad test above, we will show these as the empty string,
+ # not the 1970 epoch date. This is intentional; see 7d9a281941 (t4212:
+ # test bogus timestamps with git-log, 2014-02-24) for more discussion.
+ echo : >expect &&
+ git log -1 --format="%at:%ct" $ws_commit >actual &&
+ test_cmp expect actual
+'
+
test_done
--
2.40.0.653.g15ca972062
next reply other threads:[~2023-04-22 13:50 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-04-22 13:50 Jeff King [this message]
2023-04-22 15:53 ` [PATCH 3/3] parse_commit(): handle broken whitespace-only timestamp René Scharfe
2023-04-23 0:37 ` Jeff King
2023-04-25 5:56 ` Jeff King
2023-04-24 18:01 ` Junio C Hamano
2023-04-25 5:27 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230422135001.GA3942563@coredump.intra.peff.net \
--to=peff@peff.net \
--cc=bockthom@cs.uni-saarland.de \
--cc=derrickstolee@github.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).