[PATCH] config.c: avoid segfault with --fixed-value and valueless config

[PATCH] config.c: avoid segfault with --fixed-value and valueless config

[Taylor Blau]

When using `--fixed-value` with a key whose value is left empty (implied
as being "true"), 'git config' may crash when invoked like either of:

    $ git config set --file=config --value=value --fixed-value \
        section.key pattern
    $ git config --file=config --fixed-value section.key value pattern

The original bugreport[1] bisects to 00bbdde141 (builtin/config:
introduce "set" subcommand, 2024-05-06), which is a red-herring, since
the original bugreport uses the new 'git config set' invocation.

The behavior likely bisects back to c90702a1f6 (config: plumb
--fixed-value into config API, 2020-11-25), which introduces the new
--fixed-value option in the first place.

Looking at the relevant frame from a failed process's coredump, the
crash appears in config.c::matches() like so:

    (gdb) up
    #1  0x000055b3e8b06022 in matches (key=0x55b3ea894360 "section.key", value=0x0,
        store=0x7ffe99076eb0) at config.c:2884
    2884			return !strcmp(store->fixed_value, value);

where we are trying to compare the `--fixed-value` argument to `value`,
which is NULL.

Avoid attempting to match `--fixed-value` for configuration keys with no
explicit value. A future patch could consider the empty value to mean
"true", "yes", "on", etc. when invoked with `--type=bool`, but let's
punt on that for now in the name of avoiding the segfault.

[1]: https://lore.kernel.org/git/CANrWfmTek1xErBLrnoyhHN+gWU+rw14y6SQ+abZyzGoaBjmiKA@mail.gmail.com/

Reported-by: Han Jiang <jhcarl0814@gmail.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 config.c          | 2 +-
 t/t1300-config.sh | 9 +++++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/config.c b/config.c
index 6421894614..05f369ec0d 100644
--- a/config.c
+++ b/config.c
@@ -2914,7 +2914,7 @@ static int matches(const char *key, const char *value,
 {
 	if (strcmp(key, store->key))
 		return 0; /* not ours */
-	if (store->fixed_value)
+	if (store->fixed_value && value)
 		return !strcmp(store->fixed_value, value);
 	if (!store->value_pattern)
 		return 1; /* always matches */
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 9de2d95f06..f13277c8f3 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -2704,6 +2704,15 @@ test_expect_success '--get and --get-all with --fixed-value' '
 	test_must_fail git config --file=config --get-regexp --fixed-value fixed+ non-existent
 '
 
+test_expect_success '--fixed-value with value-less configuration' '
+	test_when_finished rm -f config &&
+	cat >config <<-\EOF &&
+	[section]
+		key
+	EOF
+	git config --file=config --fixed-value section.key value pattern
+'
+
 test_expect_success 'includeIf.hasconfig:remote.*.url' '
 	git init hasremoteurlTest &&
 	test_when_finished "rm -rf hasremoteurlTest" &&
-- 
2.46.0.dirty

Re: [PATCH] config.c: avoid segfault with --fixed-value and valueless config

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3052 bytes --]

On Thu, Aug 01, 2024 at 01:06:54PM -0400, Taylor Blau wrote:
> When using `--fixed-value` with a key whose value is left empty (implied
> as being "true"), 'git config' may crash when invoked like either of:
> 
>     $ git config set --file=config --value=value --fixed-value \
>         section.key pattern
>     $ git config --file=config --fixed-value section.key value pattern
> 
> The original bugreport[1] bisects to 00bbdde141 (builtin/config:
> introduce "set" subcommand, 2024-05-06), which is a red-herring, since
> the original bugreport uses the new 'git config set' invocation.
> 
> The behavior likely bisects back to c90702a1f6 (config: plumb
> --fixed-value into config API, 2020-11-25), which introduces the new
> --fixed-value option in the first place.
> 
> Looking at the relevant frame from a failed process's coredump, the
> crash appears in config.c::matches() like so:
> 
>     (gdb) up
>     #1  0x000055b3e8b06022 in matches (key=0x55b3ea894360 "section.key", value=0x0,
>         store=0x7ffe99076eb0) at config.c:2884
>     2884			return !strcmp(store->fixed_value, value);
> 
> where we are trying to compare the `--fixed-value` argument to `value`,
> which is NULL.
> 
> Avoid attempting to match `--fixed-value` for configuration keys with no
> explicit value. A future patch could consider the empty value to mean
> "true", "yes", "on", etc. when invoked with `--type=bool`, but let's
> punt on that for now in the name of avoiding the segfault.

Edge cases like this really make me wonder what the benefit of implicit
bools is in our config files. They have been a source of bugs, including
this one, and in my opinion only lead to confusion when reading through
a config file manually. I would claim that 99% of our users out there
don't even know that you can have implicit booleans, and would think
that the config is invalid. And the 1% that do know probably don't care
much. It's not even like it would safe you a ton of typing.

So... why do we have them in the first place? Is there even a single
good reason?

> [1]: https://lore.kernel.org/git/CANrWfmTek1xErBLrnoyhHN+gWU+rw14y6SQ+abZyzGoaBjmiKA@mail.gmail.com/
> 
> Reported-by: Han Jiang <jhcarl0814@gmail.com>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  config.c          | 2 +-
>  t/t1300-config.sh | 9 +++++++++
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/config.c b/config.c
> index 6421894614..05f369ec0d 100644
> --- a/config.c
> +++ b/config.c
> @@ -2914,7 +2914,7 @@ static int matches(const char *key, const char *value,
>  {
>  	if (strcmp(key, store->key))
>  		return 0; /* not ours */
> -	if (store->fixed_value)
> +	if (store->fixed_value && value)
>  		return !strcmp(store->fixed_value, value);

Okay, makes sense. I think we should at least have a comment here saying
that we simply ignore keys with implicit values. I was also wondering
whether we want to warn about those such that users are aware in case we
ignore them?

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] config.c: avoid segfault with --fixed-value and valueless config

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> Edge cases like this really make me wonder what the benefit of implicit
> bools is in our config files.
>
> So... why do we have them in the first place? Is there even a single
> good reason?

There isn't any good reason to introduce such a syntax if we were
desigining the configuration file format from scratch.  It was added
because originally Linus thought it was a cute syntax, and these
days a lot lot more importantly, it is kept working because you will
break a lot of existing configuration files that were hand tweaked
if you remove the support suddenly.

Re: [PATCH] config.c: avoid segfault with --fixed-value and valueless config

[Taylor Blau]

On Mon, Aug 05, 2024 at 08:45:32AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > Edge cases like this really make me wonder what the benefit of implicit
> > bools is in our config files.
> >
> > So... why do we have them in the first place? Is there even a single
> > good reason?
>
> There isn't any good reason to introduce such a syntax if we were
> desigining the configuration file format from scratch.  It was added
> because originally Linus thought it was a cute syntax, and these
> days a lot lot more importantly, it is kept working because you will
> break a lot of existing configuration files that were hand tweaked
> if you remove the support suddenly.

I agree. It's perhaps interesting to think about in the context of the
discussion in [1], but I think also worth having some perspective above.

Sure, this configuration syntax would not be invented anew today, but I
also don't think it's worth breaking existing configurations, even in a
hypothetical "Git 3.0" release.

In some sense I am sympathetic to Patrick's argument, but I also think
that having a bug in a relatively niche feature like --fixed-value that
wasn't noticed for almost four years over 17 [2] releases isn't itself a
strong argument for removing the feature.

Thanks,
Taylor

[1]: <fc1a9fa03de7330f79dc56b0f2712834cb236b5a.1715070296.git.ps@pks.im>
[2]: $ git tag --contains c90702a1f6 'v2.*.0' | wc -l

Re: [PATCH] config.c: avoid segfault with --fixed-value and valueless config

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1698 bytes --]

On Mon, Aug 05, 2024 at 03:46:42PM -0400, Taylor Blau wrote:
> On Mon, Aug 05, 2024 at 08:45:32AM -0700, Junio C Hamano wrote:
> > Patrick Steinhardt <ps@pks.im> writes:
> >
> > > Edge cases like this really make me wonder what the benefit of implicit
> > > bools is in our config files.
> > >
> > > So... why do we have them in the first place? Is there even a single
> > > good reason?
> >
> > There isn't any good reason to introduce such a syntax if we were
> > desigining the configuration file format from scratch.  It was added
> > because originally Linus thought it was a cute syntax, and these
> > days a lot lot more importantly, it is kept working because you will
> > break a lot of existing configuration files that were hand tweaked
> > if you remove the support suddenly.
> 
> I agree. It's perhaps interesting to think about in the context of the
> discussion in [1], but I think also worth having some perspective above.
> 
> Sure, this configuration syntax would not be invented anew today, but I
> also don't think it's worth breaking existing configurations, even in a
> hypothetical "Git 3.0" release.
> 
> In some sense I am sympathetic to Patrick's argument, but I also think
> that having a bug in a relatively niche feature like --fixed-value that
> wasn't noticed for almost four years over 17 [2] releases isn't itself a
> strong argument for removing the feature.

I was really just wondering whether there is actually a good reason to
have it that I couldn't think of. I certainly think that this feature
shouldn't exist, but also agree that removing it now would create more
hassle than benefit.

Thanks for the context!

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]