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

[Patrick Steinhardt <ps@pks.im>]

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