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

["René Scharfe" <l.s.r@web.de>]

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.