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

From: Jeff King <peff@peff.net>
Date: Nov 18, 2025, 08:38 UTC
Message-ID: <20251118083838.GA4164207@coredump.intra.peff.net>
In-Reply-To: <aRRuzrmbJBW8q4Dd@pks.im>

On Wed, Nov 12, 2025 at 12:26:06PM +0100, Patrick Steinhardt wrote:

Show 35 quoted lines

> > +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.

Yes, but this is true of the original code as well. It does not bother to check if strtol() saw overflow (and in fact, it is using "int" and not "long", so it would need to do its own overflow check on top).

But see below.

>   - 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 think "0" is accepted at least for it->entry_count. The original did check that strtol() advanced "ep", and I dropped that. As with the overflow, it's not a memory safety issue, but it changes how we'd react to garbage input.

For v2 I've tweaked the interface to return an error code from the helper, and it will complain if there's no input at all. I didn't add in overflow checks, as they weren't there originally and are a little tricky to get right (and I wanted to focus on the memory safety issue). But they could be easily added to the helper on top of my patches.

-Peff