{"thread":{"id":"61881","subject":"[PATCH] config.c: avoid segfault with --fixed-value and valueless config","startedAt":"2024-08-01T17:06:57Z","lastAt":"2024-08-06T06:17:34Z","messageCount":5,"participants":["Taylor Blau","Patrick Steinhardt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"499878","messageId":"c78bacfa8fb274fbb48f259b13f4f30253932f69.1722532013.git.me@ttaylorr.com","threadId":"61881","inReplyTo":null,"subject":"[PATCH] config.c: avoid segfault with --fixed-value and valueless config","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-08-01T17:06:54Z","receivedAt":"2024-08-01T17:06:57Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When using `--fixed-value` with a key whose value is left empty (implied\nas being \"true\"), 'git config' may crash when invoked like either of:\n\n $ git config set --file=config --value=value --fixed-value \\\n section.key pattern\n $ git config --file=config --fixed-value section.key value pattern\n\nThe original bugreport[1] bisects to 00bbdde141 (builtin/config:\nintroduce \"set\" subcommand, 2024-05-06), which is a red-herring, since\nthe original bugreport uses the new 'git config set' invocation.\n\nThe behavior likely bisects back to c90702a1f6 (config: plumb\n--fixed-value into config API, 2020-11-25), which introduces the new\n--fixed-value option in the first place.\n\nLooking at the relevant frame from a failed process's coredump, the\ncrash appears in config.c::matches() like so:\n\n (gdb) up\n #1 0x000055b3e8b06022 in matches (key=0x55b3ea894360 \"section.key\", value=0x0,\n store=0x7ffe99076eb0) at config.c:2884\n 2884\t\t\treturn !strcmp(store->fixed_value, value);\n\nwhere we are trying to compare the `--fixed-value` argument to `value`,\nwhich is NULL.\n\nAvoid attempting to match `--fixed-value` for configuration keys with no\nexplicit value. A future patch could consider the empty value to mean\n\"true\", \"yes\", \"on\", etc. when invoked with `--type=bool`, but let's\npunt on that for now in the name of avoiding the segfault.\n\n[1]: https://lore.kernel.org/git/CANrWfmTek1xErBLrnoyhHN+gWU+rw14y6SQ+abZyzGoaBjmiKA@mail.gmail.com/\n\nReported-by: Han Jiang \nSigned-off-by: Taylor Blau \n---\n config.c | 2 +-\n t/t1300-config.sh | 9 +++++++++\n 2 files changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/config.c b/config.c\nindex 6421894614..05f369ec0d 100644\n--- a/config.c\n+++ b/config.c\n@@ -2914,7 +2914,7 @@ static int matches(const char *key, const char *value,\n {\n \tif (strcmp(key, store->key))\n \t\treturn 0; /* not ours */\n-\tif (store->fixed_value)\n+\tif (store->fixed_value && value)\n \t\treturn !strcmp(store->fixed_value, value);\n \tif (!store->value_pattern)\n \t\treturn 1; /* always matches */\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex 9de2d95f06..f13277c8f3 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -2704,6 +2704,15 @@ test_expect_success '--get and --get-all with --fixed-value' '\n \ttest_must_fail git config --file=config --get-regexp --fixed-value fixed+ non-existent\n '\n \n+test_expect_success '--fixed-value with value-less configuration' '\n+\ttest_when_finished rm -f config &&\n+\tcat >config <<-\\EOF &&\n+\t[section]\n+\t\tkey\n+\tEOF\n+\tgit config --file=config --fixed-value section.key value pattern\n+'\n+\n test_expect_success 'includeIf.hasconfig:remote.*.url' '\n \tgit init hasremoteurlTest &&\n \ttest_when_finished \"rm -rf hasremoteurlTest\" &&\n-- \n2.46.0.dirty\n"},{"id":"500064","messageId":"ZrDH3ryOO0wIJIDE@tanuki","threadId":"61881","inReplyTo":"c78bacfa8fb274fbb48f259b13f4f30253932f69.1722532013.git.me@ttaylorr.com","subject":"Re: [PATCH] config.c: avoid segfault with --fixed-value and valueless config","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-05T12:38:54Z","receivedAt":"2024-08-05T12:39:00Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Aug 01, 2024 at 01:06:54PM -0400, Taylor Blau wrote:\n> When using `--fixed-value` with a key whose value is left empty (implied\n> as being \"true\"), 'git config' may crash when invoked like either of:\n> \n> $ git config set --file=config --value=value --fixed-value \\\n> section.key pattern\n> $ git config --file=config --fixed-value section.key value pattern\n> \n> The original bugreport[1] bisects to 00bbdde141 (builtin/config:\n> introduce \"set\" subcommand, 2024-05-06), which is a red-herring, since\n> the original bugreport uses the new 'git config set' invocation.\n> \n> The behavior likely bisects back to c90702a1f6 (config: plumb\n> --fixed-value into config API, 2020-11-25), which introduces the new\n> --fixed-value option in the first place.\n> \n> Looking at the relevant frame from a failed process's coredump, the\n> crash appears in config.c::matches() like so:\n> \n> (gdb) up\n> #1 0x000055b3e8b06022 in matches (key=0x55b3ea894360 \"section.key\", value=0x0,\n> store=0x7ffe99076eb0) at config.c:2884\n> 2884\t\t\treturn !strcmp(store->fixed_value, value);\n> \n> where we are trying to compare the `--fixed-value` argument to `value`,\n> which is NULL.\n> \n> Avoid attempting to match `--fixed-value` for configuration keys with no\n> explicit value. A future patch could consider the empty value to mean\n> \"true\", \"yes\", \"on\", etc. when invoked with `--type=bool`, but let's\n> punt on that for now in the name of avoiding the segfault.\n\nEdge cases like this really make me wonder what the benefit of implicit\nbools is in our config files. They have been a source of bugs, including\nthis one, and in my opinion only lead to confusion when reading through\na config file manually. I would claim that 99% of our users out there\ndon't even know that you can have implicit booleans, and would think\nthat the config is invalid. And the 1% that do know probably don't care\nmuch. It's not even like it would safe you a ton of typing.\n\nSo... why do we have them in the first place? Is there even a single\ngood reason?\n\n> [1]: https://lore.kernel.org/git/CANrWfmTek1xErBLrnoyhHN+gWU+rw14y6SQ+abZyzGoaBjmiKA@mail.gmail.com/\n> \n> Reported-by: Han Jiang \n> Signed-off-by: Taylor Blau \n> ---\n> config.c | 2 +-\n> t/t1300-config.sh | 9 +++++++++\n> 2 files changed, 10 insertions(+), 1 deletion(-)\n> \n> diff --git a/config.c b/config.c\n> index 6421894614..05f369ec0d 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -2914,7 +2914,7 @@ static int matches(const char *key, const char *value,\n> {\n> \tif (strcmp(key, store->key))\n> \t\treturn 0; /* not ours */\n> -\tif (store->fixed_value)\n> +\tif (store->fixed_value && value)\n> \t\treturn !strcmp(store->fixed_value, value);\n\nOkay, makes sense. I think we should at least have a comment here saying\nthat we simply ignore keys with implicit values. I was also wondering\nwhether we want to warn about those such that users are aware in case we\nignore them?\n\nPatrick\n"},{"id":"500089","messageId":"xmqqa5hr55yb.fsf@gitster.g","threadId":"61881","inReplyTo":"ZrDH3ryOO0wIJIDE@tanuki","subject":"Re: [PATCH] config.c: avoid segfault with --fixed-value and valueless config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-05T15:45:32Z","receivedAt":"2024-08-05T15:45:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> Edge cases like this really make me wonder what the benefit of implicit\n> bools is in our config files.\n>\n> So... why do we have them in the first place? Is there even a single\n> good reason?\n\nThere isn't any good reason to introduce such a syntax if we were\ndesigining the configuration file format from scratch. It was added\nbecause originally Linus thought it was a cute syntax, and these\ndays a lot lot more importantly, it is kept working because you will\nbreak a lot of existing configuration files that were hand tweaked\nif you remove the support suddenly.\n\n"},{"id":"500122","messageId":"ZrEsIhWTgkdNn3I/@nand.local","threadId":"61881","inReplyTo":"xmqqa5hr55yb.fsf@gitster.g","subject":"Re: [PATCH] config.c: avoid segfault with --fixed-value and valueless config","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-08-05T19:46:42Z","receivedAt":"2024-08-05T19:46:45Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Aug 05, 2024 at 08:45:32AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n>\n> > Edge cases like this really make me wonder what the benefit of implicit\n> > bools is in our config files.\n> >\n> > So... why do we have them in the first place? Is there even a single\n> > good reason?\n>\n> There isn't any good reason to introduce such a syntax if we were\n> desigining the configuration file format from scratch. It was added\n> because originally Linus thought it was a cute syntax, and these\n> days a lot lot more importantly, it is kept working because you will\n> break a lot of existing configuration files that were hand tweaked\n> if you remove the support suddenly.\n\nI agree. It's perhaps interesting to think about in the context of the\ndiscussion in [1], but I think also worth having some perspective above.\n\nSure, this configuration syntax would not be invented anew today, but I\nalso don't think it's worth breaking existing configurations, even in a\nhypothetical \"Git 3.0\" release.\n\nIn some sense I am sympathetic to Patrick's argument, but I also think\nthat having a bug in a relatively niche feature like --fixed-value that\nwasn't noticed for almost four years over 17 [2] releases isn't itself a\nstrong argument for removing the feature.\n\nThanks,\nTaylor\n\n[1]: \n[2]: $ git tag --contains c90702a1f6 'v2.*.0' | wc -l\n"},{"id":"500154","messageId":"ZrG_-Rgi85bswmId@tanuki","threadId":"61881","inReplyTo":"ZrEsIhWTgkdNn3I/@nand.local","subject":"Re: [PATCH] config.c: avoid segfault with --fixed-value and valueless config","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-06T06:17:29Z","receivedAt":"2024-08-06T06:17:34Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Aug 05, 2024 at 03:46:42PM -0400, Taylor Blau wrote:\n> On Mon, Aug 05, 2024 at 08:45:32AM -0700, Junio C Hamano wrote:\n> > Patrick Steinhardt writes:\n> >\n> > > Edge cases like this really make me wonder what the benefit of implicit\n> > > bools is in our config files.\n> > >\n> > > So... why do we have them in the first place? Is there even a single\n> > > good reason?\n> >\n> > There isn't any good reason to introduce such a syntax if we were\n> > desigining the configuration file format from scratch. It was added\n> > because originally Linus thought it was a cute syntax, and these\n> > days a lot lot more importantly, it is kept working because you will\n> > break a lot of existing configuration files that were hand tweaked\n> > if you remove the support suddenly.\n> \n> I agree. It's perhaps interesting to think about in the context of the\n> discussion in [1], but I think also worth having some perspective above.\n> \n> Sure, this configuration syntax would not be invented anew today, but I\n> also don't think it's worth breaking existing configurations, even in a\n> hypothetical \"Git 3.0\" release.\n> \n> In some sense I am sympathetic to Patrick's argument, but I also think\n> that having a bug in a relatively niche feature like --fixed-value that\n> wasn't noticed for almost four years over 17 [2] releases isn't itself a\n> strong argument for removing the feature.\n\nI was really just wondering whether there is actually a good reason to\nhave it that I couldn't think of. I certainly think that this feature\nshouldn't exist, but also agree that removing it now would create more\nhassle than benefit.\n\nThanks for the context!\n\nPatrick\n"}]}