{"thread":{"id":"37286","subject":"[PATCH] pretty.c: make git_pretty_formats_config return -1 on git_config_string failure","startedAt":"2014-08-04T14:41:15Z","lastAt":"2014-08-04T22:25:23Z","messageCount":8,"participants":["Tanay Abhra","Matthieu Moy","Eric Sunshine","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"247209","messageId":"1407163275-3006-1-git-send-email-tanayabh@gmail.com","threadId":"37286","inReplyTo":null,"subject":"[PATCH] pretty.c: make git_pretty_formats_config return -1 on git_config_string failure","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-08-04T14:41:15Z","receivedAt":"2014-08-04T14:41:15Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"`git_pretty_formats_config()` continues without checking git_config_string's\nreturn value which can lead to a SEGFAULT. Instead return -1 when\ngit_config_string fails signalling `git_config()` to die printing the location\nof the erroneous variable.\n\nSigned-off-by: Tanay Abhra \n---\n pretty.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex 3a1da6f..72dbf55 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -65,7 +65,9 @@ static int git_pretty_formats_config(const char *var, const char *value, void *c\n \n \tcommit_format->name = xstrdup(name);\n \tcommit_format->format = CMIT_FMT_USERFORMAT;\n-\tgit_config_string(&fmt, var, value);\n+\tif (git_config_string(&fmt, var, value))\n+\t\treturn -1;\n+\n \tif (starts_with(fmt, \"format:\") || starts_with(fmt, \"tformat:\")) {\n \t\tcommit_format->is_tformat = fmt[0] == 't';\n \t\tfmt = strchr(fmt, ':') + 1;\n-- \n1.9.0.GIT\n"},{"id":"247212","messageId":"vpqmwbki7h3.fsf@anie.imag.fr","threadId":"37286","inReplyTo":"1407163275-3006-1-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH] pretty.c: make git_pretty_formats_config return -1 on git_config_string failure","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-08-04T15:45:44Z","receivedAt":"2014-08-04T15:45:44Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tanay Abhra writes:\n\n> `git_pretty_formats_config()` continues without checking git_config_string's\n> return value which can lead to a SEGFAULT.\n\nIndeed, without the patch:\n\n$ git -c pretty.my= log --pretty=my \nerror: Missing value for 'pretty.my' \nzsh: segmentation fault git -c pretty.my= log --pretty=my\n\n> diff --git a/pretty.c b/pretty.c\n> index 3a1da6f..72dbf55 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -65,7 +65,9 @@ static int git_pretty_formats_config(const char *var, const char *value, void *c\n> \n> \tcommit_format->name = xstrdup(name);\n> \tcommit_format->format = CMIT_FMT_USERFORMAT;\n> -\tgit_config_string(&fmt, var, value);\n> +\tif (git_config_string(&fmt, var, value))\n> +\t\treturn -1;\n> +\n\nAck-ed-by: Matthieu Moy \n\nMy first thought reading this was \"why not rewrite using non-callback\nAPI?\", but this particular call to git_config needs to iterate over\nconfig keys anyway.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"247236","messageId":"CAPig+cRo05mG9yeU61VSjAvXWRHU9soaaH-Cv7MKoxZ=it15Rw@mail.gmail.com","threadId":"37286","inReplyTo":"vpqmwbki7h3.fsf@anie.imag.fr","subject":"Re: [PATCH] pretty.c: make git_pretty_formats_config return -1 on git_config_string failure","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-08-04T18:56:03Z","receivedAt":"2014-08-04T18:56:03Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Aug 4, 2014 at 11:45 AM, Matthieu Moy\n wrote:\n> Tanay Abhra writes:\n>\n>> `git_pretty_formats_config()` continues without checking git_config_string's\n>> return value which can lead to a SEGFAULT.\n>\n> Indeed, without the patch:\n>\n> $ git -c pretty.my= log --pretty=my\n> error: Missing value for 'pretty.my'\n> zsh: segmentation fault git -c pretty.my= log --pretty=my\n\nThis probably should be formalized as a proper test and included with\nTanay's patch.\n\n>> diff --git a/pretty.c b/pretty.c\n>> index 3a1da6f..72dbf55 100644\n>> --- a/pretty.c\n>> +++ b/pretty.c\n>> @@ -65,7 +65,9 @@ static int git_pretty_formats_config(const char *var, const char *value, void *c\n>>\n>> commit_format->name = xstrdup(name);\n>> commit_format->format = CMIT_FMT_USERFORMAT;\n>> - git_config_string(&fmt, var, value);\n>> + if (git_config_string(&fmt, var, value))\n>> + return -1;\n>> +\n>\n> Ack-ed-by: Matthieu Moy \n>\n> My first thought reading this was \"why not rewrite using non-callback\n> API?\", but this particular call to git_config needs to iterate over\n> config keys anyway.\n>\n> --\n> Matthieu Moy\n> http://www-verimag.imag.fr/~moy/\n"},{"id":"247241","messageId":"vpqy4v4avc0.fsf@anie.imag.fr","threadId":"37286","inReplyTo":"CAPig+cRo05mG9yeU61VSjAvXWRHU9soaaH-Cv7MKoxZ=it15Rw@mail.gmail.com","subject":"Re: [PATCH] pretty.c: make git_pretty_formats_config return -1 on git_config_string failure","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-08-04T19:49:51Z","receivedAt":"2014-08-04T19:49:51Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Eric Sunshine writes:\n\n> On Mon, Aug 4, 2014 at 11:45 AM, Matthieu Moy\n> wrote:\n>> Tanay Abhra writes:\n>>\n>>> `git_pretty_formats_config()` continues without checking git_config_string's\n>>> return value which can lead to a SEGFAULT.\n>>\n>> Indeed, without the patch:\n>>\n>> $ git -c pretty.my= log --pretty=my\n>> error: Missing value for 'pretty.my'\n>> zsh: segmentation fault git -c pretty.my= log --pretty=my\n>\n> This probably should be formalized as a proper test and included with\n> Tanay's patch.\n\nNot sure it's worth the trouble: the bug corresponds to a\nmis-application of a pattern used in tens of places in Git's code\n(basically, each call to git_config_string, 50 callsites). Testing this\nparticular case does not ensure non-regression, and testing all\noccurences of the pattern would be overkill IMHO.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"247247","messageId":"20140804203351.GA12898@peff.net","threadId":"37286","inReplyTo":"vpqmwbki7h3.fsf@anie.imag.fr","subject":"Re: [PATCH] pretty.c: make git_pretty_formats_config return -1 on git_config_string failure","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-04T20:33:51Z","receivedAt":"2014-08-04T20:33:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 04, 2014 at 05:45:44PM +0200, Matthieu Moy wrote:\n\n> Tanay Abhra writes:\n> \n> > `git_pretty_formats_config()` continues without checking git_config_string's\n> > return value which can lead to a SEGFAULT.\n> \n> Indeed, without the patch:\n> \n> $ git -c pretty.my= log --pretty=my \n> error: Missing value for 'pretty.my' \n> zsh: segmentation fault git -c pretty.my= log --pretty=my\n\nHmm. Not related to the original patch, but that really looks like a\nbug. Shouldn't \"git -c pretty.my= ...\" set pretty.my to the empty string?\n\nI'd expect \"git -c pretty.my ...\" to set it to NULL (i.e., the \"implicit\ntrue\" you get from omitting the \"=\" in the config files themselves).\n\n-Peff\n"},{"id":"247254","messageId":"vpqtx5s7yo4.fsf@anie.imag.fr","threadId":"37286","inReplyTo":"20140804203351.GA12898@peff.net","subject":"Re: [PATCH] pretty.c: make git_pretty_formats_config return -1 on git_config_string failure","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-08-04T21:06:03Z","receivedAt":"2014-08-04T21:06:03Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Jeff King writes:\n\n> On Mon, Aug 04, 2014 at 05:45:44PM +0200, Matthieu Moy wrote:\n>\n>> Tanay Abhra writes:\n>> \n>> > `git_pretty_formats_config()` continues without checking git_config_string's\n>> > return value which can lead to a SEGFAULT.\n>> \n>> Indeed, without the patch:\n>> \n>> $ git -c pretty.my= log --pretty=my \n>> error: Missing value for 'pretty.my' \n>> zsh: segmentation fault git -c pretty.my= log --pretty=my\n>\n> Hmm. Not related to the original patch, but that really looks like a\n> bug. Shouldn't \"git -c pretty.my= ...\" set pretty.my to the empty string?\n>\n> I'd expect \"git -c pretty.my ...\" to set it to NULL (i.e., the \"implicit\n> true\" you get from omitting the \"=\" in the config files themselves).\n\nIndeed.\n\nstrbuf_split_buf() does not seem to distinguish between x= and x. No\ntime to debug this further, sorry.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"247258","messageId":"20140804215644.GA21510@peff.net","threadId":"37286","inReplyTo":"vpqtx5s7yo4.fsf@anie.imag.fr","subject":"[PATCH] config: teach \"git -c\" to recognize an empty string","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-04T21:56:44Z","receivedAt":"2014-08-04T21:56:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 04, 2014 at 11:06:03PM +0200, Matthieu Moy wrote:\n\n> > Hmm. Not related to the original patch, but that really looks like a\n> > bug. Shouldn't \"git -c pretty.my= ...\" set pretty.my to the empty string?\n> >\n> > I'd expect \"git -c pretty.my ...\" to set it to NULL (i.e., the \"implicit\n> > true\" you get from omitting the \"=\" in the config files themselves).\n> \n> Indeed.\n> \n> strbuf_split_buf() does not seem to distinguish between x= and x. No\n> time to debug this further, sorry.\n\nOh, I didn't expect you to work on it. The bug is totally my fault. :)\nYour email just made me realize it was there.\n\nHere's a patch to fix it.\n\n-- >8 --\nSubject: config: teach \"git -c\" to recognize an empty string\n\nIn a config file, you can do:\n\n [foo]\n bar\n\nto turn the \"foo.bar\" boolean flag on, and you can do:\n\n [foo]\n bar=\n\nto set \"foo.bar\" to the empty string. However, git's \"-c\"\nparameter treats both:\n\n git -c foo.bar\n\nand\n\n git -c foo.bar=\n\nas the boolean flag, and there is no way to set a variable\nto the empty string. This patch enables the latter form to\ndo that.\n\nSigned-off-by: Jeff King \n---\nThis is technically a backwards incompatibility, but I'd consider it a\nsimple bugfix. The existing behavior was unintentional, made no sense,\nand was never documented.\n\nLooking over strbuf_split's interface, I think it's rather\ncounter-intuitive, and I was tempted to change it. But there are several\nother callers that rely on it, and the chance for introducing a subtle\nbug is high. This is the least invasive fix (and it really is not any\nless readable than what was already there :) ).\n\n Documentation/git.txt | 5 +++++\n config.c | 12 ++++++++++--\n t/t1300-repo-config.sh | 11 +++++++++++\n 3 files changed, 26 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex b1c4f7a..e7783f0 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -447,6 +447,11 @@ example the following invocations are equivalent:\n \tgiven will override values from configuration files.\n \tThe is expected in the same format as listed by\n \t'git config' (subkeys separated by dots).\n++\n+Note that omitting the `=` in `git -c foo.bar ...` is allowed and sets\n+`foo.bar` to the boolean true value (just like `[foo]bar` would in a\n+config file). Including the equals but with an empty value (like `git -c\n+foo.bar= ...`) sets `foo.bar` to the empty string.\n \n --exec-path[=]::\n \tPath to wherever your core Git programs are installed.\ndiff --git a/config.c b/config.c\nindex 058505c..fe6216f 100644\n--- a/config.c\n+++ b/config.c\n@@ -162,19 +162,27 @@ void git_config_push_parameter(const char *text)\n int git_config_parse_parameter(const char *text,\n \t\t\t config_fn_t fn, void *data)\n {\n+\tconst char *value;\n \tstruct strbuf **pair;\n+\n \tpair = strbuf_split_str(text, '=', 2);\n \tif (!pair[0])\n \t\treturn error(\"bogus config parameter: %s\", text);\n-\tif (pair[0]->len && pair[0]->buf[pair[0]->len - 1] == '=')\n+\n+\tif (pair[0]->len && pair[0]->buf[pair[0]->len - 1] == '=') {\n \t\tstrbuf_setlen(pair[0], pair[0]->len - 1);\n+\t\tvalue = pair[1] ? pair[1]->buf : \"\";\n+\t} else\n+\t\tvalue = NULL;\n+\n \tstrbuf_trim(pair[0]);\n \tif (!pair[0]->len) {\n \t\tstrbuf_list_free(pair);\n \t\treturn error(\"bogus config parameter: %s\", text);\n \t}\n+\n \tstrbuf_tolower(pair[0]);\n-\tif (fn(pair[0]->buf, pair[1] ? pair[1]->buf : NULL, data) < 0) {\n+\tif (fn(pair[0]->buf, value, data) < 0) {\n \t\tstrbuf_list_free(pair);\n \t\treturn -1;\n \t}\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex 3f80ff0..46f6ae2 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -1010,6 +1010,17 @@ test_expect_success 'git -c \"key=value\" support' '\n \ttest_must_fail git -c name=value config core.name\n '\n \n+# We just need a type-specifier here that cares about the\n+# distinction internally between a NULL boolean and a real\n+# string (because most of git's internal parsers do care).\n+# Using \"--path\" works, but we do not otherwise care about\n+# its semantics.\n+test_expect_success 'git -c can represent empty string' '\n+\techo >expect &&\n+\tgit -c foo.empty= config --path foo.empty >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'key sanity-checking' '\n \ttest_must_fail git config foo=bar &&\n \ttest_must_fail git config foo=.bar &&\n-- \n2.1.0.rc0.286.g5c67d74\n"},{"id":"247260","messageId":"xmqq61i7riy4.fsf@gitster.dls.corp.google.com","threadId":"37286","inReplyTo":"20140804215644.GA21510@peff.net","subject":"Re: [PATCH] config: teach \"git -c\" to recognize an empty string","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-04T22:25:23Z","receivedAt":"2014-08-04T22:25:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> This is technically a backwards incompatibility, but I'd consider it a\n> simple bugfix. The existing behavior was unintentional, made no sense,\n> and was never documented.\n\nYeah, I tend to agree. I actually would not shed any tears if the\nbreakage were that it was impossible to pass \"NULL is true\" boolean\nvia \"git -c\" interface, but it is the other way around. It is much\nmore grave a problem that we cannot pass an empty string as a value,\nand we should fix it.\n\n> Looking over strbuf_split's interface, I think it's rather\n> counter-intuitive, and I was tempted to change it. But there are several\n> other callers that rely on it, and the chance for introducing a subtle\n> bug is high. This is the least invasive fix (and it really is not any\n> less readable than what was already there :) ).\n\n;-)\n\n> +# We just need a type-specifier here that cares about the\n> +# distinction internally between a NULL boolean and a real\n> +# string (because most of git's internal parsers do care).\n> +# Using \"--path\" works, but we do not otherwise care about\n> +# its semantics.\n> +test_expect_success 'git -c can represent empty string' '\n> +\techo >expect &&\n> +\tgit -c foo.empty= config --path foo.empty >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nAnother way may be \"git config -l\" and see if we see a = on the\nentry for foo.empty, but I think the way you did this is nicer.\n\n> test_expect_success 'key sanity-checking' '\n> \ttest_must_fail git config foo=bar &&\n> \ttest_must_fail git config foo=.bar &&\n"}]}