a few config integer parsing fixes

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