| Message ID | d1ac79909b9e777cae40a6a301e5cfd988c5f9d7.1668003388.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 84356ff7709bd45c7e61632f1b837a7144a5178f |
| Headers | show |
| Series | a few config integer parsing fixes | expand |
On Wed, Nov 09 2022, Phillip Wood via GitGitGadget wrote: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > 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 <peff@peff.net> > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > --- > 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 cbb5a3bab74..d5069d4f01d 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) There's nothing wrong with this, but since the topic here is "some issues I noticed" here's another one: We don't actually care if you set "errno = EINVAL" here in particular, just as long as it's not "ERANGE", anything else will do. So, not worth a re-roll in itself, but maybe a prep patch (or follow-up) to do this would be nice? to make sure this errno handling is "reachable"? diff --git a/config.c b/config.c index ff4ea29784b..33d05fde0ea 100644 --- a/config.c +++ b/config.c @@ -1260,9 +1260,12 @@ NORETURN static void die_bad_number(const char *name, const char *value) { const char *error_type = (errno == ERANGE) ? - N_("out of range") : N_("invalid unit"); + N_("out of range") : errno == EINVAL ? N_("invalid unit") : NULL; const char *bad_numeric = N_("bad numeric config value '%s' for '%s': %s"); + if (!error_type) + BUG("unhandled errno %d: %s", errno, strerror(errno)); + if (!value) value = "";
diff --git a/config.c b/config.c index cbb5a3bab74..d5069d4f01d 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 5cc62306e39..64d2327b744 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 4f3aa17c994..c71932b0242 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