{"thread":{"id":"26994","subject":"Value size limits on git config files","startedAt":"2011-04-05T16:29:31Z","lastAt":"2011-04-05T18:15:02Z","messageCount":4,"participants":["Jeff Adamson","Erik Faye-Lund","Drew Northup"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"165183","messageId":"BANLkTikknAiYr4y-it3+qZ3dnXE56dQBoQ@mail.gmail.com","threadId":"26994","inReplyTo":null,"subject":"Value size limits on git config files","fromName":"Jeff Adamson","fromEmail":"jwa@urbancode.com","sentAt":"2011-04-05T16:29:31Z","receivedAt":"2011-04-05T16:29:31Z","isPatch":false,"sender":{"key":"jwa@urbancode.com","avatar":null},"body":"When trying to incorporate large aliases into my ~/.gitconfig file via\n`git config --global \"!$(cat myscript)\"`, I determined that there\nappears to be a limit to the size of a value.  When exceeded, git\nprints out \"fatal: bad config file line 82 in ~/.gitconfig\".  I was\nable to strip enough comments and such from myscript that it then no\nlonger invalidated the config once the value was less than 1024 chars.\n (Minor aside, I am doing this as a small experiment to potentially\naid in synchronizing all my git behaviors across machines by inlining\nthem into the config file.  Whether this is a good path to explore or\nnot is a separate issue.)\n\nMy questions are:\n   Is this 1024 char limit on config file values documented somewhere?\n   Should it be considered a bug that git config can write out config\nfiles which it can not then read (e.g. it should be changed to either\nenforce the value-length limit during writing, or the parser updated\nto handle large values)?\n\n\nThanks,\nJeff\n"},{"id":"165185","messageId":"BANLkTimLxPdr=mGJfxmdKkeq0zouxz05Uw@mail.gmail.com","threadId":"26994","inReplyTo":"BANLkTikknAiYr4y-it3+qZ3dnXE56dQBoQ@mail.gmail.com","subject":"Re: Value size limits on git config files","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-04-05T17:01:27Z","receivedAt":"2011-04-05T17:01:27Z","isPatch":false,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Tue, Apr 5, 2011 at 6:29 PM, Jeff Adamson <jwa@urbancode.com> wrote:\n> When trying to incorporate large aliases into my ~/.gitconfig file via\n> `git config --global \"!$(cat myscript)\"`, I determined that there\n> appears to be a limit to the size of a value.  When exceeded, git\n> prints out \"fatal: bad config file line 82 in ~/.gitconfig\".  I was\n> able to strip enough comments and such from myscript that it then no\n> longer invalidated the config once the value was less than 1024 chars.\n>  (Minor aside, I am doing this as a small experiment to potentially\n> aid in synchronizing all my git behaviors across machines by inlining\n> them into the config file.  Whether this is a good path to explore or\n> not is a separate issue.)\n>\n> My questions are:\n>    Is this 1024 char limit on config file values documented somewhere?\n>    Should it be considered a bug that git config can write out config\n> files which it can not then read (e.g. it should be changed to either\n> enforce the value-length limit during writing, or the parser updated\n> to handle large values)?\n>\n\nIt's due to use of a fixed-size buffer. This patch fixes it for me:\n\ndiff --git a/config.c b/config.c\nindex 0abcada..bc6ea49 100644\n--- a/config.c\n+++ b/config.c\n@@ -133,23 +133,20 @@ static int get_next_char(void)\n\n static char *parse_value(void)\n {\n-\tstatic char value[1024];\n-\tint quote = 0, comment = 0, len = 0, space = 0;\n+\tstruct strbuf value = STRBUF_INIT;\n+\tint quote = 0, comment = 0, space = 0;\n\n \tfor (;;) {\n \t\tint c = get_next_char();\n-\t\tif (len >= sizeof(value) - 1)\n-\t\t\treturn NULL;\n \t\tif (c == '\\n') {\n \t\t\tif (quote)\n \t\t\t\treturn NULL;\n-\t\t\tvalue[len] = 0;\n-\t\t\treturn value;\n+\t\t\treturn strbuf_detach(&value, NULL);\n \t\t}\n \t\tif (comment)\n \t\t\tcontinue;\n \t\tif (isspace(c) && !quote) {\n-\t\t\tif (len)\n+\t\t\tif (value.len)\n \t\t\t\tspace++;\n \t\t\tcontinue;\n \t\t}\n@@ -160,7 +157,7 @@ static char *parse_value(void)\n \t\t\t}\n \t\t}\n \t\tfor (; space; space--)\n-\t\t\tvalue[len++] = ' ';\n+\t\t\tstrbuf_addch(&value, ' ');\n \t\tif (c == '\\\\') {\n \t\t\tc = get_next_char();\n \t\t\tswitch (c) {\n@@ -180,16 +177,17 @@ static char *parse_value(void)\n \t\t\t\tbreak;\n \t\t\t/* Reject unknown escape sequences */\n \t\t\tdefault:\n+\t\t\t\tstrbuf_release(&value);\n \t\t\t\treturn NULL;\n \t\t\t}\n-\t\t\tvalue[len++] = c;\n+\t\t\tstrbuf_addch(&value, c);\n \t\t\tcontinue;\n \t\t}\n \t\tif (c == '\"') {\n \t\t\tquote = 1-quote;\n \t\t\tcontinue;\n \t\t}\n-\t\tvalue[len++] = c;\n+\t\tstrbuf_addch(&value, c);\n \t}\n }\n\n@@ -200,7 +198,7 @@ static inline int iskeychar(int c)\n\n static int get_value(config_fn_t fn, void *data, char *name, unsigned int len)\n {\n-\tint c;\n+\tint c, ret;\n \tchar *value;\n\n \t/* Get the full name */\n@@ -226,7 +224,9 @@ static int get_value(config_fn_t fn, void *data,\nchar *name, unsigned int len)\n \t\tif (!value)\n \t\t\treturn -1;\n \t}\n-\treturn fn(name, value, data);\n+\tret = fn(name, value, data);\n+\tfree(value);\n+\treturn ret;\n }\n\n static int get_extended_base_var(char *name, int baselen, int c)\n"},{"id":"165191","messageId":"1302024946.17813.57.camel@drew-northup.unet.maine.edu","threadId":"26994","inReplyTo":"BANLkTimLxPdr=mGJfxmdKkeq0zouxz05Uw@mail.gmail.com","subject":"Re: Value size limits on git config files","fromName":"Drew Northup","fromEmail":"drew.northup@maine.edu","sentAt":"2011-04-05T17:35:46Z","receivedAt":"2011-04-05T17:35:46Z","isPatch":false,"sender":{"key":"drew.northup@maine.edu","avatar":"https://avatars.githubusercontent.com/u/18331571?v=4"},"body":"\nOn Tue, 2011-04-05 at 19:01 +0200, Erik Faye-Lund wrote:\n> On Tue, Apr 5, 2011 at 6:29 PM, Jeff Adamson <jwa@urbancode.com> wrote:\n> I was\n> > able to strip enough comments and such from myscript that it then no\n> > longer invalidated the config once the value was less than 1024 chars.\n\n> It's due to use of a fixed-size buffer. This patch fixes it for me:\n> \n> diff --git a/config.c b/config.c\n> index 0abcada..bc6ea49 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -133,23 +133,20 @@ static int get_next_char(void)\n> \n>  static char *parse_value(void)\n>  {\n> -\tstatic char value[1024];\n> -\tint quote = 0, comment = 0, len = 0, space = 0;\n> +\tstruct strbuf value = STRBUF_INIT;\n> +\tint quote = 0, comment = 0, space = 0;\n\nEric,\nYou're doing a lot more here than just making a simple char buffer\nlarger...\n\n>  \tfor (;;) {\n>  \t\tint c = get_next_char();\n> -\t\tif (len >= sizeof(value) - 1)\n> -\t\t\treturn NULL;\n>  \t\tif (c == '\\n') {\n>  \t\t\tif (quote)\n>  \t\t\t\treturn NULL;\n> -\t\t\tvalue[len] = 0;\n> -\t\t\treturn value;\n> +\t\t\treturn strbuf_detach(&value, NULL);\n\n...ditto...\n\n>  \t\t}\n>  \t\tif (comment)\n>  \t\t\tcontinue;\n>  \t\tif (isspace(c) && !quote) {\n> -\t\t\tif (len)\n> +\t\t\tif (value.len)\n\n...ditto...\n\n>  \t\t\t\tspace++;\n>  \t\t\tcontinue;\n>  \t\t}\n> @@ -160,7 +157,7 @@ static char *parse_value(void)\n>  \t\t\t}\n>  \t\t}\n>  \t\tfor (; space; space--)\n> -\t\t\tvalue[len++] = ' ';\n> +\t\t\tstrbuf_addch(&value, ' ');\n\n...ditto...\n\n(The rest cut for discussion...)\n\nThe the first question that needs to be asked is: Is there a reason why\nit was still only 1kiB long?\nThe second is why adopt the struct here and not use an expanded char[]\nelement?\n\nI'm not saying this is wrong by any means, but it is a lot more than\njust a simple change in the length of a char buffer.\n\n-- \n-Drew Northup\n________________________________________________\n\"As opposed to vegetable or mineral error?\"\n-John Pescatore, SANS NewsBites Vol. 12 Num. 59\n"},{"id":"165194","messageId":"BANLkTikwX4xoeyrskKEC83Wvz5Y8_7ykeA@mail.gmail.com","threadId":"26994","inReplyTo":"1302024946.17813.57.camel@drew-northup.unet.maine.edu","subject":"Re: Value size limits on git config files","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-04-05T18:15:02Z","receivedAt":"2011-04-05T18:15:02Z","isPatch":false,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Tue, Apr 5, 2011 at 7:35 PM, Drew Northup <drew.northup@maine.edu> wrote:\n>\n> On Tue, 2011-04-05 at 19:01 +0200, Erik Faye-Lund wrote:\n>> On Tue, Apr 5, 2011 at 6:29 PM, Jeff Adamson <jwa@urbancode.com> wrote:\n>> I was\n>> > able to strip enough comments and such from myscript that it then no\n>> > longer invalidated the config once the value was less than 1024 chars.\n>\n>> It's due to use of a fixed-size buffer. This patch fixes it for me:\n>>\n>> diff --git a/config.c b/config.c\n>> index 0abcada..bc6ea49 100644\n>> --- a/config.c\n>> +++ b/config.c\n>> @@ -133,23 +133,20 @@ static int get_next_char(void)\n>>\n>>  static char *parse_value(void)\n>>  {\n>> -     static char value[1024];\n>> -     int quote = 0, comment = 0, len = 0, space = 0;\n>> +     struct strbuf value = STRBUF_INIT;\n>> +     int quote = 0, comment = 0, space = 0;\n>\n> Eric,\n> You're doing a lot more here than just making a simple char buffer\n> larger...\n>\n\nI'm not quite sure why you're telling me this. After all, I wrote the\npatch - of course I know that.\n\n>>       for (;;) {\n>>               int c = get_next_char();\n>> -             if (len >= sizeof(value) - 1)\n>> -                     return NULL;\n>>               if (c == '\\n') {\n>>                       if (quote)\n>>                               return NULL;\n>> -                     value[len] = 0;\n>> -                     return value;\n>> +                     return strbuf_detach(&value, NULL);\n>\n> ...ditto...\n>\n>>               }\n>>               if (comment)\n>>                       continue;\n>>               if (isspace(c) && !quote) {\n>> -                     if (len)\n>> +                     if (value.len)\n>\n> ...ditto...\n>\n>>                               space++;\n>>                       continue;\n>>               }\n>> @@ -160,7 +157,7 @@ static char *parse_value(void)\n>>                       }\n>>               }\n>>               for (; space; space--)\n>> -                     value[len++] = ' ';\n>> +                     strbuf_addch(&value, ' ');\n>\n> ...ditto...\n>\n> (The rest cut for discussion...)\n>\n> The the first question that needs to be asked is: Is there a reason why\n> it was still only 1kiB long?\n> The second is why adopt the struct here and not use an expanded char[]\n> element?\n>\n\nYeah, and I don't know the answer to those questions.\n\nBut I do know how to fix the problem, so I posted a patch\n\n> I'm not saying this is wrong by any means, but it is a lot more than\n> just a simple change in the length of a char buffer.\n>\n\nWe die on config-lines that we fail to parse. Increasing the size of\nthe buffer is just playing cat and mouse. And besides, we can save\narbitrary large values. If we're able to write config files we're\nunable to parse, then we're violating the robustness principle.\n\nIMO, I think this is the right approach. If you disagree, feel free to\ncomplain when I submit the patch (after I get confirmation that this\nwas the culprit for Jeff, or some amount of time has passed).\n"}]}