{"thread":{"id":"61876","subject":"git config set --file, key-value pair without '= value', gives Segmentation fault","startedAt":"2024-07-31T11:55:19Z","lastAt":"2024-08-01T01:28:48Z","messageCount":6,"participants":["Han Jiang","Taylor Blau","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"499687","messageId":"CANrWfmTek1xErBLrnoyhHN+gWU+rw14y6SQ+abZyzGoaBjmiKA@mail.gmail.com","threadId":"61876","inReplyTo":null,"subject":"git config set --file, key-value pair without '= value', gives Segmentation fault","fromName":"Han Jiang","fromEmail":"jhcarl0814@gmail.com","sentAt":"2024-07-31T11:55:06Z","receivedAt":"2024-07-31T11:55:19Z","isPatch":false,"sender":{"key":"jhcarl0814@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5171262?v=4"},"body":"Thank you for filling out a Git bug report!\nPlease answer the following questions to help us understand your issue.\n\nWhat did you do before the bug happened? (Steps to reproduce your issue)\n\ninput:\ncd '/'; cd '/'; rm --force --recursive -- './test_git'; mkdir \"$_\"; cd \"$_\";\ncat >'./config.txt' <<'EOF'\n[section]\nkey\n#key =\nkey = value1\nkey = value2\nkey = value3\nkey = value4\nEOF\ngit config set --file './config.txt' --value='[2]' --fixed-value\n'section.key' 'value6'\n\noutput: Segmentation fault\n$?: 139\n\nWhat did you expect to happen? (Expected behavior)\n\n'./config.txt' has 'key = value6' appended to its end\n\nWhat happened instead? (Actual behavior)\n\noutput: Segmentation fault\n$?: 139\n\nWhat's different between what you expected and what actually happened?\n\nAnything else you want to add:\n\nPlease review the rest of the bug report below.\nYou can delete any lines you don't wish to share.\n\n\n[System Info]\ngit version:\ngit version 2.46.0.windows.1\ncpu: x86_64\nbuilt from commit: 2e6a859ffc0471f60f79c1256f766042b0d5d17d\nsizeof-long: 4\nsizeof-size_t: 8\nshell-path: D:/git-sdk-64-build-installers/usr/bin/sh\nfeature: fsmonitor--daemon\nlibcurl: 8.9.0\nOpenSSL: OpenSSL 3.2.2 4 Jun 2024\nzlib: 1.3.1\nuname: Windows 10.0 22631\ncompiler info: gnuc: 14.1\nlibc info: no libc information available\n$SHELL (typically, interactive shell): C:\\Program Files\\Git\\usr\\bin\\bash.exe\n\n\n[Enabled Hooks]\nnot run from a git repository - no hooks to show\n"},{"id":"499745","messageId":"ZqqucpNgqSgZDPtA@nand.local","threadId":"61876","inReplyTo":"CANrWfmTek1xErBLrnoyhHN+gWU+rw14y6SQ+abZyzGoaBjmiKA@mail.gmail.com","subject":"Re: git config set --file, key-value pair without '= value', gives Segmentation fault","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-07-31T21:36:50Z","receivedAt":"2024-07-31T21:36:53Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jul 31, 2024 at 11:55:06PM +1200, Han Jiang wrote:\n> Thank you for filling out a Git bug report!\n\nThanks for the bugreport.\n\n> What did you do before the bug happened? (Steps to reproduce your issue)\n>\n> input:\n> cd '/'; cd '/'; rm --force --recursive -- './test_git'; mkdir \"$_\"; cd \"$_\";\n> cat >'./config.txt' <<'EOF'\n> [section]\n> key\n> #key =\n> key = value1\n> key = value2\n> key = value3\n> key = value4\n> EOF\n> git config set --file './config.txt' --value='[2]' --fixed-value\n> 'section.key' 'value6'\n\nI was able to trim reproduction script slightly to the following:\n\n--- 8< ---\nset -x\n\nrm -f config.txt*\n\ncat >'./config.txt' <<EOF\n[section]\nkey\nEOF\n\ngit.compile config set --file ./config.txt --value='[2]' --fixed-value section.key value6\n--- >8 ---\n\n, which just relies on having --value, --fixed-value, and a key without\nan explicit value.\n\nThe script you provided bisects to 00bbdde141 (builtin/config: introduce\n\"set\" subcommand, 2024-05-06), which is the commit which introduced the\nnew 'git config set' invocation.\n\nBut that appears to be a red herring, since the segfault happens in\nconfig.c::matches() here:\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\nSo I think that the behavior dates back to c90702a1f6 (config: plumb\n--fixed-value into config API, 2020-11-25). I think that the fix looks\nsomething like:\n\n--- 8< ---\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--- >8 ---\n\nI'd like to hear from Stolee (CC'd), who is the author of c90702a1f6\nbefore submitting this as a standalone patch.\n\nThanks,\nTaylor\n"},{"id":"499754","messageId":"xmqq5xslyyaf.fsf@gitster.g","threadId":"61876","inReplyTo":"ZqqucpNgqSgZDPtA@nand.local","subject":"Re: git config set --file, key-value pair without '= value', gives Segmentation fault","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-31T22:47:04Z","receivedAt":"2024-07-31T22:47:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"> So I think that the behavior dates back to c90702a1f6 (config: plumb\n> --fixed-value into config API, 2020-11-25). I think that the fix looks\n> something like:\n>\n> --- 8< ---\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>  \tif (!store->value_pattern)\n>  \t\treturn 1; /* always matches */\n\nHmph. fixed_value is about the string given on the command line\nbeing not a pattern.  And value that is NULL is the valueless \"true\"\nin this case.\n\nI would have actually expected the fix that follow your analysis (by\nthe way, I found it really well done) would say something more like\n\n\tif (strcmp(key, store->key))\n\t\treturn 0; /* not ours */\n+\tif (!value)\n+\t\tvalue = \"true\";\n\tif (store->fixed_value)\n\t\treturn !strcmp(store->fixed_value, value);\n\nbut I am not sure exactly how we want to handle synonymous Boolean\nvalues here.  Regardless of how \"true\" value is spelled in the\nconfiguration file, e.g.\n\n        [section]               [section]\n                key                     key = true\n\nI wonder if \"git config set --value=yes [--fixed-value] section.key\nfalse\" should replace either of them to false.\n\nIt would especially true if the command is invoked with --type=bool\nbut it seems that the --type option does not participate in the\nmatching with the current value.\n\n> I'd like to hear from Stolee (CC'd), who is the author of c90702a1f6\n> before submitting this as a standalone patch.\n\n"},{"id":"499756","messageId":"ZqrKeG+alFldB7OY@nand.local","threadId":"61876","inReplyTo":"xmqq5xslyyaf.fsf@gitster.g","subject":"Re: git config set --file, key-value pair without '= value', gives Segmentation fault","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-07-31T23:36:34Z","receivedAt":"2024-07-31T23:36:43Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jul 31, 2024 at 03:47:04PM -0700, Junio C Hamano wrote:\n> I would have actually expected the fix that follow your analysis (by\n> the way, I found it really well done) would say something more like\n>\n> \tif (strcmp(key, store->key))\n> \t\treturn 0; /* not ours */\n> +\tif (!value)\n> +\t\tvalue = \"true\";\n> \tif (store->fixed_value)\n> \t\treturn !strcmp(store->fixed_value, value);\n>\n> but I am not sure exactly how we want to handle synonymous Boolean\n> values here.  Regardless of how \"true\" value is spelled in the\n> configuration file, e.g.\n>\n>         [section]               [section]\n>                 key                     key = true\n>\n> I wonder if \"git config set --value=yes [--fixed-value] section.key\n> false\" should replace either of them to false.\n\nI think it gets tricky when we talk about the implicit true value\nhere for exactly that reason. Do we take true to mean just \"true\"? Or\n\"true\" and \"yes\", \"1\", \"\", and so on?\n\nI tried to match the behavior a few lines down in that function, where\nwe only call regexec() if value is non-NULL.\n\n> It would especially true if the command is invoked with --type=bool\n> but it seems that the --type option does not participate in the\n> matching with the current value.\n\nI think I could be convinced that --type=bool should treat \"\", \"true\",\n\"1\", \"on\", and so on as matching here, but I'm not sure.\n\n> > I'd like to hear from Stolee (CC'd), who is the author of c90702a1f6\n> > before submitting this as a standalone patch.\n\nThanks,\nTaylor\n"},{"id":"499757","messageId":"CANrWfmQ89+NN0_O+=g51DkXHJANPYYMYN_zK8Os9JBiuZiHHQA@mail.gmail.com","threadId":"61876","inReplyTo":"ZqrKeG+alFldB7OY@nand.local","subject":"Re: git config set --file, key-value pair without '= value', gives Segmentation fault","fromName":"Han Jiang","fromEmail":"jhcarl0814@gmail.com","sentAt":"2024-07-31T23:46:30Z","receivedAt":"2024-07-31T23:46:45Z","isPatch":false,"sender":{"key":"jhcarl0814@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5171262?v=4"},"body":"> but I am not sure exactly how we want to handle synonymous Boolean\n> values here.  Regardless of how \"true\" value is spelled in the\n> configuration file, e.g.\n>\n>         [section]               [section]\n>                 key                     key = true\n>\n> I wonder if \"git config set --value=yes [--fixed-value] section.key\n> false\" should replace either of them to false.\n\n'key' can be selected by `--value='!'`\n'key =' can be selected by `--value='^$'` or `--value='' --fixed-value`\ncommands for demonstration:\n\ncd '/'; cd '/'; rm --force --recursive -- './test_git'; mkdir \"$_\"; cd \"$_\";\ncat >'./config.txt' <<'EOF'\n[section]\n    key\n    key =\n    key = value1\n    key = value2\nEOF\ngit config set --file './config.txt' --value='^$' 'section.key' 'value3'\ngit config get --file './config.txt' --show-origin --show-scope --all\n'section.key'\ngit config get --file './config.txt' --show-origin --show-scope 'section.key'\n\ncat >'./config.txt' <<'EOF'\n[section]\n    key\n    key =\n    key = value1\n    key = value2\nEOF\ngit config set --file './config.txt' --value='^$' --fixed-value\n'section.key' 'value4'\ngit config get --file './config.txt' --show-origin --show-scope --all\n'section.key'\ngit config get --file './config.txt' --show-origin --show-scope 'section.key'\n\ncat >'./config.txt' <<'EOF'\n[section]\n    key\n    key =\n    key = value1\n    key = value2\nEOF\ngit config set --file './config.txt' --value='' 'section.key' 'value5'\ngit config get --file './config.txt' --show-origin --show-scope --all\n'section.key'\ngit config get --file './config.txt' --show-origin --show-scope 'section.key'\n\ncat >'./config.txt' <<'EOF'\n[section]\n    key\n    key =\n    key = value1\n    key = value2\nEOF\ngit config set --file './config.txt' --value='' --fixed-value\n'section.key' 'value6'\ngit config get --file './config.txt' --show-origin --show-scope --all\n'section.key'\ngit config get --file './config.txt' --show-origin --show-scope 'section.key'\n\ncat >'./config.txt' <<'EOF'\n[section]\n    key\n    key =\n    key = value1\n    key = value2\nEOF\ngit config set --file './config.txt' --value='!' 'section.key' 'value7'\ngit config get --file './config.txt' --show-origin --show-scope --all\n'section.key'\ngit config get --file './config.txt' --show-origin --show-scope 'section.key'\n\n> It would especially true if the command is invoked with --type=bool\n> but it seems that the --type option does not participate in the\n> matching with the current value.\n\n> I think I could be convinced that --type=bool should treat \"\", \"true\",\n> \"1\", \"on\", and so on as matching here, but I'm not sure.\n\nIt seems that `--regexp` and `--value=<regexp>` and `--fixed-value`\ntake effect before `--default=<default>` which is before\n`--type=<type>`, so creating a special case (treating 'key' as 'key =\ntrue' if `--type=bool` is present in downstream process, and when\n`--default=<default>` is present, requiring users to remember which\nhas higher priority or which takes effect first) would be too\nsurprising. (Currently you can use `--value=abcd` to kick 'key' away\nthen `--default=false` to turn 'key' into false, use `--value=abcd` to\nkick 'key=' away then `--default=true` to turn 'key=' into true.)\n\nOn Thu, Aug 1, 2024 at 11:36 AM Taylor Blau <me@ttaylorr.com> wrote:\n>\n> On Wed, Jul 31, 2024 at 03:47:04PM -0700, Junio C Hamano wrote:\n> > I would have actually expected the fix that follow your analysis (by\n> > the way, I found it really well done) would say something more like\n> >\n> >       if (strcmp(key, store->key))\n> >               return 0; /* not ours */\n> > +     if (!value)\n> > +             value = \"true\";\n> >       if (store->fixed_value)\n> >               return !strcmp(store->fixed_value, value);\n> >\n> > but I am not sure exactly how we want to handle synonymous Boolean\n> > values here.  Regardless of how \"true\" value is spelled in the\n> > configuration file, e.g.\n> >\n> >         [section]               [section]\n> >                 key                     key = true\n> >\n> > I wonder if \"git config set --value=yes [--fixed-value] section.key\n> > false\" should replace either of them to false.\n>\n> I think it gets tricky when we talk about the implicit true value\n> here for exactly that reason. Do we take true to mean just \"true\"? Or\n> \"true\" and \"yes\", \"1\", \"\", and so on?\n>\n> I tried to match the behavior a few lines down in that function, where\n> we only call regexec() if value is non-NULL.\n>\n> > It would especially true if the command is invoked with --type=bool\n> > but it seems that the --type option does not participate in the\n> > matching with the current value.\n>\n> I think I could be convinced that --type=bool should treat \"\", \"true\",\n> \"1\", \"on\", and so on as matching here, but I'm not sure.\n>\n> > > I'd like to hear from Stolee (CC'd), who is the author of c90702a1f6\n> > > before submitting this as a standalone patch.\n>\n> Thanks,\n> Taylor\n"},{"id":"499760","messageId":"xmqq1q39yqsy.fsf@gitster.g","threadId":"61876","inReplyTo":"ZqrKeG+alFldB7OY@nand.local","subject":"Re: git config set --file, key-value pair without '= value', gives Segmentation fault","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-01T01:28:45Z","receivedAt":"2024-08-01T01:28:48Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau <me@ttaylorr.com> writes:\n\n> I tried to match the behavior a few lines down in that function, where\n> we only call regexec() if value is non-NULL.\n\nYes, that is exactly what made me question the code.  With the\ncurrent code, valueless true cannot be updated with \"--value\"\nrestriction, ever, because it will never match anything.\n\nIn any case, the patch is a strict improvement over the current code\nthat segfaults.  Making it aware of \"--type=bool\" and have it treat\nsynonymous values of true/false properly would be better left as a\nfollow-up topic.\n\nThanks.\n"}]}