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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 25, 2023, 16:06 UTC
Message-ID: <xmqqttx43q08.fsf@gitster.g>
In-Reply-To: <06bc5530-55b0-7299-a6e6-fde1deab6605@gmail.com>

Phillip Wood <phillip.wood123@gmail.com> writes:

Show 11 quoted lines

> This probably doesn't matter in practice but we define our own
> isspace() that does not treat '\v' and '\f' as whitespace. However
> parse_timestamp() (which is just strtoumax()) uses the standard
> library's isspace() which does treat those characters as whitespace
> and is locale dependent. This means we can potentially stop at a
> character that parse_timestamp() treats as whitespace and if there are
> no digits after it we'll still walk past the end of the line. Using
> Rene's suggestion of testing the character with isdigit() would fix
> that. It would also avoid parsing negative timestamps as positive
> numbers and reject any timestamps that begin with a locale dependent
> digit.

A very interesting observation. I wonder if a curious person can craft a malformed timestamp with "hash-object --literally" to do more than DoS themselves?

We are not going to put anything other than [ 0-9+-] after the '>' we scan for, and making sure '>' is followed by SP and then [0-9] would be sufficient to ensure strtoumax() to stop before the '\n' but does not ensure that the "signal a bad timestamp with 0" happens. Perhaps that would be sufficient. I dunno.

> I'm not familiar with this code, but would it be worth changing
> parse_timestamp() to stop parsing if it sees a newline?

Meaning replace or write our own strtoumax() equivalent?