* [PATCH 0/3] a few config integer parsing fixes @ 2022-10-21 13:45 Phillip Wood via GitGitGadget 2022-10-21 13:45 ` [PATCH 1/3] git_parse_unsigned: reject negative values Phillip Wood via GitGitGadget ` (3 more replies) 0 siblings, 4 replies; 27+ messages in thread From: Phillip Wood via GitGitGadget @ 2022-10-21 13:45 UTC (permalink / raw) To: git; +Cc: René Scharfe, Phillip Wood 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 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 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) base-commit: e85701b4af5b7c2a9f3a1b07858703318dce365d Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1389%2Fphillipwood%2Fconfig-integer-parsing-fixes-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1389/phillipwood/config-integer-parsing-fixes-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/1389 -- gitgitgadget ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 1/3] git_parse_unsigned: reject negative values 2022-10-21 13:45 [PATCH 0/3] a few config integer parsing fixes Phillip Wood via GitGitGadget @ 2022-10-21 13:45 ` Phillip Wood via GitGitGadget 2022-10-21 18:09 ` Junio C Hamano 2022-10-21 20:13 ` Jeff King 2022-10-21 13:45 ` [PATCH 2/3] config: require at least one digit when parsing numbers Phillip Wood via GitGitGadget ` (2 subsequent siblings) 3 siblings, 2 replies; 27+ messages in thread From: Phillip Wood via GitGitGadget @ 2022-10-21 13:45 UTC (permalink / raw) To: git; +Cc: René Scharfe, Phillip Wood, Phillip Wood 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. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- config.c | 5 +++++ 1 file changed, 5 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) -- gitgitgadget ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH 1/3] git_parse_unsigned: reject negative values 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 1 sibling, 0 replies; 27+ messages in thread From: Junio C Hamano @ 2022-10-21 18:09 UTC (permalink / raw) To: Phillip Wood via GitGitGadget; +Cc: git, René Scharfe, Phillip Wood "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > 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. And the code now would reject something like "43-21" because it does not insist that "-" must be used for a sign, so it makes EINVAL doubly appropriate, I would think. In the original code, "43-21" would have been parsed as "43" (by strtoumax) followed by "-" (which is rejected by get_unit_factor() and yielded EINVAL, so this change would not change the behaviour for such an input, if we receive EINVAL when we see a "-". A devil's advocate would consider if we ever want to have a unit factor that has "-" in it (e.g. "10000e-3" == 10). I can buy it if we want to declare that it is not worth supporting. > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > --- > config.c | 5 +++++ > 1 file changed, 5 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) ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/3] git_parse_unsigned: reject negative values 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 1 sibling, 1 reply; 27+ messages in thread From: Jeff King @ 2022-10-21 20:13 UTC (permalink / raw) To: Phillip Wood via GitGitGadget; +Cc: git, René Scharfe, Phillip Wood On Fri, Oct 21, 2022 at 01:45:12PM +0000, Phillip Wood via GitGitGadget wrote: > 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. Certainly this seems like the right thing to do for a function parsing an unsigned value. It would be nice if we could demonstrate the visible effect with a test, though (and of course catch any later regressions). Sadly "git config" doesn't let you ask to parse an unsigned type. But we can find things in the core.* region that are parsed by default (and not likely to change, as they represent file/memory sizes). Like: git -c core.bigFileThreshold=-1 rev-parse which quietly passes before your patch, but fails after. It does make me wonder if anybody uses a negative value like this in the wild for "no limit", as it does what you might imagine currently (I get 2^64-1). -Peff ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/3] git_parse_unsigned: reject negative values 2022-10-21 20:13 ` Jeff King @ 2022-10-22 17:54 ` Junio C Hamano 0 siblings, 0 replies; 27+ messages in thread From: Junio C Hamano @ 2022-10-22 17:54 UTC (permalink / raw) To: Jeff King Cc: Phillip Wood via GitGitGadget, git, René Scharfe, Phillip Wood Jeff King <peff@peff.net> writes: > It does make me wonder if anybody uses a negative value like this in the > wild for "no limit", as it does what you might imagine currently (I get > 2^64-1). I vaguely recall complaints on the command line argument that used to take -1 to mean "practically unlimited" when its parsing got tightened. I wouldn't be surprised if we are making a new issue in the same category in the configuration file with this change. But we can switch to the signed variant when it becomes an issue, I guess. ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 2/3] config: require at least one digit when parsing numbers 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 13:45 ` Phillip Wood via GitGitGadget 2022-10-21 18:19 ` Junio C Hamano 2022-10-21 20:17 ` Jeff King 2022-10-21 13:45 ` [PATCH 3/3] git_parse_signed(): avoid integer overflow Phillip Wood via GitGitGadget 2022-11-09 14:16 ` [PATCH v2 0/3] a few config integer parsing fixes Phillip Wood via GitGitGadget 3 siblings, 2 replies; 27+ messages in thread From: Phillip Wood via GitGitGadget @ 2022-10-21 13:45 UTC (permalink / raw) To: git; +Cc: René Scharfe, Phillip Wood, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> 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"). Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- config.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/config.c b/config.c index d5069d4f01d..b7fb68026d8 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; -- gitgitgadget ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH 2/3] config: require at least one digit when parsing numbers 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-21 20:17 ` Jeff King 1 sibling, 1 reply; 27+ messages in thread From: Junio C Hamano @ 2022-10-21 18:19 UTC (permalink / raw) To: Phillip Wood via GitGitGadget; +Cc: git, René Scharfe, Phillip Wood "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > 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"). > > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > --- > config.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/config.c b/config.c > index d5069d4f01d..b7fb68026d8 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; > + } This means well, but doesn't strto*() family of functions silently ignore leading blanks, e.g. l = strtol(" 432k", &end, 0); ... l == 432, *end = k ... If you really want to reject a string with no number before the optional unit, end at this point may not match value. With " k" as input, value would point at the space at the beginning, and end would point at 'k'. It does not look _too_ bad if we just let such an empty string through and interpreted it as zero. Is that a problem? Who are we trying to help? > 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; ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/3] config: require at least one digit when parsing numbers 2022-10-21 18:19 ` Junio C Hamano @ 2022-10-25 9:54 ` Phillip Wood 2022-10-25 16:08 ` Junio C Hamano 0 siblings, 1 reply; 27+ messages in thread From: Phillip Wood @ 2022-10-25 9:54 UTC (permalink / raw) To: Junio C Hamano, Phillip Wood via GitGitGadget Cc: git, René Scharfe, Jeff King Hi Junio On 21/10/2022 19:19, Junio C Hamano wrote: > "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: >> @@ -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; >> + } > > This means well, but doesn't strto*() family of functions silently > ignore leading blanks, e.g. > > l = strtol(" 432k", &end, 0); > ... l == 432, *end = k ... > > If you really want to reject a string with no number before the > optional unit, end at this point may not match value. With " k" as > input, value would point at the space at the beginning, and end > would point at 'k'. It only skips the space if it sees a digit, if it does not find anything to convert it sets *end = start. Using peff's trick for testing this patch we can see there is no change in behavior if there is leading whitespace $ bin-wrappers/git config --int --default " m" some.key fatal: bad numeric config value ' m' for 'some.key': invalid unit $ git config --int --default " m" some.key fatal: bad numeric config value ' m' for 'some.key': invalid unit > It does not look _too_ bad if we just let such an empty string > through and interpreted it as zero. Is that a problem? Who are we > trying to help? My reasoning was that a single units factor is likely to be the result of some kind of bad edit and defaulting to zero when the user thought they set a large value is not likely to be helpful. Having said that I'm not that wedded to this patch if you feel it would be better to drop it. Best Wishes Phillip ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/3] config: require at least one digit when parsing numbers 2022-10-25 9:54 ` Phillip Wood @ 2022-10-25 16:08 ` Junio C Hamano 0 siblings, 0 replies; 27+ messages in thread From: Junio C Hamano @ 2022-10-25 16:08 UTC (permalink / raw) To: Phillip Wood Cc: Phillip Wood via GitGitGadget, git, René Scharfe, Jeff King Phillip Wood <phillip.wood123@gmail.com> writes: > On 21/10/2022 19:19, Junio C Hamano wrote: >> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: >>> @@ -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; >>> + } >> This means well, but doesn't strto*() family of functions silently >> ignore leading blanks, e.g. >> l = strtol(" 432k", &end, 0); >> ... l == 432, *end = k ... >> If you really want to reject a string with no number before the >> optional unit, end at this point may not match value. With " k" as >> input, value would point at the space at the beginning, and end >> would point at 'k'. > > It only skips the space if it sees a digit, if it does not find > anything to convert it sets *end = start. Yeah, thanks. Yes, my earlier observation was based on a faulty experiments, and the code posted as-is would be OK. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/3] config: require at least one digit when parsing numbers 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-21 20:17 ` Jeff King 2022-10-22 17:51 ` Junio C Hamano 2022-10-25 9:55 ` Phillip Wood 1 sibling, 2 replies; 27+ messages in thread From: Jeff King @ 2022-10-21 20:17 UTC (permalink / raw) To: Phillip Wood via GitGitGadget; +Cc: git, René Scharfe, Phillip Wood On Fri, Oct 21, 2022 at 01:45:13PM +0000, Phillip Wood via GitGitGadget wrote: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > 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"). This one is easier to test than the last. Just: git config --int --default='m' some.key works. And even playing devil's advocate, I can't think of a case where anybody would rely on the current behavior. -Peff ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/3] config: require at least one digit when parsing numbers 2022-10-21 20:17 ` Jeff King @ 2022-10-22 17:51 ` Junio C Hamano 2022-10-22 20:25 ` Jeff King 2022-10-25 9:55 ` Phillip Wood 1 sibling, 1 reply; 27+ messages in thread From: Junio C Hamano @ 2022-10-22 17:51 UTC (permalink / raw) To: Jeff King Cc: Phillip Wood via GitGitGadget, git, René Scharfe, Phillip Wood Jeff King <peff@peff.net> writes: > On Fri, Oct 21, 2022 at 01:45:13PM +0000, Phillip Wood via GitGitGadget wrote: > >> From: Phillip Wood <phillip.wood@dunelm.org.uk> >> >> 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"). > > This one is easier to test than the last. Just: > > git config --int --default='m' some.key > > works. And even playing devil's advocate, I can't think of a case where > anybody would rely on the current behavior. Hmph, but --default=" m" would not be caught with the patch with the same error, but is still a valid way to say zero mega unit. Personally I do not see if this one is worth worrying about at all, even though the fixes in 1/3 and 3/3 may be more worthwhile. THanks. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/3] config: require at least one digit when parsing numbers 2022-10-22 17:51 ` Junio C Hamano @ 2022-10-22 20:25 ` Jeff King 2022-10-22 21:00 ` Junio C Hamano 0 siblings, 1 reply; 27+ messages in thread From: Jeff King @ 2022-10-22 20:25 UTC (permalink / raw) To: Junio C Hamano Cc: Phillip Wood via GitGitGadget, git, René Scharfe, Phillip Wood On Sat, Oct 22, 2022 at 10:51:23AM -0700, Junio C Hamano wrote: > > This one is easier to test than the last. Just: > > > > git config --int --default='m' some.key > > > > works. And even playing devil's advocate, I can't think of a case where > > anybody would rely on the current behavior. > > Hmph, but --default=" m" would not be caught with the patch with the > same error, but is still a valid way to say zero mega unit. I assumed that --default=" m" was supposed to be an error. It is already in the current code (because strtoimax doesn't skip the whitespace). I admit I don't care much either way, though. -Peff ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/3] config: require at least one digit when parsing numbers 2022-10-22 20:25 ` Jeff King @ 2022-10-22 21:00 ` Junio C Hamano 0 siblings, 0 replies; 27+ messages in thread From: Junio C Hamano @ 2022-10-22 21:00 UTC (permalink / raw) To: Jeff King Cc: Phillip Wood via GitGitGadget, git, René Scharfe, Phillip Wood Jeff King <peff@peff.net> writes: > I admit I don't care much either way, though. Me neither. If the patch were to make --default=m mean "one mega unit" instead of "zero mega unit", then I may care a bit more, though. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/3] config: require at least one digit when parsing numbers 2022-10-21 20:17 ` Jeff King 2022-10-22 17:51 ` Junio C Hamano @ 2022-10-25 9:55 ` Phillip Wood 1 sibling, 0 replies; 27+ messages in thread From: Phillip Wood @ 2022-10-25 9:55 UTC (permalink / raw) To: Jeff King, Phillip Wood via GitGitGadget; +Cc: git, René Scharfe Hi Peff On 21/10/2022 21:17, Jeff King wrote: > On Fri, Oct 21, 2022 at 01:45:13PM +0000, Phillip Wood via GitGitGadget wrote: > >> From: Phillip Wood <phillip.wood@dunelm.org.uk> >> >> 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"). > > This one is easier to test than the last. Just: > > git config --int --default='m' some.key Thanks for posting that, I'd forgotten about the --int flag for git config. Best Wishes Phillip > works. And even playing devil's advocate, I can't think of a case where > anybody would rely on the current behavior. > > -Peff ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 3/3] git_parse_signed(): avoid integer overflow 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 13:45 ` [PATCH 2/3] config: require at least one digit when parsing numbers Phillip Wood via GitGitGadget @ 2022-10-21 13:45 ` Phillip Wood via GitGitGadget 2022-10-21 18:31 ` Junio C Hamano 2022-11-09 14:16 ` [PATCH v2 0/3] a few config integer parsing fixes Phillip Wood via GitGitGadget 3 siblings, 1 reply; 27+ messages in thread From: Phillip Wood via GitGitGadget @ 2022-10-21 13:45 UTC (permalink / raw) To: git; +Cc: René Scharfe, Phillip Wood, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> 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 <phillip.wood@dunelm.org.uk> --- config.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/config.c b/config.c index b7fb68026d8..aad3e00341d 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; } -- gitgitgadget ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH 3/3] git_parse_signed(): avoid integer overflow 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 0 siblings, 1 reply; 27+ messages in thread From: Junio C Hamano @ 2022-10-21 18:31 UTC (permalink / raw) To: Phillip Wood via GitGitGadget; +Cc: git, René Scharfe, Phillip Wood "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > 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`. It is a problem that is worth looking into. > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > --- > config.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/config.c b/config.c > index b7fb68026d8..aad3e00341d 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"); In parse_signed(), would we expect to accept end-user input that is a negative integer? We must. Otherwise we would not be calling a "signed" parser. Now, are there cases where the valid value range is bounded by a negative integer at the top? No current callers may pass such a value, but is it reasonable to add such a new constraints to an existing API function? ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 3/3] git_parse_signed(): avoid integer overflow 2022-10-21 18:31 ` Junio C Hamano @ 2022-10-22 8:09 ` René Scharfe 2022-10-22 16:51 ` Junio C Hamano 0 siblings, 1 reply; 27+ messages in thread From: René Scharfe @ 2022-10-22 8:09 UTC (permalink / raw) To: Junio C Hamano, Phillip Wood via GitGitGadget; +Cc: git, Phillip Wood Am 21.10.22 um 20:31 schrieb Junio C Hamano: > "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> From: Phillip Wood <phillip.wood@dunelm.org.uk> >> >> 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`. > > It is a problem that is worth looking into. > >> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> >> --- >> config.c | 11 ++++++----- >> 1 file changed, 6 insertions(+), 5 deletions(-) >> >> diff --git a/config.c b/config.c >> index b7fb68026d8..aad3e00341d 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"); > > In parse_signed(), would we expect to accept end-user input that is > a negative integer? We must. Otherwise we would not be calling a > "signed" parser. Now, are there cases where the valid value range > is bounded by a negative integer at the top? No current callers may > pass such a value, but is it reasonable to add such a new constraints > to an existing API function? Hmm, if minimum and maximum are not symmetric, then we need to supply both, don't we? --- >8 --- Subject: [PATCH] config: let git_parse_signed() check minimum 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`. Avoid overflow during sign inversion by supplying the minimum value to the function as well. Make `factor` signed to avoid promoting the division results in the limit check line to unsigned, but check whether it's positive. Add a new macro, minimum_signed_value_of_type, and use it in the callers of git_parse_signed() to provide the newly required minimum value. It calculates the minimum for a given type using division instead of right shift because the latter is implementation-defined for signed types, and we need an arithmetic right shift here, not a logical one. Original-patch-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: René Scharfe <l.s.r@web.de> --- git_config_get_expiry_in_days() could use git_parse_int(), but that's a different topic. config.c | 28 +++++++++++++++++----------- git-compat-util.h | 2 ++ 2 files changed, 19 insertions(+), 11 deletions(-) diff --git a/config.c b/config.c index cbb5a3bab7..7cf196dc84 100644 --- a/config.c +++ b/config.c @@ -1155,26 +1155,24 @@ static uintmax_t get_unit_factor(const char *end) return 0; } -static int git_parse_signed(const char *value, intmax_t *ret, intmax_t max) +static int git_parse_signed(const char *value, intmax_t *ret, + intmax_t min, intmax_t max) { if (value && *value) { char *end; intmax_t val; - uintmax_t uval; - uintmax_t factor; + intmax_t factor; errno = 0; val = strtoimax(value, &end, 0); if (errno == ERANGE) return 0; factor = get_unit_factor(end); - if (!factor) { + if (factor < 1) { errno = EINVAL; return 0; } - uval = val < 0 ? -val : val; - if (unsigned_mult_overflows(factor, uval) || - factor * uval > max) { + if (val < min / factor || val > max / factor) { errno = ERANGE; return 0; } @@ -1218,7 +1216,9 @@ static int git_parse_unsigned(const char *value, uintmax_t *ret, uintmax_t max) static int git_parse_int(const char *value, int *ret) { intmax_t tmp; - if (!git_parse_signed(value, &tmp, maximum_signed_value_of_type(int))) + if (!git_parse_signed(value, &tmp, + minimum_signed_value_of_type(int), + maximum_signed_value_of_type(int))) return 0; *ret = tmp; return 1; @@ -1227,7 +1227,9 @@ static int git_parse_int(const char *value, int *ret) static int git_parse_int64(const char *value, int64_t *ret) { intmax_t tmp; - if (!git_parse_signed(value, &tmp, maximum_signed_value_of_type(int64_t))) + if (!git_parse_signed(value, &tmp, + minimum_signed_value_of_type(int64_t), + maximum_signed_value_of_type(int64_t))) return 0; *ret = tmp; return 1; @@ -1245,7 +1247,9 @@ int git_parse_ulong(const char *value, unsigned long *ret) int git_parse_ssize_t(const char *value, ssize_t *ret) { intmax_t tmp; - if (!git_parse_signed(value, &tmp, maximum_signed_value_of_type(ssize_t))) + if (!git_parse_signed(value, &tmp, + minimum_signed_value_of_type(ssize_t), + maximum_signed_value_of_type(ssize_t))) return 0; *ret = tmp; return 1; @@ -2751,7 +2755,9 @@ int git_config_get_expiry_in_days(const char *key, timestamp_t *expiry, timestam if (git_config_get_string_tmp(key, &expiry_string)) return 1; /* no such thing */ - if (git_parse_signed(expiry_string, &days, maximum_signed_value_of_type(int))) { + if (git_parse_signed(expiry_string, &days, + minimum_signed_value_of_type(int), + maximum_signed_value_of_type(int))) { const int scale = 86400; *expiry = now - days * scale; return 0; diff --git a/git-compat-util.h b/git-compat-util.h index ea53ea4a78..35425c373b 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -112,6 +112,8 @@ struct strbuf; #define bitsizeof(x) (CHAR_BIT * sizeof(x)) +#define minimum_signed_value_of_type(a) \ + (INTMAX_MIN / ((intmax_t)1 << (bitsizeof(intmax_t) - bitsizeof(a)))) #define maximum_signed_value_of_type(a) \ (INTMAX_MAX >> (bitsizeof(intmax_t) - bitsizeof(a))) -- 2.38.1 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH 3/3] git_parse_signed(): avoid integer overflow 2022-10-22 8:09 ` René Scharfe @ 2022-10-22 16:51 ` Junio C Hamano 2022-10-23 5:57 ` René Scharfe 0 siblings, 1 reply; 27+ messages in thread From: Junio C Hamano @ 2022-10-22 16:51 UTC (permalink / raw) To: René Scharfe; +Cc: Phillip Wood via GitGitGadget, git, Phillip Wood René Scharfe <l.s.r@web.de> writes: >>> + if (max < 0) >>> + BUG("max must be a positive integer"); >> >> In parse_signed(), would we expect to accept end-user input that is >> a negative integer? We must. Otherwise we would not be calling a >> "signed" parser. Now, are there cases where the valid value range >> is bounded by a negative integer at the top? No current callers may >> pass such a value, but is it reasonable to add such a new constraints >> to an existing API function? > > Hmm, if minimum and maximum are not symmetric, then we need to supply > both, don't we? Ah, thanks for injecting doze of sanity---I totally missed that the bound was about the absolute value, so we can say "this is signed, and the allowed values are (-3, -2, -1, 0, 1, 2, 3). If so, then the "reject negative max" in the posted patch is not a problem as I said above. I somehow thought that giving -1 as "max" would allow callers to say "non-negative numbers are not allowed". But that is not what is going on. Allowing callers to specify both lower and uppoer bounds so that they can say "the allowed values are (-1, 0, 1, 2, 3)", while it might make it more useful, is a separate new feature development and outside the scope of "let's tighten the parsing of end user input" Phillip has here. Sorry about the thinko, and thanks for a new and interesting tangent. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 3/3] git_parse_signed(): avoid integer overflow 2022-10-22 16:51 ` Junio C Hamano @ 2022-10-23 5:57 ` René Scharfe 2022-10-25 10:00 ` Phillip Wood 0 siblings, 1 reply; 27+ messages in thread From: René Scharfe @ 2022-10-23 5:57 UTC (permalink / raw) To: Junio C Hamano; +Cc: Phillip Wood via GitGitGadget, git, Phillip Wood Am 22.10.22 um 18:51 schrieb Junio C Hamano: > René Scharfe <l.s.r@web.de> writes: > >>>> + if (max < 0) >>>> + BUG("max must be a positive integer"); >>> >>> In parse_signed(), would we expect to accept end-user input that is >>> a negative integer? We must. Otherwise we would not be calling a >>> "signed" parser. Now, are there cases where the valid value range >>> is bounded by a negative integer at the top? No current callers may >>> pass such a value, but is it reasonable to add such a new constraints >>> to an existing API function? >> >> Hmm, if minimum and maximum are not symmetric, then we need to supply >> both, don't we? > > Ah, thanks for injecting doze of sanity---I totally missed that the > bound was about the absolute value, so we can say "this is signed, > and the allowed values are (-3, -2, -1, 0, 1, 2, 3). If so, then the > "reject negative max" in the posted patch is not a problem as I said > above. I somehow thought that giving -1 as "max" would allow callers > to say "non-negative numbers are not allowed". But that is not what > is going on. Right, currently the value of `max` is used to check the absolute value, i.e. it imposes a limit in both the positive and negative direction. > Allowing callers to specify both lower and uppoer bounds so that > they can say "the allowed values are (-1, 0, 1, 2, 3)", while it > might make it more useful, is a separate new feature development and > outside the scope of "let's tighten the parsing of end user input" > Phillip has here. Allowing arbitrary limits in both directions might be a new feature, but it's required if we want to support the full range of values. E.g. on my system INT_MAX is 2147483647 and INT_MIN is -2147483647-1. Currently git_parse_int() rejects INT_MIN as out of range. In my eyes the assumption that a single limit can be used to check both directions of the signed number line is flawed and caused the undefined behavior. Dropping it avoids tricky calculations that try to infer the lower limit somehow and opens the full range of values to us. That said, I'm not sure how useful the values INT_MIN, INT64_MIN and SSIZE_MIN (which unlike SSIZE_MAX is not defined by POSIX [*]) actually are. But doing the checks properly requires separate min and max values. René [*] Perhaps git_parse_ssize_t() should reject values less than -1; only that single negative number is needed to represent errors or unlimited. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 3/3] git_parse_signed(): avoid integer overflow 2022-10-23 5:57 ` René Scharfe @ 2022-10-25 10:00 ` Phillip Wood 2022-10-26 11:01 ` René Scharfe 0 siblings, 1 reply; 27+ messages in thread From: Phillip Wood @ 2022-10-25 10:00 UTC (permalink / raw) To: René Scharfe, Junio C Hamano; +Cc: Phillip Wood via GitGitGadget, git On 23/10/2022 06:57, René Scharfe wrote: > Am 22.10.22 um 18:51 schrieb Junio C Hamano: >> René Scharfe <l.s.r@web.de> writes: >> >>>>> + if (max < 0) >>>>> + BUG("max must be a positive integer"); >>>> >>>> In parse_signed(), would we expect to accept end-user input that is >>>> a negative integer? We must. Otherwise we would not be calling a >>>> "signed" parser. Now, are there cases where the valid value range >>>> is bounded by a negative integer at the top? No current callers may >>>> pass such a value, but is it reasonable to add such a new constraints >>>> to an existing API function? >>> >>> Hmm, if minimum and maximum are not symmetric, then we need to supply >>> both, don't we? >> >> Ah, thanks for injecting doze of sanity---I totally missed that the >> bound was about the absolute value, so we can say "this is signed, >> and the allowed values are (-3, -2, -1, 0, 1, 2, 3). If so, then the >> "reject negative max" in the posted patch is not a problem as I said >> above. I somehow thought that giving -1 as "max" would allow callers >> to say "non-negative numbers are not allowed". But that is not what >> is going on. > > Right, currently the value of `max` is used to check the absolute value, > i.e. it imposes a limit in both the positive and negative direction. > >> Allowing callers to specify both lower and uppoer bounds so that >> they can say "the allowed values are (-1, 0, 1, 2, 3)", while it >> might make it more useful, is a separate new feature development and >> outside the scope of "let's tighten the parsing of end user input" >> Phillip has here. > > Allowing arbitrary limits in both directions might be a new feature, but > it's required if we want to support the full range of values. E.g. on > my system INT_MAX is 2147483647 and INT_MIN is -2147483647-1. Currently > git_parse_int() rejects INT_MIN as out of range. > > In my eyes the assumption that a single limit can be used to check both > directions of the signed number line is flawed and caused the undefined > behavior. Dropping it avoids tricky calculations that try to infer the > lower limit somehow and opens the full range of values to us. > > That said, I'm not sure how useful the values INT_MIN, INT64_MIN and > SSIZE_MIN (which unlike SSIZE_MAX is not defined by POSIX [*]) actually > are. But doing the checks properly requires separate min and max values. I'm happy to go either way, while I agree passing separate limits to allow INT_MIN is technically correct I'm not sure anyone has complained that the current code is too restrictive. Best Wishes Phillip > René > > > [*] Perhaps git_parse_ssize_t() should reject values less than -1; only > that single negative number is needed to represent errors or unlimited. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 3/3] git_parse_signed(): avoid integer overflow 2022-10-25 10:00 ` Phillip Wood @ 2022-10-26 11:01 ` René Scharfe 0 siblings, 0 replies; 27+ messages in thread From: René Scharfe @ 2022-10-26 11:01 UTC (permalink / raw) To: phillip.wood, Junio C Hamano; +Cc: Phillip Wood via GitGitGadget, git Am 25.10.22 um 12:00 schrieb Phillip Wood: > On 23/10/2022 06:57, René Scharfe wrote: >> >> That said, I'm not sure how useful the values INT_MIN, INT64_MIN >> and SSIZE_MIN (which unlike SSIZE_MAX is not defined by POSIX [*]) >> actually are. But doing the checks properly requires separate min >> and max values. > > I'm happy to go either way, while I agree passing separate limits to > allow INT_MIN is technically correct I'm not sure anyone has > complained that the current code is too restrictive. Right. Having separate patches for the different aspects (your fix, simplification due to adding a min parameter, range extension made possible by that) is probably best to discuss the merits of each. René ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 0/3] a few config integer parsing fixes 2022-10-21 13:45 [PATCH 0/3] a few config integer parsing fixes Phillip Wood via GitGitGadget ` (2 preceding siblings ...) 2022-10-21 13:45 ` [PATCH 3/3] git_parse_signed(): avoid integer overflow Phillip Wood via GitGitGadget @ 2022-11-09 14:16 ` Phillip Wood via GitGitGadget 2022-11-09 14:16 ` [PATCH v2 1/3] git_parse_unsigned: reject negative values Phillip Wood via GitGitGadget ` (3 more replies) 3 siblings, 4 replies; 27+ messages in thread From: Phillip Wood via GitGitGadget @ 2022-11-09 14:16 UTC (permalink / raw) To: git; +Cc: René Scharfe, Jeff King, Phillip Wood 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 1/3] git_parse_unsigned: reject negative values 2022-11-09 14:16 ` [PATCH v2 0/3] a few config integer parsing fixes Phillip Wood via GitGitGadget @ 2022-11-09 14:16 ` 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 ` (2 subsequent siblings) 3 siblings, 1 reply; 27+ messages in thread From: Phillip Wood via GitGitGadget @ 2022-11-09 14:16 UTC (permalink / raw) To: git; +Cc: René Scharfe, Jeff King, Phillip Wood, Phillip Wood 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) 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 -- gitgitgadget ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v2 1/3] git_parse_unsigned: reject negative values 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 0 siblings, 0 replies; 27+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-11-09 15:57 UTC (permalink / raw) To: Phillip Wood via GitGitGadget Cc: git, René Scharfe, Jeff King, Phillip Wood 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 = ""; ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 2/3] config: require at least one digit when parsing numbers 2022-11-09 14:16 ` [PATCH v2 0/3] a few config integer parsing fixes Phillip Wood via GitGitGadget 2022-11-09 14:16 ` [PATCH v2 1/3] git_parse_unsigned: reject negative values Phillip Wood via GitGitGadget @ 2022-11-09 14:16 ` 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 3 siblings, 0 replies; 27+ messages in thread From: Phillip Wood via GitGitGadget @ 2022-11-09 14:16 UTC (permalink / raw) To: git; +Cc: René Scharfe, Jeff King, Phillip Wood, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> 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 <peff@peff.net> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- 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 d5069d4f01d..b7fb68026d8 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 64d2327b744..7d7ecfd5716 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 c6661e61af5..2575279ab84 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 -- gitgitgadget ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 3/3] git_parse_signed(): avoid integer overflow 2022-11-09 14:16 ` [PATCH v2 0/3] a few config integer parsing fixes Phillip Wood via GitGitGadget 2022-11-09 14:16 ` [PATCH v2 1/3] git_parse_unsigned: reject negative values Phillip Wood via GitGitGadget 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 ` Phillip Wood via GitGitGadget 2022-11-10 2:35 ` [PATCH v2 0/3] a few config integer parsing fixes Taylor Blau 3 siblings, 0 replies; 27+ messages in thread From: Phillip Wood via GitGitGadget @ 2022-11-09 14:16 UTC (permalink / raw) To: git; +Cc: René Scharfe, Jeff King, Phillip Wood, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> 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 <phillip.wood@dunelm.org.uk> --- config.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/config.c b/config.c index b7fb68026d8..aad3e00341d 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; } -- gitgitgadget ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v2 0/3] a few config integer parsing fixes 2022-11-09 14:16 ` [PATCH v2 0/3] a few config integer parsing fixes Phillip Wood via GitGitGadget ` (2 preceding siblings ...) 2022-11-09 14:16 ` [PATCH v2 3/3] git_parse_signed(): avoid integer overflow Phillip Wood via GitGitGadget @ 2022-11-10 2:35 ` Taylor Blau 3 siblings, 0 replies; 27+ messages in thread From: Taylor Blau @ 2022-11-10 2:35 UTC (permalink / raw) To: Phillip Wood via GitGitGadget Cc: git, René Scharfe, Jeff King, Phillip Wood On Wed, Nov 09, 2022 at 02:16:25PM +0000, Phillip Wood via GitGitGadget wrote: > 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. Having read this round and the discussion on the original version, I think that this is ready to start merging down. That said, it would be nice to hear from some other reviewer(s) who took a look or are interested in this area. Thanks, Taylor ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2022-11-10 2:35 UTC | newest] Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 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 ` [PATCH v2 0/3] a few config integer parsing fixes Phillip Wood via GitGitGadget 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
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).