From: Phillip Wood <phillip.wood123@gmail.com>
Date: Tue, 18 Nov 2025 14:30:32 GMT
Subject: Re: [PATCH v2 4/9] cache-tree: avoid strtol() on non-string buffer
Message-ID: <ca6d99cc-d05c-49fb-ab3c-d7668077d32b@gmail.com>
In-Reply-To: <20251118091218.GD529192@coredump.intra.peff.net>

Hi Peff

On 18/11/2025 09:12, Jeff King wrote:
> Let's fix it by just parsing the values ourselves with a helper function
> that is careful not to go past the end of the buffer. There are a few
> behavior changes here that should not matter:
> 
>    - We do not consider overflow, as strtol() would. But nor did the
>      original code. However, we don't trust the value we get from the
>      on-disk file, and if it says to read 2^30 entries, we would notice
>      that we do not have that many and bail before reading off the end of
>      the buffer.
> 
>    - Our helper does not skip past extra leading whitespace as strtol()
>      would, but according to gitformat-index(5) there should not be any.
> 
>    - The original quit parsing at a newline or a NUL byte, but now we
>      insist on a newline (which is what the documentation says, and what
>      Git has always produced).

I think that sounds reasonable, I've left a couple of comments below.

> +static int parse_int(const char **ptr, unsigned long *len_p, int *out)
> +{
> +	const char *s = *ptr;
> +	unsigned long len = *len_p;
> +	int ret = 0;

This is signed which means that any overflow is undefined. While the 
existing code does not check for overflow I think it is well defined in 
the presence of overflow. It also means parsing INT_MIN is undefined as 
we parse the value as unsigned and then multiply by -1 if we saw a 
leading '-'. We shouldn't see any negative values apart from "-1" but 
given we're changing this code to be more robust in handling malformed 
input it would be nice if parsing INT_MIN was well defined.

> +	int sign = 1;
> +
> +	while (len && *s == '-') {
> +		sign *= -1;
> +		s++;
> +		len--;
> +	}

This accepts any number of '-' signs but I believe strtol() only accepts 
a single sign (the standard says "optionally preceded by a plus or minus 
sign") so this is a change in behavior from the existing code. I'm not 
sure we really need to be that accommodating here.

> +	while (len) {
> +		if (!isdigit(*s))
> +			break;
> +		ret *= 10;
> +		ret += *s - '0';
> +		s++;
> +		len--;
> +	}
> +
> +	if (s == *ptr)
> +		return -1;

This accepts "-" as a valid input, as we're tightening up our parsing it 
would be nice to require a digit after any '-' sign.

 > [...]> +	buf++; size--;
> +	if (parse_int(&buf, &size, &subtree_nr) < 0)
> +		goto free_return;

This isn't a new problem but if subtree_nr is negative we end up trying 
to allocate a huge chunk of memory. If that somehow succeeds we then end 
up calling die("cache-tree: internal error"). The existing code looks 
safe but it would be nice to die() a bit earlier if subtree_nr is negative.

Thanks

Phillip
  > +	if (!size || *buf != '\n')
>   		goto free_return;
>   	buf++; size--;
>   	if (0 <= it->entry_count) {