From 84356ff7709bd45c7e61632f1b837a7144a5178f Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Wed, 9 Nov 2022 14:16:26 +0000 Subject: [PATCH 1/3] git_parse_unsigned: reject negative values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Phillip Wood Signed-off-by: Taylor Blau --- 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 cbb5a3bab7..d5069d4f01 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 5cc62306e3..64d2327b74 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 4f3aa17c99..c71932b024 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 From 7595c0ece1d45ca540f26cecf485285f5ce8186f Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Wed, 9 Nov 2022 14:16:27 +0000 Subject: [PATCH 2/3] config: require at least one digit when parsing numbers 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 Signed-off-by: Phillip Wood Signed-off-by: Taylor Blau --- 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 d5069d4f01..b7fb68026d 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 64d2327b74..7d7ecfd571 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 c6661e61af..2575279ab8 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 From 14770cf0de218cc373e7d286b864f526e5ea2840 Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Wed, 9 Nov 2022 14:16:28 +0000 Subject: [PATCH 3/3] git_parse_signed(): avoid integer overflow 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 Signed-off-by: Taylor Blau --- config.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/config.c b/config.c index b7fb68026d..aad3e00341 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; }