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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 23, 2025, 06:19 UTC
Message-ID: <xmqqtsylz2xh.fsf@gitster.g>
In-Reply-To: <ca6d99cc-d05c-49fb-ab3c-d7668077d32b@gmail.com>

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

Show 10 quoted lines

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

Show 14 quoted lines

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

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

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;
}