Re: [PATCH 3/3] git_parse_signed(): avoid integer overflow

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 22, 2022, 16:51 UTC
Message-ID: <xmqqv8obhkeb.fsf@gitster.g>
In-Reply-To: <80fe7c62-2c59-a715-3a8b-2dc2331d8e37@web.de>

René Scharfe <l.s.r@web.de> writes:

Show 12 quoted lines

>>> +		if (max < 0)
>>> +			BUG("max must be a positive integer");
>>
>> In parse_signed(), would we expect to accept end-user input that is
>> a negative integer?  We must.  Otherwise we would not be calling a
>> "signed" parser.  Now, are there cases where the valid value range
>> is bounded by a negative integer at the top?  No current callers may
>> pass such a value, but is it reasonable to add such a new constraints
>> to an existing API function?
>
> Hmm, if minimum and maximum are not symmetric, then we need to supply
> both, don't we?

Ah, thanks for injecting doze of sanity---I totally missed that the bound was about the absolute value, so we can say "this is signed, and the allowed values are (-3, -2, -1, 0, 1, 2, 3). If so, then the "reject negative max" in the posted patch is not a problem as I said above. I somehow thought that giving -1 as "max" would allow callers to say "non-negative numbers are not allowed". But that is not what is going on.

Allowing callers to specify both lower and uppoer bounds so that they can say "the allowed values are (-1, 0, 1, 2, 3)", while it might make it more useful, is a separate new feature development and outside the scope of "let's tighten the parsing of end user input" Phillip has here.

Sorry about the thinko, and thanks for a new and interesting tangent.