Re: [PATCH 4/9] cache-tree: avoid strtol() on non-string buffer

[Patrick Steinhardt <ps@pks.im>]

On Wed, Nov 12, 2025 at 03:05:37AM -0500, Jeff King wrote:
> A cache-tree extension entry in the index looks like this:
> 
>   <name> NUL <entry_nr> SPACE <subtree_nr> NEWLINE <binary_oid>
> 
> where the "_nr" items are human-readable base-10 ASCII. We parse them
> with strtol(), even though we do not have a NUL-terminated string (we'd
> generally have an mmap() of the on-disk index file). For a well-formed
> entry, this is not a problem; strtol() will stop when it sees the
> newline. But there are two problems:
> 
>   1. A corrupted entry could omit the newline, causing us to read
>      further. You'd mostly get stopped by seeing non-digits in the oid
>      field (and if it is likewise truncated, there will still be 20 or
>      more bytes of the index checksum). So it's possible, though
>      unlikely, to see read off the end of the mmap'd buffer. Of course a

s/see read/read/

>      malicious index file can fake the oid and the index checksum to all
>      (ASCII) 0's.
> 
>      This is further complicated by the fact that mmap'd buffers tend to
>      be zero-padded up to the page boundary. So to run off the end, the
>      index size also has to be a multiple of the page size. This is also
>      unlikely, though you can construct a malicious index file that
>      matches this.
> 
>      The security implications aren't too interesting. The index file is
>      a local file anyway (so you can't attack somebody by cloning, but
>      only if you convince them to operate in a .git directory you made,
>      at which point attacking .git/config is much easier). And it's just
>      a read overflow via strtol(), which is unlikely to buy you much
>      beyond a crash.

Agreed. Good to fix it regardless.

> diff --git a/cache-tree.c b/cache-tree.c
> index 2aba47060e..ab20ffe863 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -548,12 +548,36 @@ void cache_tree_write(struct strbuf *sb, struct cache_tree *root)
>  	trace2_region_leave("cache_tree", "write", the_repository);
>  }
>  
> +static long parse_long(const char **ptr, unsigned long *len_p)
> +{
> +	const char *s = *ptr;
> +	unsigned long len = *len_p;
> +	long ret = 0;
> +	int sign = 1;
> +
> +	while (len && *s == '-') {
> +		sign *= -1;
> +		s++;
> +		len--;
> +	}
> +
> +	while (len) {
> +		if (!isdigit(*s))
> +			break;
> +		ret *= 10;
> +		ret += *s - '0';
> +		s++;
> +		len--;
> +	}
> +	*ptr = s;
> +	*len_p = len;
> +	return sign * ret;
> +}

Hm. I'm not a huge fan of not having any error handling at all. It just
feels way too fragile for my taste:

  - As you mention we don't detect overflows, as we would detect them at
    a later point in time when trying to access index entries at invalid
    offsets. But if the input is crafted in a way that the overflow ends
    up with a reasonable index entry we might just as well _not_ detect
    that an overflow has happened and end up using the wrong index
    entry.

  - We don't verify that we even have a number in the first place. We'd
    simply return "0" in that case and not advance the pointer. This is
    fine though as we verify that the returned size is non-zero, so we'd
    detect this case.

I'd much rather prefer to have an interface similar to `git_parse_int()`
and related functions, which are way easier to use compared to the likes
of `stroi()`.

Otherwise, the next time we want to have something similar like you
introduce here we might not be aware of this existing helper and may not
know to generalize it, so we'd introduce another ad-hoc helper.

Anyway. I won't object if you want to go with your version anyway, as it
at least fixes one class of errors.

Thanks!

Patrick