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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 26, 2023, 15:32 UTC
Message-ID: <xmqqildiveu6.fsf@gitster.g>
In-Reply-To: <20230426113658.GC130148@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 11 quoted lines

> Instead, we should go back to what the original iteration of the series
> was doing, and make sure there is at least one digit (i.e., a "forbid
> unknown" strategy). Assuming that there is no locale where ascii "1" is
> considered whitespace. ;)
>
> Note that will exclude a few cases that we do allow now, like:
>
>   committer name <email> \v123456 +0000\n
>
> Right now that parses as "123456", but we'd reject it as "0" after such
> a patch.

I would say that it is a good thing.

The only (somewhat) end-user controlled things on the line are the name and email, and even there name is sanitized to remove "crud". The user-supplied timestamp goes through date.c::parse_date(), ending up with what date.c::date_string() formats, so there will not be syntactically incorrect timestamp there. So we can be strict format-wise on the timestamp field, once we identify where it begins, which is the point of scanning backwards for '>'.

Unless the user does "hash-object" and deliberately creates a malformed commit object---they can keep both halves just fine in such a case as long as we do reject such a timestamp correctly.

Show 10 quoted lines

> The alternative is to check _all_ of the characters between ">" and the
> newline and make sure there is some digit somewhere, which would be
> sufficient to prevent strtoumax() from walking past the newline.
>
> I guess it's not even any more expensive in the normal case (since the
> very first non-whitespace entry should be a digit!). I'm not sure it's
> worth caring about too much either way. Garbage making it into
> name/email is an easy mistake to make (for users and implementations).
> Putting whitespace control codes into your timestamp is not, and marking
> them as "0" is an OK outcome.

Yeah, I think it is fine either way.

Thanks