Re: [PATCH] config.c: avoid segfault with --fixed-value and valueless config
From: Patrick Steinhardt <hidden>
Date: 2024-08-05 12:39:00
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?
quoted hunk ↗ jump to hunk
[1]: https://lore.kernel.org/git/CANrWfmTek1xErBLrnoyhHN+gWU+rw14y6SQ+abZyzGoaBjmiKA@mail.gmail.com/ (local) Reported-by: Han Jiang <redacted> Signed-off-by: Taylor Blau <redacted> --- 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
Attachments
- signature.asc [application/pgp-signature] 833 bytes