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

["Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com>]

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