[2/3] config: require at least one digit when parsing numbers

Commit Message

Phillip Wood Oct. 21, 2022, 1:45 p.m. UTC

From: Phillip Wood <[e-mail]>

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 <[e-mail]>
---
 config.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Junio C Hamano Oct. 21, 2022, 6:19 p.m. UTC | #1

"Phillip Wood via GitGitGadget" <[e-mail]> writes:

> From: Phillip Wood <[e-mail]>
>
> 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 <[e-mail]>
> ---
>  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;

Jeff King Oct. 21, 2022, 8:17 p.m. UTC | #2

On Fri, Oct 21, 2022 at 01:45:13PM +0000, Phillip Wood via GitGitGadget wrote:

> From: Phillip Wood <[e-mail]>
> 
> 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

Junio C Hamano Oct. 22, 2022, 5:51 p.m. UTC | #3

Jeff King <[e-mail]> writes:

> On Fri, Oct 21, 2022 at 01:45:13PM +0000, Phillip Wood via GitGitGadget wrote:
>
>> From: Phillip Wood <[e-mail]>
>> 
>> 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.

Jeff King Oct. 22, 2022, 8:25 p.m. UTC | #4

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

Junio C Hamano Oct. 22, 2022, 9 p.m. UTC | #5

Jeff King <[e-mail]> 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.

Phillip Wood Oct. 25, 2022, 9:54 a.m. UTC | #6

Hi Junio

On 21/10/2022 19:19, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <[e-mail]> 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

Phillip Wood Oct. 25, 2022, 9:55 a.m. UTC | #7

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 <[e-mail]>
>>
>> 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

Junio C Hamano Oct. 25, 2022, 4:08 p.m. UTC | #8

Phillip Wood <[e-mail]> writes:

> On 21/10/2022 19:19, Junio C Hamano wrote:
>> "Phillip Wood via GitGitGadget" <[e-mail]> 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.

Patch

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;