[PATCH 0/3] a few config integer parsing fixes

[PATCH 0/3] a few config integer parsing fixes

[Phillip Wood via GitGitGadget]

This series fixes some issues I noticed when reading the integer parsing
code in config.c

 * git_parse_unsigned() does not reject negative values
 * git_parse_[un]signed() accept a units specifier without any digits
 * git_parse_signed() has in integer overflow when parsing MAXINT_MIN

Ideally we'd have a test tool to unit test functions like this, I haven't
found time to write that yet. cc'ing René for patch 3 as he was the last
person to touch that code.

Phillip Wood (3):
  git_parse_unsigned: reject negative values
  config: require at least one digit when parsing numbers
  git_parse_signed(): avoid integer overflow

 config.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)


base-commit: e85701b4af5b7c2a9f3a1b07858703318dce365d
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1389%2Fphillipwood%2Fconfig-integer-parsing-fixes-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1389/phillipwood/config-integer-parsing-fixes-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1389
-- 
gitgitgadget

[PATCH 1/3] git_parse_unsigned: reject negative values

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

git_parse_unsigned() relies on strtoumax() which unfortunately parses
negative values as large positive integers. Fix this by rejecting any
string that contains '-' as we do in strtoul_ui(). I've chosen to treat
negative numbers as invalid input and set errno to EINVAL rather than
ERANGE one the basis that they are never acceptable if we're looking for
a unsigned integer.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 config.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/config.c b/config.c
index cbb5a3bab74..d5069d4f01d 100644
--- a/config.c
+++ b/config.c
@@ -1193,6 +1193,11 @@ static int git_parse_unsigned(const char *value, uintmax_t *ret, uintmax_t max)
 		uintmax_t val;
 		uintmax_t factor;
 
+		/* negative values would be accepted by strtoumax */
+		if (strchr(value, '-')) {
+			errno = EINVAL;
+			return 0;
+		}
 		errno = 0;
 		val = strtoumax(value, &end, 0);
 		if (errno == ERANGE)
-- 
gitgitgadget

Re: [PATCH 1/3] git_parse_unsigned: reject negative values

[Junio C Hamano]

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

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> git_parse_unsigned() relies on strtoumax() which unfortunately parses
> negative values as large positive integers. Fix this by rejecting any
> string that contains '-' as we do in strtoul_ui(). I've chosen to treat
> negative numbers as invalid input and set errno to EINVAL rather than
> ERANGE one the basis that they are never acceptable if we're looking for
> a unsigned integer.

And the code now would reject something like "43-21" because it does
not insist that "-" must be used for a sign, so it makes EINVAL
doubly appropriate, I would think.  In the original code, "43-21"
would have been parsed as "43" (by strtoumax) followed by "-" (which
is rejected by get_unit_factor() and yielded EINVAL, so this change
would not change the behaviour for such an input, if we receive
EINVAL when we see a "-".

A devil's advocate would consider if we ever want to have a unit
factor that has "-" in it (e.g. "10000e-3" == 10).  I can buy it if
we want to declare that it is not worth supporting.

> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  config.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/config.c b/config.c
> index cbb5a3bab74..d5069d4f01d 100644
> --- a/config.c
> +++ b/config.c
> @@ -1193,6 +1193,11 @@ static int git_parse_unsigned(const char *value, uintmax_t *ret, uintmax_t max)
>  		uintmax_t val;
>  		uintmax_t factor;
>  
> +		/* negative values would be accepted by strtoumax */
> +		if (strchr(value, '-')) {
> +			errno = EINVAL;
> +			return 0;
> +		}
>  		errno = 0;
>  		val = strtoumax(value, &end, 0);
>  		if (errno == ERANGE)

Re: [PATCH 1/3] git_parse_unsigned: reject negative values

[Jeff King]

On Fri, Oct 21, 2022 at 01:45:12PM +0000, Phillip Wood via GitGitGadget wrote:

> git_parse_unsigned() relies on strtoumax() which unfortunately parses
> negative values as large positive integers. Fix this by rejecting any
> string that contains '-' as we do in strtoul_ui(). I've chosen to treat
> negative numbers as invalid input and set errno to EINVAL rather than
> ERANGE one the basis that they are never acceptable if we're looking for
> a unsigned integer.

Certainly this seems like the right thing to do for a function parsing
an unsigned value. It would be nice if we could demonstrate the visible
effect with a test, though (and of course catch any later regressions).

Sadly "git config" doesn't let you ask to parse an unsigned type. But we
can find things in the core.* region that are parsed by default (and not
likely to change, as they represent file/memory sizes). Like:

  git -c core.bigFileThreshold=-1 rev-parse

which quietly passes before your patch, but fails after.

It does make me wonder if anybody uses a negative value like this in the
wild for "no limit", as it does what you might imagine currently (I get
2^64-1).

-Peff

Re: [PATCH 1/3] git_parse_unsigned: reject negative values

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> It does make me wonder if anybody uses a negative value like this in the
> wild for "no limit", as it does what you might imagine currently (I get
> 2^64-1).

I vaguely recall complaints on the command line argument that used
to take -1 to mean "practically unlimited" when its parsing got
tightened.  I wouldn't be surprised if we are making a new issue in
the same category in the configuration file with this change.

But we can switch to the signed variant when it becomes an issue, I
guess.

[PATCH 2/3] config: require at least one digit when parsing numbers

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

If the input to strtoimax() or strtoumax() does not contain any digits
then they return zero and set `end` to point to the start of the input
string.  git_parse_[un]signed() do not check `end` and so fail to return
an error and instead return a value of zero if the input string is a
valid units factor without any digits (e.g "k").

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 config.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/config.c b/config.c
index d5069d4f01d..b7fb68026d8 100644
--- a/config.c
+++ b/config.c
@@ -1167,6 +1167,10 @@ static int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)
 		val = strtoimax(value, &end, 0);
 		if (errno == ERANGE)
 			return 0;
+		if (end == value) {
+			errno = EINVAL;
+			return 0;
+		}
 		factor = get_unit_factor(end);
 		if (!factor) {
 			errno = EINVAL;
@@ -1202,6 +1206,10 @@ static int git_parse_unsigned(const char *value, uintmax_t *ret, uintmax_t max)
 		val = strtoumax(value, &end, 0);
 		if (errno == ERANGE)
 			return 0;
+		if (end == value) {
+			errno = EINVAL;
+			return 0;
+		}
 		factor = get_unit_factor(end);
 		if (!factor) {
 			errno = EINVAL;
-- 
gitgitgadget

Re: [PATCH 2/3] config: require at least one digit when parsing numbers

[Junio C Hamano]

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

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> If the input to strtoimax() or strtoumax() does not contain any digits
> then they return zero and set `end` to point to the start of the input
> string.  git_parse_[un]signed() do not check `end` and so fail to return
> an error and instead return a value of zero if the input string is a
> valid units factor without any digits (e.g "k").
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  config.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/config.c b/config.c
> index d5069d4f01d..b7fb68026d8 100644
> --- a/config.c
> +++ b/config.c
> @@ -1167,6 +1167,10 @@ static int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)
>  		val = strtoimax(value, &end, 0);
>  		if (errno == ERANGE)
>  			return 0;
> +		if (end == value) {
> +			errno = EINVAL;
> +			return 0;
> +		}

This means well, but doesn't strto*() family of functions silently
ignore leading blanks, e.g.

    l = strtol("  432k", &end, 0);
	... l == 432, *end = k ...

If you really want to reject a string with no number before the
optional unit, end at this point may not match value.  With " k" as
input, value would point at the space at the beginning, and end
would point at 'k'.

It does not look _too_ bad if we just let such an empty string
through and interpreted it as zero.  Is that a problem?  Who are we
trying to help?

>  		factor = get_unit_factor(end);
>  		if (!factor) {
>  			errno = EINVAL;
> @@ -1202,6 +1206,10 @@ static int git_parse_unsigned(const char *value, uintmax_t *ret, uintmax_t max)
>  		val = strtoumax(value, &end, 0);
>  		if (errno == ERANGE)
>  			return 0;
> +		if (end == value) {
> +			errno = EINVAL;
> +			return 0;
> +		}
>  		factor = get_unit_factor(end);
>  		if (!factor) {
>  			errno = EINVAL;

Re: [PATCH 2/3] config: require at least one digit when parsing numbers

[Phillip Wood]

Hi Junio

On 21/10/2022 19:19, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> @@ -1167,6 +1167,10 @@ static int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)
>>   		val = strtoimax(value, &end, 0);
>>   		if (errno == ERANGE)
>>   			return 0;
>> +		if (end == value) {
>> +			errno = EINVAL;
>> +			return 0;
>> +		}
> 
> This means well, but doesn't strto*() family of functions silently
> ignore leading blanks, e.g.
> 
>      l = strtol("  432k", &end, 0);
> 	... l == 432, *end = k ...
> 
> If you really want to reject a string with no number before the
> optional unit, end at this point may not match value.  With " k" as
> input, value would point at the space at the beginning, and end
> would point at 'k'.

It only skips the space if it sees a digit, if it does not find anything 
to convert it sets *end = start. Using peff's trick for testing this 
patch we can see there is no change in behavior if there is leading 
whitespace

$ bin-wrappers/git config --int --default " m" some.key
fatal: bad numeric config value ' m' for 'some.key': invalid unit

$ git config --int --default " m" some.key
fatal: bad numeric config value ' m' for 'some.key': invalid unit

> It does not look _too_ bad if we just let such an empty string
> through and interpreted it as zero.  Is that a problem?  Who are we
> trying to help?

My reasoning was that a single units factor is likely to be the result 
of some kind of bad edit and defaulting to zero when the user thought 
they set a large value is not likely to be helpful. Having said that I'm 
not that wedded to this patch if you feel it would be better to drop it.

Best Wishes

Phillip

Re: [PATCH 2/3] config: require at least one digit when parsing numbers

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> On 21/10/2022 19:19, Junio C Hamano wrote:
>> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>> @@ -1167,6 +1167,10 @@ static int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)
>>>   		val = strtoimax(value, &end, 0);
>>>   		if (errno == ERANGE)
>>>   			return 0;
>>> +		if (end == value) {
>>> +			errno = EINVAL;
>>> +			return 0;
>>> +		}
>> This means well, but doesn't strto*() family of functions silently
>> ignore leading blanks, e.g.
>>      l = strtol("  432k", &end, 0);
>> 	... l == 432, *end = k ...
>> If you really want to reject a string with no number before the
>> optional unit, end at this point may not match value.  With " k" as
>> input, value would point at the space at the beginning, and end
>> would point at 'k'.
>
> It only skips the space if it sees a digit, if it does not find
> anything to convert it sets *end = start.

Yeah, thanks.  Yes, my earlier observation was based on a faulty
experiments, and the code posted as-is would be OK.

Re: [PATCH 2/3] config: require at least one digit when parsing numbers

[Jeff King]

On Fri, Oct 21, 2022 at 01:45:13PM +0000, Phillip Wood via GitGitGadget wrote:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> 
> If the input to strtoimax() or strtoumax() does not contain any digits
> then they return zero and set `end` to point to the start of the input
> string.  git_parse_[un]signed() do not check `end` and so fail to return
> an error and instead return a value of zero if the input string is a
> valid units factor without any digits (e.g "k").

This one is easier to test than the last. Just:

  git config --int --default='m' some.key

works. And even playing devil's advocate, I can't think of a case where
anybody would rely on the current behavior.

-Peff

Re: [PATCH 2/3] config: require at least one digit when parsing numbers

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Oct 21, 2022 at 01:45:13PM +0000, Phillip Wood via GitGitGadget wrote:
>
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>> 
>> If the input to strtoimax() or strtoumax() does not contain any digits
>> then they return zero and set `end` to point to the start of the input
>> string.  git_parse_[un]signed() do not check `end` and so fail to return
>> an error and instead return a value of zero if the input string is a
>> valid units factor without any digits (e.g "k").
>
> This one is easier to test than the last. Just:
>
>   git config --int --default='m' some.key
>
> works. And even playing devil's advocate, I can't think of a case where
> anybody would rely on the current behavior.

Hmph, but --default=" m" would not be caught with the patch with the
same error, but is still a valid way to say zero mega unit.

Personally I do not see if this one is worth worrying about at all,
even though the fixes in 1/3 and 3/3 may be more worthwhile.

THanks.

Re: [PATCH 2/3] config: require at least one digit when parsing numbers

[Jeff King]

On Sat, Oct 22, 2022 at 10:51:23AM -0700, Junio C Hamano wrote:

> > This one is easier to test than the last. Just:
> >
> >   git config --int --default='m' some.key
> >
> > works. And even playing devil's advocate, I can't think of a case where
> > anybody would rely on the current behavior.
> 
> Hmph, but --default=" m" would not be caught with the patch with the
> same error, but is still a valid way to say zero mega unit.

I assumed that --default=" m" was supposed to be an error. It is already
in the current code (because strtoimax doesn't skip the whitespace).

I admit I don't care much either way, though.

-Peff

Re: [PATCH 2/3] config: require at least one digit when parsing numbers

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I admit I don't care much either way, though.

Me neither.  If the patch were to make --default=m mean "one mega unit"
instead of "zero mega unit", then I may care a bit more, though.

Re: [PATCH 2/3] config: require at least one digit when parsing numbers

[Phillip Wood]

Hi Peff

On 21/10/2022 21:17, Jeff King wrote:
> On Fri, Oct 21, 2022 at 01:45:13PM +0000, Phillip Wood via GitGitGadget wrote:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> If the input to strtoimax() or strtoumax() does not contain any digits
>> then they return zero and set `end` to point to the start of the input
>> string.  git_parse_[un]signed() do not check `end` and so fail to return
>> an error and instead return a value of zero if the input string is a
>> valid units factor without any digits (e.g "k").
> 
> This one is easier to test than the last. Just:
> 
>    git config --int --default='m' some.key

Thanks for posting that, I'd forgotten about the --int flag for git config.

Best Wishes

Phillip

> works. And even playing devil's advocate, I can't think of a case where
> anybody would rely on the current behavior.
> 
> -Peff

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

[Phillip Wood via GitGitGadget]

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`. To fix this avoid negating `val` when it is negative by
having separate overflow checks for positive and negative values.

An alternative would be to special case INTMAX_MIN before negating `val`
as it is always out of range. That would enable us to keep the existing
code but I'm not sure that the current two-stage check is any clearer
than the new version.

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");
 
 		errno = 0;
 		val = strtoimax(value, &end, 0);
@@ -1176,9 +1178,8 @@ static int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)
 			errno = EINVAL;
 			return 0;
 		}
-		uval = val < 0 ? -val : val;
-		if (unsigned_mult_overflows(factor, uval) ||
-		    factor * uval > max) {
+		if ((val < 0 && -max / factor > val) ||
+		    (val > 0 && max / factor < val)) {
 			errno = ERANGE;
 			return 0;
 		}
-- 
gitgitgadget

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

[Junio C Hamano]

"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?

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

[René Scharfe]

Am 21.10.22 um 20:31 schrieb Junio C Hamano:
> "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?

Hmm, if minimum and maximum are not symmetric, then we need to supply
both, don't we?

--- >8 ---
Subject: [PATCH] config: let git_parse_signed() check minimum

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`.

Avoid overflow during sign inversion by supplying the minimum value to
the function as well.  Make `factor` signed to avoid promoting the
division results in the limit check line to unsigned, but check whether
it's positive.

Add a new macro, minimum_signed_value_of_type, and use it in the callers
of git_parse_signed() to provide the newly required minimum value.  It
calculates the minimum for a given type using division instead of right
shift because the latter is implementation-defined for signed types,
and we need an arithmetic right shift here, not a logical one.

Original-patch-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
git_config_get_expiry_in_days() could use git_parse_int(), but that's
a different topic.

 config.c          | 28 +++++++++++++++++-----------
 git-compat-util.h |  2 ++
 2 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/config.c b/config.c
index cbb5a3bab7..7cf196dc84 100644
--- a/config.c
+++ b/config.c
@@ -1155,26 +1155,24 @@ static uintmax_t get_unit_factor(const char *end)
 	return 0;
 }

-static int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)
+static int git_parse_signed(const char *value, intmax_t *ret,
+			    intmax_t min, intmax_t max)
 {
 	if (value && *value) {
 		char *end;
 		intmax_t val;
-		uintmax_t uval;
-		uintmax_t factor;
+		intmax_t factor;

 		errno = 0;
 		val = strtoimax(value, &end, 0);
 		if (errno == ERANGE)
 			return 0;
 		factor = get_unit_factor(end);
-		if (!factor) {
+		if (factor < 1) {
 			errno = EINVAL;
 			return 0;
 		}
-		uval = val < 0 ? -val : val;
-		if (unsigned_mult_overflows(factor, uval) ||
-		    factor * uval > max) {
+		if (val < min / factor || val > max / factor) {
 			errno = ERANGE;
 			return 0;
 		}
@@ -1218,7 +1216,9 @@ static int git_parse_unsigned(const char *value, uintmax_t *ret, uintmax_t max)
 static int git_parse_int(const char *value, int *ret)
 {
 	intmax_t tmp;
-	if (!git_parse_signed(value, &tmp, maximum_signed_value_of_type(int)))
+	if (!git_parse_signed(value, &tmp,
+			      minimum_signed_value_of_type(int),
+			      maximum_signed_value_of_type(int)))
 		return 0;
 	*ret = tmp;
 	return 1;
@@ -1227,7 +1227,9 @@ static int git_parse_int(const char *value, int *ret)
 static int git_parse_int64(const char *value, int64_t *ret)
 {
 	intmax_t tmp;
-	if (!git_parse_signed(value, &tmp, maximum_signed_value_of_type(int64_t)))
+	if (!git_parse_signed(value, &tmp,
+			      minimum_signed_value_of_type(int64_t),
+			      maximum_signed_value_of_type(int64_t)))
 		return 0;
 	*ret = tmp;
 	return 1;
@@ -1245,7 +1247,9 @@ int git_parse_ulong(const char *value, unsigned long *ret)
 int git_parse_ssize_t(const char *value, ssize_t *ret)
 {
 	intmax_t tmp;
-	if (!git_parse_signed(value, &tmp, maximum_signed_value_of_type(ssize_t)))
+	if (!git_parse_signed(value, &tmp,
+			      minimum_signed_value_of_type(ssize_t),
+			      maximum_signed_value_of_type(ssize_t)))
 		return 0;
 	*ret = tmp;
 	return 1;
@@ -2751,7 +2755,9 @@ int git_config_get_expiry_in_days(const char *key, timestamp_t *expiry, timestam
 	if (git_config_get_string_tmp(key, &expiry_string))
 		return 1; /* no such thing */

-	if (git_parse_signed(expiry_string, &days, maximum_signed_value_of_type(int))) {
+	if (git_parse_signed(expiry_string, &days,
+			     minimum_signed_value_of_type(int),
+			     maximum_signed_value_of_type(int))) {
 		const int scale = 86400;
 		*expiry = now - days * scale;
 		return 0;
diff --git a/git-compat-util.h b/git-compat-util.h
index ea53ea4a78..35425c373b 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -112,6 +112,8 @@ struct strbuf;

 #define bitsizeof(x)  (CHAR_BIT * sizeof(x))

+#define minimum_signed_value_of_type(a) \
+    (INTMAX_MIN / ((intmax_t)1 << (bitsizeof(intmax_t) - bitsizeof(a))))
 #define maximum_signed_value_of_type(a) \
     (INTMAX_MAX >> (bitsizeof(intmax_t) - bitsizeof(a)))

--
2.38.1

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

[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.

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.

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

[René Scharfe]

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.

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

[Phillip Wood]

On 23/10/2022 06:57, René Scharfe wrote:
> 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.

I'm happy to go either way, while I agree passing separate limits to 
allow INT_MIN is technically correct I'm not sure anyone has complained 
that the current code is too restrictive.

Best Wishes

Phillip

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

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

[René Scharfe]

Am 25.10.22 um 12:00 schrieb Phillip Wood:
> On 23/10/2022 06:57, René Scharfe wrote:
>>
>> 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.
>
> I'm happy to go either way, while I agree passing separate limits to
> allow INT_MIN is technically correct I'm not sure anyone has
> complained that the current code is too restrictive.
Right.  Having separate patches for the different aspects (your fix,
simplification due to adding a min parameter, range extension made
possible by that) is probably best to discuss the merits of each.

René

[PATCH v2 0/3] a few config integer parsing fixes

[Phillip Wood via GitGitGadget]

This series fixes some issues I noticed when reading the integer parsing
code in config.c

 * git_parse_unsigned() does not reject negative values
 * git_parse_[un]signed() accept a units specifier without any digits
 * git_parse_signed() has in integer overflow when parsing MAXINT_MIN

Thanks to everyone who commented on V1. I've updated patches 1 & 2 to
include the tests suggested by peff and added tests for OPT_MAGNITUDE() as
that uses the same code path.

Cover Letter for V1:

Ideally we'd have a test tool to unit test functions like this, I haven't
found time to write that yet. cc'ing René for patch 3 as he was the last
person to touch that code.

Phillip Wood (3):
  git_parse_unsigned: reject negative values
  config: require at least one digit when parsing numbers
  git_parse_signed(): avoid integer overflow

 config.c                 | 24 +++++++++++++++++++-----
 t/t0040-parse-options.sh | 12 ++++++++++++
 t/t1050-large.sh         |  6 ++++++
 t/t1300-config.sh        |  6 ++++++
 4 files changed, 43 insertions(+), 5 deletions(-)


base-commit: e85701b4af5b7c2a9f3a1b07858703318dce365d
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1389%2Fphillipwood%2Fconfig-integer-parsing-fixes-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1389/phillipwood/config-integer-parsing-fixes-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1389

Range-diff vs v1:

 1:  9c8440e5e82 ! 1:  d1ac79909b9 git_parse_unsigned: reject negative values
     @@ Commit message
          string that contains '-' as we do in strtoul_ui(). I've chosen to treat
          negative numbers as invalid input and set errno to EINVAL rather than
          ERANGE one the basis that they are never acceptable if we're looking for
     -    a unsigned integer.
     +    a unsigned integer. This is also consistent with the existing behavior
     +    of rejecting "1–2" with EINVAL.
      
     +    As we do not have unit tests for this function it is tested indirectly
     +    by checking that negative values of reject for core.bigFileThreshold are
     +    rejected. As this function is also used by OPT_MAGNITUDE() a test is
     +    added to check that rejects negative values too.
     +
     +    Helped-by: Jeff King <peff@peff.net>
          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
      
       ## config.c ##
     @@ config.c: static int git_parse_unsigned(const char *value, uintmax_t *ret, uintm
       		errno = 0;
       		val = strtoumax(value, &end, 0);
       		if (errno == ERANGE)
     +
     + ## t/t0040-parse-options.sh ##
     +@@ t/t0040-parse-options.sh: test_expect_success 'subcommands are incompatible with KEEP_DASHDASH unless in c
     + 	grep ^BUG err
     + '
     + 
     ++test_expect_success 'negative magnitude' '
     ++	test_must_fail test-tool parse-options --magnitude -1 >out 2>err &&
     ++	grep "non-negative integer" err &&
     ++	test_must_be_empty out
     ++'
     + test_done
     +
     + ## t/t1050-large.sh ##
     +@@ t/t1050-large.sh: test_description='adding and checking out large blobs'
     + 
     + . ./test-lib.sh
     + 
     ++test_expect_success 'core.bigFileThreshold must be non-negative' '
     ++	test_must_fail git -c core.bigFileThreshold=-1 rev-parse >out 2>err &&
     ++	grep "bad numeric config value" err &&
     ++	test_must_be_empty out
     ++'
     ++
     + test_expect_success setup '
     + 	# clone does not allow us to pass core.bigfilethreshold to
     + 	# new repos, so set core.bigfilethreshold globally
 2:  cd753602e48 ! 2:  54f2ebefa0d config: require at least one digit when parsing numbers
     @@ Commit message
          an error and instead return a value of zero if the input string is a
          valid units factor without any digits (e.g "k").
      
     +    Tests are added to check that 'git config --int' and OPT_MAGNITUDE()
     +    reject a units specifier without a leading digit.
     +
     +    Helped-by: Jeff King <peff@peff.net>
          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
      
       ## config.c ##
     @@ config.c: static int git_parse_unsigned(const char *value, uintmax_t *ret, uintm
       		factor = get_unit_factor(end);
       		if (!factor) {
       			errno = EINVAL;
     +
     + ## t/t0040-parse-options.sh ##
     +@@ t/t0040-parse-options.sh: test_expect_success 'negative magnitude' '
     + 	grep "non-negative integer" err &&
     + 	test_must_be_empty out
     + '
     ++
     ++test_expect_success 'magnitude with units but no numbers' '
     ++	test_must_fail test-tool parse-options --magnitude m >out 2>err &&
     ++	grep "non-negative integer" err &&
     ++	test_must_be_empty out
     ++'
     ++
     + test_done
     +
     + ## t/t1300-config.sh ##
     +@@ t/t1300-config.sh: test_expect_success '--type rejects unknown specifiers' '
     + 	test_i18ngrep "unrecognized --type argument" error
     + '
     + 
     ++test_expect_success '--type=int requires at least one digit' '
     ++	test_must_fail git config --type int --default m some.key >out 2>error &&
     ++	grep "bad numeric config value" error &&
     ++	test_must_be_empty out
     ++'
     ++
     + test_expect_success '--replace-all does not invent newlines' '
     + 	q_to_tab >.git/config <<-\EOF &&
     + 	[abc]key
 3:  f058f391c38 = 3:  673e6f1ab93 git_parse_signed(): avoid integer overflow

-- 
gitgitgadget

[PATCH v2 1/3] git_parse_unsigned: reject negative values

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

git_parse_unsigned() relies on strtoumax() which unfortunately parses
negative values as large positive integers. Fix this by rejecting any
string that contains '-' as we do in strtoul_ui(). I've chosen to treat
negative numbers as invalid input and set errno to EINVAL rather than
ERANGE one the basis that they are never acceptable if we're looking for
a unsigned integer. This is also consistent with the existing behavior
of rejecting "1–2" with EINVAL.

As we do not have unit tests for this function it is tested indirectly
by checking that negative values of reject for core.bigFileThreshold are
rejected. As this function is also used by OPT_MAGNITUDE() a test is
added to check that rejects negative values too.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 config.c                 | 5 +++++
 t/t0040-parse-options.sh | 5 +++++
 t/t1050-large.sh         | 6 ++++++
 3 files changed, 16 insertions(+)

diff --git a/config.c b/config.c
index cbb5a3bab74..d5069d4f01d 100644
--- a/config.c
+++ b/config.c
@@ -1193,6 +1193,11 @@ static int git_parse_unsigned(const char *value, uintmax_t *ret, uintmax_t max)
 		uintmax_t val;
 		uintmax_t factor;
 
+		/* negative values would be accepted by strtoumax */
+		if (strchr(value, '-')) {
+			errno = EINVAL;
+			return 0;
+		}
 		errno = 0;
 		val = strtoumax(value, &end, 0);
 		if (errno == ERANGE)
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index 5cc62306e39..64d2327b744 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -709,4 +709,9 @@ test_expect_success 'subcommands are incompatible with KEEP_DASHDASH unless in c
 	grep ^BUG err
 '
 
+test_expect_success 'negative magnitude' '
+	test_must_fail test-tool parse-options --magnitude -1 >out 2>err &&
+	grep "non-negative integer" err &&
+	test_must_be_empty out
+'
 test_done
diff --git a/t/t1050-large.sh b/t/t1050-large.sh
index 4f3aa17c994..c71932b0242 100755
--- a/t/t1050-large.sh
+++ b/t/t1050-large.sh
@@ -5,6 +5,12 @@ test_description='adding and checking out large blobs'
 
 . ./test-lib.sh
 
+test_expect_success 'core.bigFileThreshold must be non-negative' '
+	test_must_fail git -c core.bigFileThreshold=-1 rev-parse >out 2>err &&
+	grep "bad numeric config value" err &&
+	test_must_be_empty out
+'
+
 test_expect_success setup '
 	# clone does not allow us to pass core.bigfilethreshold to
 	# new repos, so set core.bigfilethreshold globally
-- 
gitgitgadget

Re: [PATCH v2 1/3] git_parse_unsigned: reject negative values

[Ævar Arnfjörð Bjarmason]

On Wed, Nov 09 2022, Phillip Wood via GitGitGadget wrote:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> git_parse_unsigned() relies on strtoumax() which unfortunately parses
> negative values as large positive integers. Fix this by rejecting any
> string that contains '-' as we do in strtoul_ui(). I've chosen to treat
> negative numbers as invalid input and set errno to EINVAL rather than
> ERANGE one the basis that they are never acceptable if we're looking for
> a unsigned integer. This is also consistent with the existing behavior
> of rejecting "1–2" with EINVAL.
>
> As we do not have unit tests for this function it is tested indirectly
> by checking that negative values of reject for core.bigFileThreshold are
> rejected. As this function is also used by OPT_MAGNITUDE() a test is
> added to check that rejects negative values too.
>
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  config.c                 | 5 +++++
>  t/t0040-parse-options.sh | 5 +++++
>  t/t1050-large.sh         | 6 ++++++
>  3 files changed, 16 insertions(+)
>
> diff --git a/config.c b/config.c
> index cbb5a3bab74..d5069d4f01d 100644
> --- a/config.c
> +++ b/config.c
> @@ -1193,6 +1193,11 @@ static int git_parse_unsigned(const char *value, uintmax_t *ret, uintmax_t max)
>  		uintmax_t val;
>  		uintmax_t factor;
>  
> +		/* negative values would be accepted by strtoumax */
> +		if (strchr(value, '-')) {
> +			errno = EINVAL;
> +			return 0;
> +		}
>  		errno = 0;
>  		val = strtoumax(value, &end, 0);
>  		if (errno == ERANGE)

There's nothing wrong with this, but since the topic here is "some
issues I noticed" here's another one: We don't actually care if you set
"errno = EINVAL" here in particular, just as long as it's not "ERANGE",
anything else will do.

So, not worth a re-roll in itself, but maybe a prep patch (or follow-up)
to do this would be nice? to make sure this errno handling is
"reachable"?

diff --git a/config.c b/config.c
index ff4ea29784b..33d05fde0ea 100644
--- a/config.c
+++ b/config.c
@@ -1260,9 +1260,12 @@ NORETURN
 static void die_bad_number(const char *name, const char *value)
 {
 	const char *error_type = (errno == ERANGE) ?
-		N_("out of range") : N_("invalid unit");
+		N_("out of range") : errno == EINVAL ? N_("invalid unit") : NULL;
 	const char *bad_numeric = N_("bad numeric config value '%s' for '%s': %s");
 
+	if (!error_type)
+		BUG("unhandled errno %d: %s", errno, strerror(errno));
+
 	if (!value)
 		value = "";
 

[PATCH v2 2/3] config: require at least one digit when parsing numbers

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

If the input to strtoimax() or strtoumax() does not contain any digits
then they return zero and set `end` to point to the start of the input
string.  git_parse_[un]signed() do not check `end` and so fail to return
an error and instead return a value of zero if the input string is a
valid units factor without any digits (e.g "k").

Tests are added to check that 'git config --int' and OPT_MAGNITUDE()
reject a units specifier without a leading digit.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 config.c                 | 8 ++++++++
 t/t0040-parse-options.sh | 7 +++++++
 t/t1300-config.sh        | 6 ++++++
 3 files changed, 21 insertions(+)

diff --git a/config.c b/config.c
index d5069d4f01d..b7fb68026d8 100644
--- a/config.c
+++ b/config.c
@@ -1167,6 +1167,10 @@ static int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)
 		val = strtoimax(value, &end, 0);
 		if (errno == ERANGE)
 			return 0;
+		if (end == value) {
+			errno = EINVAL;
+			return 0;
+		}
 		factor = get_unit_factor(end);
 		if (!factor) {
 			errno = EINVAL;
@@ -1202,6 +1206,10 @@ static int git_parse_unsigned(const char *value, uintmax_t *ret, uintmax_t max)
 		val = strtoumax(value, &end, 0);
 		if (errno == ERANGE)
 			return 0;
+		if (end == value) {
+			errno = EINVAL;
+			return 0;
+		}
 		factor = get_unit_factor(end);
 		if (!factor) {
 			errno = EINVAL;
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index 64d2327b744..7d7ecfd5716 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -714,4 +714,11 @@ test_expect_success 'negative magnitude' '
 	grep "non-negative integer" err &&
 	test_must_be_empty out
 '
+
+test_expect_success 'magnitude with units but no numbers' '
+	test_must_fail test-tool parse-options --magnitude m >out 2>err &&
+	grep "non-negative integer" err &&
+	test_must_be_empty out
+'
+
 test_done
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index c6661e61af5..2575279ab84 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -2228,6 +2228,12 @@ test_expect_success '--type rejects unknown specifiers' '
 	test_i18ngrep "unrecognized --type argument" error
 '
 
+test_expect_success '--type=int requires at least one digit' '
+	test_must_fail git config --type int --default m some.key >out 2>error &&
+	grep "bad numeric config value" error &&
+	test_must_be_empty out
+'
+
 test_expect_success '--replace-all does not invent newlines' '
 	q_to_tab >.git/config <<-\EOF &&
 	[abc]key
-- 
gitgitgadget

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

[Phillip Wood via GitGitGadget]

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`. To fix this avoid negating `val` when it is negative by
having separate overflow checks for positive and negative values.

An alternative would be to special case INTMAX_MIN before negating `val`
as it is always out of range. That would enable us to keep the existing
code but I'm not sure that the current two-stage check is any clearer
than the new version.

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");
 
 		errno = 0;
 		val = strtoimax(value, &end, 0);
@@ -1176,9 +1178,8 @@ static int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)
 			errno = EINVAL;
 			return 0;
 		}
-		uval = val < 0 ? -val : val;
-		if (unsigned_mult_overflows(factor, uval) ||
-		    factor * uval > max) {
+		if ((val < 0 && -max / factor > val) ||
+		    (val > 0 && max / factor < val)) {
 			errno = ERANGE;
 			return 0;
 		}
-- 
gitgitgadget

Re: [PATCH v2 0/3] a few config integer parsing fixes

[Taylor Blau]

On Wed, Nov 09, 2022 at 02:16:25PM +0000, Phillip Wood via GitGitGadget wrote:
> This series fixes some issues I noticed when reading the integer parsing
> code in config.c
>
>  * git_parse_unsigned() does not reject negative values
>  * git_parse_[un]signed() accept a units specifier without any digits
>  * git_parse_signed() has in integer overflow when parsing MAXINT_MIN
>
> Thanks to everyone who commented on V1. I've updated patches 1 & 2 to
> include the tests suggested by peff and added tests for OPT_MAGNITUDE() as
> that uses the same code path.

Having read this round and the discussion on the original version, I
think that this is ready to start merging down.

That said, it would be nice to hear from some other reviewer(s) who
took a look or are interested in this area.

Thanks,
Taylor