[PATCH] pretty.c: make git_pretty_formats_config return -1 on git_config_string failure

[PATCH] pretty.c: make git_pretty_formats_config return -1 on git_config_string failure

[Tanay Abhra]

`git_pretty_formats_config()` continues without checking git_config_string's
return value which can lead to a SEGFAULT. Instead return -1 when
git_config_string fails signalling `git_config()` to die printing the location
of the erroneous variable.

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 pretty.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/pretty.c b/pretty.c
index 3a1da6f..72dbf55 100644
--- a/pretty.c
+++ b/pretty.c
@@ -65,7 +65,9 @@ static int git_pretty_formats_config(const char *var, const char *value, void *c
 
 	commit_format->name = xstrdup(name);
 	commit_format->format = CMIT_FMT_USERFORMAT;
-	git_config_string(&fmt, var, value);
+	if (git_config_string(&fmt, var, value))
+		return -1;
+
 	if (starts_with(fmt, "format:") || starts_with(fmt, "tformat:")) {
 		commit_format->is_tformat = fmt[0] == 't';
 		fmt = strchr(fmt, ':') + 1;
-- 
1.9.0.GIT

Re: [PATCH] pretty.c: make git_pretty_formats_config return -1 on git_config_string failure

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> `git_pretty_formats_config()` continues without checking git_config_string's
> return value which can lead to a SEGFAULT.

Indeed, without the patch:

$ git -c pretty.my= log --pretty=my                        
error: Missing value for 'pretty.my'                         
zsh: segmentation fault  git -c pretty.my= log --pretty=my

> diff --git a/pretty.c b/pretty.c
> index 3a1da6f..72dbf55 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -65,7 +65,9 @@ static int git_pretty_formats_config(const char *var, const char *value, void *c
>  
>  	commit_format->name = xstrdup(name);
>  	commit_format->format = CMIT_FMT_USERFORMAT;
> -	git_config_string(&fmt, var, value);
> +	if (git_config_string(&fmt, var, value))
> +		return -1;
> +

Ack-ed-by: Matthieu Moy <Matthieu.Moy@imag.fr>

My first thought reading this was "why not rewrite using non-callback
API?", but this particular call to git_config needs to iterate over
config keys anyway.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH] pretty.c: make git_pretty_formats_config return -1 on git_config_string failure

[Eric Sunshine]

On Mon, Aug 4, 2014 at 11:45 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Tanay Abhra <tanayabh@gmail.com> writes:
>
>> `git_pretty_formats_config()` continues without checking git_config_string's
>> return value which can lead to a SEGFAULT.
>
> Indeed, without the patch:
>
> $ git -c pretty.my= log --pretty=my
> error: Missing value for 'pretty.my'
> zsh: segmentation fault  git -c pretty.my= log --pretty=my

This probably should be formalized as a proper test and included with
Tanay's patch.

>> diff --git a/pretty.c b/pretty.c
>> index 3a1da6f..72dbf55 100644
>> --- a/pretty.c
>> +++ b/pretty.c
>> @@ -65,7 +65,9 @@ static int git_pretty_formats_config(const char *var, const char *value, void *c
>>
>>       commit_format->name = xstrdup(name);
>>       commit_format->format = CMIT_FMT_USERFORMAT;
>> -     git_config_string(&fmt, var, value);
>> +     if (git_config_string(&fmt, var, value))
>> +             return -1;
>> +
>
> Ack-ed-by: Matthieu Moy <Matthieu.Moy@imag.fr>
>
> My first thought reading this was "why not rewrite using non-callback
> API?", but this particular call to git_config needs to iterate over
> config keys anyway.
>
> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/

Re: [PATCH] pretty.c: make git_pretty_formats_config return -1 on git_config_string failure

[Matthieu Moy]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Mon, Aug 4, 2014 at 11:45 AM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Tanay Abhra <tanayabh@gmail.com> writes:
>>
>>> `git_pretty_formats_config()` continues without checking git_config_string's
>>> return value which can lead to a SEGFAULT.
>>
>> Indeed, without the patch:
>>
>> $ git -c pretty.my= log --pretty=my
>> error: Missing value for 'pretty.my'
>> zsh: segmentation fault  git -c pretty.my= log --pretty=my
>
> This probably should be formalized as a proper test and included with
> Tanay's patch.

Not sure it's worth the trouble: the bug corresponds to a
mis-application of a pattern used in tens of places in Git's code
(basically, each call to git_config_string, 50 callsites). Testing this
particular case does not ensure non-regression, and testing all
occurences of the pattern would be overkill IMHO.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH] pretty.c: make git_pretty_formats_config return -1 on git_config_string failure

[Jeff King]

On Mon, Aug 04, 2014 at 05:45:44PM +0200, Matthieu Moy wrote:

> Tanay Abhra <tanayabh@gmail.com> writes:
> 
> > `git_pretty_formats_config()` continues without checking git_config_string's
> > return value which can lead to a SEGFAULT.
> 
> Indeed, without the patch:
> 
> $ git -c pretty.my= log --pretty=my                        
> error: Missing value for 'pretty.my'                         
> zsh: segmentation fault  git -c pretty.my= log --pretty=my

Hmm. Not related to the original patch, but that really looks like a
bug. Shouldn't "git -c pretty.my= ..." set pretty.my to the empty string?

I'd expect "git -c pretty.my ..." to set it to NULL (i.e., the "implicit
true" you get from omitting the "=" in the config files themselves).

-Peff

Re: [PATCH] pretty.c: make git_pretty_formats_config return -1 on git_config_string failure

[Matthieu Moy]

Jeff King <peff@peff.net> writes:

> On Mon, Aug 04, 2014 at 05:45:44PM +0200, Matthieu Moy wrote:
>
>> Tanay Abhra <tanayabh@gmail.com> writes:
>> 
>> > `git_pretty_formats_config()` continues without checking git_config_string's
>> > return value which can lead to a SEGFAULT.
>> 
>> Indeed, without the patch:
>> 
>> $ git -c pretty.my= log --pretty=my                        
>> error: Missing value for 'pretty.my'                         
>> zsh: segmentation fault  git -c pretty.my= log --pretty=my
>
> Hmm. Not related to the original patch, but that really looks like a
> bug. Shouldn't "git -c pretty.my= ..." set pretty.my to the empty string?
>
> I'd expect "git -c pretty.my ..." to set it to NULL (i.e., the "implicit
> true" you get from omitting the "=" in the config files themselves).

Indeed.

strbuf_split_buf() does not seem to distinguish between x= and x. No
time to debug this further, sorry.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

[PATCH] config: teach "git -c" to recognize an empty string

[Jeff King]

On Mon, Aug 04, 2014 at 11:06:03PM +0200, Matthieu Moy wrote:

> > Hmm. Not related to the original patch, but that really looks like a
> > bug. Shouldn't "git -c pretty.my= ..." set pretty.my to the empty string?
> >
> > I'd expect "git -c pretty.my ..." to set it to NULL (i.e., the "implicit
> > true" you get from omitting the "=" in the config files themselves).
> 
> Indeed.
> 
> strbuf_split_buf() does not seem to distinguish between x= and x. No
> time to debug this further, sorry.

Oh, I didn't expect you to work on it. The bug is totally my fault. :)
Your email just made me realize it was there.

Here's a patch to fix it.

-- >8 --
Subject: config: teach "git -c" to recognize an empty string

In a config file, you can do:

  [foo]
  bar

to turn the "foo.bar" boolean flag on, and you can do:

  [foo]
  bar=

to set "foo.bar" to the empty string. However, git's "-c"
parameter treats both:

  git -c foo.bar

and

  git -c foo.bar=

as the boolean flag, and there is no way to set a variable
to the empty string. This patch enables the latter form to
do that.

Signed-off-by: Jeff King <peff@peff.net>
---
This is technically a backwards incompatibility, but I'd consider it a
simple bugfix. The existing behavior was unintentional, made no sense,
and was never documented.

Looking over strbuf_split's interface, I think it's rather
counter-intuitive, and I was tempted to change it. But there are several
other callers that rely on it, and the chance for introducing a subtle
bug is high. This is the least invasive fix (and it really is not any
less readable than what was already there :) ).

 Documentation/git.txt  |  5 +++++
 config.c               | 12 ++++++++++--
 t/t1300-repo-config.sh | 11 +++++++++++
 3 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index b1c4f7a..e7783f0 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -447,6 +447,11 @@ example the following invocations are equivalent:
 	given will override values from configuration files.
 	The <name> is expected in the same format as listed by
 	'git config' (subkeys separated by dots).
++
+Note that omitting the `=` in `git -c foo.bar ...` is allowed and sets
+`foo.bar` to the boolean true value (just like `[foo]bar` would in a
+config file). Including the equals but with an empty value (like `git -c
+foo.bar= ...`) sets `foo.bar` to the empty string.
 
 --exec-path[=<path>]::
 	Path to wherever your core Git programs are installed.
diff --git a/config.c b/config.c
index 058505c..fe6216f 100644
--- a/config.c
+++ b/config.c
@@ -162,19 +162,27 @@ void git_config_push_parameter(const char *text)
 int git_config_parse_parameter(const char *text,
 			       config_fn_t fn, void *data)
 {
+	const char *value;
 	struct strbuf **pair;
+
 	pair = strbuf_split_str(text, '=', 2);
 	if (!pair[0])
 		return error("bogus config parameter: %s", text);
-	if (pair[0]->len && pair[0]->buf[pair[0]->len - 1] == '=')
+
+	if (pair[0]->len && pair[0]->buf[pair[0]->len - 1] == '=') {
 		strbuf_setlen(pair[0], pair[0]->len - 1);
+		value = pair[1] ? pair[1]->buf : "";
+	} else
+		value = NULL;
+
 	strbuf_trim(pair[0]);
 	if (!pair[0]->len) {
 		strbuf_list_free(pair);
 		return error("bogus config parameter: %s", text);
 	}
+
 	strbuf_tolower(pair[0]);
-	if (fn(pair[0]->buf, pair[1] ? pair[1]->buf : NULL, data) < 0) {
+	if (fn(pair[0]->buf, value, data) < 0) {
 		strbuf_list_free(pair);
 		return -1;
 	}
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 3f80ff0..46f6ae2 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1010,6 +1010,17 @@ test_expect_success 'git -c "key=value" support' '
 	test_must_fail git -c name=value config core.name
 '
 
+# We just need a type-specifier here that cares about the
+# distinction internally between a NULL boolean and a real
+# string (because most of git's internal parsers do care).
+# Using "--path" works, but we do not otherwise care about
+# its semantics.
+test_expect_success 'git -c can represent empty string' '
+	echo >expect &&
+	git -c foo.empty= config --path foo.empty >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'key sanity-checking' '
 	test_must_fail git config foo=bar &&
 	test_must_fail git config foo=.bar &&
-- 
2.1.0.rc0.286.g5c67d74

Re: [PATCH] config: teach "git -c" to recognize an empty string

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> This is technically a backwards incompatibility, but I'd consider it a
> simple bugfix. The existing behavior was unintentional, made no sense,
> and was never documented.

Yeah, I tend to agree.  I actually would not shed any tears if the
breakage were that it was impossible to pass "NULL is true" boolean
via "git -c" interface, but it is the other way around.  It is much
more grave a problem that we cannot pass an empty string as a value,
and we should fix it.

> Looking over strbuf_split's interface, I think it's rather
> counter-intuitive, and I was tempted to change it. But there are several
> other callers that rely on it, and the chance for introducing a subtle
> bug is high. This is the least invasive fix (and it really is not any
> less readable than what was already there :) ).

;-)

> +# We just need a type-specifier here that cares about the
> +# distinction internally between a NULL boolean and a real
> +# string (because most of git's internal parsers do care).
> +# Using "--path" works, but we do not otherwise care about
> +# its semantics.
> +test_expect_success 'git -c can represent empty string' '
> +	echo >expect &&
> +	git -c foo.empty= config --path foo.empty >actual &&
> +	test_cmp expect actual
> +'

Another way may be "git config -l" and see if we see a = on the
entry for foo.empty, but I think the way you did this is nicer.

>  test_expect_success 'key sanity-checking' '
>  	test_must_fail git config foo=bar &&
>  	test_must_fail git config foo=.bar &&