Re: [PATCH v3 3/4] parse_commit(): handle broken whitespace-only timestamp

[Phillip Wood <phillip.wood123@gmail.com>]

On 27/04/2023 09:17, Jeff King wrote:
> 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.
> 
> We can solve this by trimming the whitespace ourselves, making sure that
> it has some non-whitespace to parse. Note that we need to be a bit
> careful about the definition of "whitespace" here, as our isspace()
> doesn't match exotic characters like vertical tab or formfeed. We can
> work around that by checking for an actual number (see the in-code
> comment). This is slightly more restrictive than the current code, but
> in practice the results are either the same (we reject "foo" as "0", but
> so would parse_timestamp()) or extremely unlikely even for broken
> commits (parse_timestamp() would allow "\v123" as "123", but we'll now
> make it "0").
> 
> I did also allow "-" here, which may be controversial, as we don't
> currently support negative timestamps. My reasoning was two-fold. One,
> the design of parse_timestamp() is such that we should be able to easily
> switch it to handling signed values, and this otherwise creates a
> hard-to-find gotcha that anybody doing that work would get tripped up
> on. And two, the status quo is that we currently parse them, though the
> result of course ends up as a very large unsigned value (which is likely
> to just get clamped to "0" for display anyway, since our date routines
> can't handle it).

I think this makes a good case for accepting '-'. The commit message is 
well explained as always :-) This all looks good to me apart from a 
query about one of the tests.

> The new test checks the commit parser (via "--until") for both vanilla
> spaces and the vertical-tab case. I also added a test to check these
> against the pretty-print formatter, which uses split_ident_line().  It's
> not subject to the same bug, because it already insists that there be
> one or more digits in the timestamp.
> 
> Helped-by: Phillip Wood <phillip.wood123@gmail.com>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>   commit.c               | 28 ++++++++++++++++++++++++++--
>   t/t4212-log-corrupt.sh | 41 +++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 67 insertions(+), 2 deletions(-)
> 

> +test_expect_success 'create commits with whitespace committer dates' '
> +	# 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.
> +	#
> +	# We will make two munged commits here. The first, ws_commit, will
> +	# be purely spaces. The second contains a vertical tab, which is
> +	# considered a space by strtoumax(), but not by our isspace().

This comment is really helpful to explain what's going on and testing 
'\v' as well as ' ' is a good idea.

> +	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) &&
> +	sed "s/>.*/>   $(printf "\013")/" <commit.orig >commit.munge &&

Does the shell eat the '\v' when it trims trailing whitespace from the 
command substitution (I can't remember the rules off the top of my head)?

Best Wishes

Phillip

> +	vt_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 &&
> +
> +	echo $vt_commit >expect &&
> +	git rev-list --until=1980-01-01 $vt_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 &&
> +	git log -1 --format="%at:%ct" $vt_commit >actual &&
> +	test_cmp expect actual
> +'
> +
>   test_done