From: "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: "René Scharfe" <l.s.r@web.de>, "Jeff King" <peff@peff.net>,
"Phillip Wood" <phillip.wood@dunelm.org.uk>
Subject: [PATCH v2 0/3] a few config integer parsing fixes
Date: Wed, 09 Nov 2022 14:16:25 +0000 [thread overview]
Message-ID: <pull.1389.v2.git.1668003388.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.1389.git.1666359915.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
next prev parent reply other threads:[~2022-11-09 14:16 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-21 13:45 [PATCH 0/3] a few config integer parsing fixes Phillip Wood via GitGitGadget
2022-10-21 13:45 ` [PATCH 1/3] git_parse_unsigned: reject negative values Phillip Wood via GitGitGadget
2022-10-21 18:09 ` Junio C Hamano
2022-10-21 20:13 ` Jeff King
2022-10-22 17:54 ` Junio C Hamano
2022-10-21 13:45 ` [PATCH 2/3] config: require at least one digit when parsing numbers Phillip Wood via GitGitGadget
2022-10-21 18:19 ` Junio C Hamano
2022-10-25 9:54 ` Phillip Wood
2022-10-25 16:08 ` Junio C Hamano
2022-10-21 20:17 ` Jeff King
2022-10-22 17:51 ` Junio C Hamano
2022-10-22 20:25 ` Jeff King
2022-10-22 21:00 ` Junio C Hamano
2022-10-25 9:55 ` Phillip Wood
2022-10-21 13:45 ` [PATCH 3/3] git_parse_signed(): avoid integer overflow Phillip Wood via GitGitGadget
2022-10-21 18:31 ` Junio C Hamano
2022-10-22 8:09 ` René Scharfe
2022-10-22 16:51 ` Junio C Hamano
2022-10-23 5:57 ` René Scharfe
2022-10-25 10:00 ` Phillip Wood
2022-10-26 11:01 ` René Scharfe
2022-11-09 14:16 ` Phillip Wood via GitGitGadget [this message]
2022-11-09 14:16 ` [PATCH v2 1/3] git_parse_unsigned: reject negative values Phillip Wood via GitGitGadget
2022-11-09 15:57 ` Ævar Arnfjörð Bjarmason
2022-11-09 14:16 ` [PATCH v2 2/3] config: require at least one digit when parsing numbers Phillip Wood via GitGitGadget
2022-11-09 14:16 ` [PATCH v2 3/3] git_parse_signed(): avoid integer overflow Phillip Wood via GitGitGadget
2022-11-10 2:35 ` [PATCH v2 0/3] a few config integer parsing fixes Taylor Blau
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=pull.1389.v2.git.1668003388.gitgitgadget@gmail.com \
--to=gitgitgadget@gmail.com \
--cc=git@vger.kernel.org \
--cc=l.s.r@web.de \
--cc=peff@peff.net \
--cc=phillip.wood@dunelm.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).