| Message ID | c78bacfa8fb274fbb48f259b13f4f30253932f69.1722532013.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 615d2de3b457272216d4179ceb82b3b2b86b1929 |
| Headers | show |
| Series | config.c: avoid segfault with --fixed-value and valueless config | expand |
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
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.
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
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
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" &&
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(-)