Re: [PATCH 3/3] git_parse_signed(): avoid integer overflow
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 21, 2022, 18:31 UTC
- Message-ID
- <xmqqpmeljae9.fsf@gitster.g>
- In-Reply-To
- <f058f391c3821b341a15fda9ae9fd20dda6a0494.1666359915.git.gitgitgadget@gmail.com>
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 6 quoted lines
> 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.
Show 19 quoted lines
> 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?