From: Phillip Wood Date: Thu, 27 Apr 2023 10:11:12 GMT Subject: Re: [PATCH v3 3/4] parse_commit(): handle broken whitespace-only timestamp Message-ID: In-Reply-To: <20230427081715.GA1478467@coredump.intra.peff.net> 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 \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 > Signed-off-by: Jeff King > --- > 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.munge && > + ws_commit=$(git hash-object --literally -w -t commit commit.munge) && > + sed "s/>.*/> $(printf "\013")/" 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