a few config integer parsing fixes

From: Phillip Wood <[email protected]>

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.

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

index cbb5a3bab74..d5069d4f01d 100644
@@ -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

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

From: Phillip Wood <[email protected]>

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").

---
 config.c | 8 ++++++++
 1 file changed, 8 insertions(+)

index d5069d4f01d..b7fb68026d8 100644
@@ -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

From: Phillip Wood <[email protected]>

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.

---
 config.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

index b7fb68026d8..aad3e00341d 100644
@@ -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

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.

It is a problem that is worth looking into.

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

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

Am 21.10.22 um 20:31 schrieb Junio C Hamano:

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.

---
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(-)

index cbb5a3bab7..7cf196dc84 100644
@@ -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;

index ea53ea4a78..35425c373b 100644
@@ -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

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.

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.

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.

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

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.

Am 22.10.22 um 18:51 schrieb Junio C Hamano:

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

Hi Junio

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

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

Hi Peff

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

Best Wishes

Phillip

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

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

Am 25.10.22 um 12:00 schrieb Phillip Wood:

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é

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 <[email protected]>

## 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 <[email protected]>

## 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

From: Phillip Wood <[email protected]>

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.

---
 config.c                 | 5 +++++
 t/t0040-parse-options.sh | 5 +++++
 t/t1050-large.sh         | 6 ++++++
 3 files changed, 16 insertions(+)

index cbb5a3bab74..d5069d4f01d 100644
@@ -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)

index 5cc62306e39..64d2327b744 100755
@@ -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

index 4f3aa17c994..c71932b0242 100755
@@ -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

From: Phillip Wood <[email protected]>

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.

---
 config.c                 | 8 ++++++++
 t/t0040-parse-options.sh | 7 +++++++
 t/t1300-config.sh        | 6 ++++++
 3 files changed, 21 insertions(+)

index d5069d4f01d..b7fb68026d8 100644
@@ -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;

index 64d2327b744..7d7ecfd5716 100755
@@ -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

index c6661e61af5..2575279ab84 100755
@@ -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

From: Phillip Wood <[email protected]>

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.

---
 config.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

index b7fb68026d8..aad3e00341d 100644
@@ -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

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