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

> Am 22.10.22 um 18:51 schrieb Junio C Hamano:
>> René Scharfe <l.s.r@web.de> writes:
>>
>>>>> +		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.
> 
> Right, currently the value of `max` is used to check the absolute value,
> i.e. it imposes a limit in both the positive and negative direction.
> 
>> 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.
> 
> Allowing arbitrary limits in both directions might be a new feature, but
> it's required if we want to support the full range of values.  E.g. on
> my system INT_MAX is 2147483647 and INT_MIN is -2147483647-1.  Currently
> git_parse_int() rejects INT_MIN as out of range.
> 
> In my eyes the assumption that a single limit can be used to check both
> directions of the signed number line is flawed and caused the undefined
> behavior.  Dropping it avoids tricky calculations that try to infer the
> lower limit somehow and opens the full range of values to us.
> 
> That said, I'm not sure how useful the values INT_MIN, INT64_MIN and
> SSIZE_MIN (which unlike SSIZE_MAX is not defined by POSIX [*]) actually
> are. But doing the checks properly requires separate min and max values.

> René
> 
> 
> [*] Perhaps git_parse_ssize_t() should reject values less than -1; only
> that single negative number is needed to represent errors or unlimited.