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

[Junio C Hamano <gitster@pobox.com>]

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> git_parse_signed() checks that the absolute value of the parsed string
> is less than or equal to a caller supplied maximum value. When
> calculating the absolute value there is a integer overflow if `val ==
> INTMAX_MIN`.

It is a problem that is worth looking into.

> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  config.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/config.c b/config.c
> index b7fb68026d8..aad3e00341d 100644
> --- a/config.c
> +++ b/config.c
> @@ -1160,8 +1160,10 @@ static int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)
>  	if (value && *value) {
>  		char *end;
>  		intmax_t val;
> -		uintmax_t uval;
> -		uintmax_t factor;
> +		intmax_t factor;
> +
> +		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?