From: Phillip Wood <phillip.wood123@gmail.com>
Date: Sun, 23 Nov 2025 15:51:57 GMT
Subject: Re: [PATCH v2 4/9] cache-tree: avoid strtol() on non-string buffer
Message-ID: <633f4d92-c258-45a8-9d32-116c94838e68@gmail.com>
In-Reply-To: <xmqqtsylz2xh.fsf@gitster.g>

On 23/11/2025 06:19, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>>> +	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.
> 
> That is true, but at the same time I do not think we really need to
> make it more strict with extra code.

All we need to do to accept a single minus sign is s/while/if/

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

If we limit ourselves to accepting a single minus sign then this can become
	if (s == *ptr + (sign == -1))

so we need very little in the way of extra code.

> We could try to be more careful, but it quickly became messy when I
> tried.  Here is an unfinished attempt of mine.

A generic helper to replace strtol() that takes a length rather than 
assuming the input is NUL terminated could be useful elsewhere but I'm 
not sure we need something that complicated here. I do like the fact 
that overflow does not cause undefined behavior though. Changing ret for 
"int" to "unsigned" in peff's patch should fix that.

Thanks

Phillip

> 
> static int parse_int(const char **ptr, unsigned long *len_p, int *out)
> {
> 	const char *s = *ptr;
> 	unsigned long len = *len_p;
> 	unsigned val = 0;
> 	bool negate = false;
> 	int saw_digits = 0;
> 
> 	while (len && isspace(*s)) {
> 		len--;
> 		s++;
> 	}
> 	if (!len)
> 		return -1;
> 	switch (*s) {
> 	case '-':
> 		negate = true;
> 		/* fallthru */
> 	case '+':
> 		s++;
> 		len--;
> 		break;
> 	default:
> 		break;
> 	}
> 
> 	while (len) {
> 		unsigned next;
> 		if (!isdigit(*s))
> 			break;
> 		next = val * 10 + *s - '0';
> 		if (next < val)
> 			return -1;
> 		val = next;
> 		s++;
> 		len--;
> 		saw_digits = 1;
> 	}
> 	if (!saw_digits ||
> 	    (!negate && INT_MAX <= val) ||
> 	    (negate && INT_MAX < val))
> 		return -1;
> 
> 	*ptr = s;
> 	*len_p = len;
> 	*out = negate ? (0 - val) : val;
> 	return 0;
> }