{"thread":{"id":"31463","subject":"[PATCH] Prevent git-config from storing section keys that are too long","startedAt":"2012-09-07T00:47:09Z","lastAt":"2012-10-01T19:33:02Z","messageCount":9,"participants":["Ben Walton","Junio C Hamano","Michael Haggerty"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"198497","messageId":"1346978829-4486-1-git-send-email-bwalton@artsci.utoronto.ca","threadId":"31463","inReplyTo":null,"subject":"[PATCH] Prevent git-config from storing section keys that are too long","fromName":"Ben Walton","fromEmail":"bwalton@artsci.utoronto.ca","sentAt":"2012-09-07T00:47:09Z","receivedAt":"2012-09-07T00:47:09Z","isPatch":true,"sender":{"key":"bdwalton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/396061?v=4"},"body":"Key names have a length limit defined by MAXNAME in config.c.  When\nreading the config file, we reserve half of this limit for the section\nidentifier and the other half for the key name within that section.\n\nFor example, if setting a key named url.foo.insteadOf, url.foo may use\nat most half of MANXNAME.\n\nThe parser will throw an error if this condition is violated.\n\nThis patch ensures that git-config enforces the same restriction\nduring the creation of a section identifier so that it doesn't allow\nthe generate a configuration file that cannot be re-read later.\n\nThis patch also adds a test to t1303-wacky-config to catch any future\nissues with this check.\n\nSigned-off-by: Ben Walton <bwalton@artsci.utoronto.ca>\n---\n\nHi All,\n\nI happened to notice this while running the test suite in a deeply\nnested directory...\n\nThe check for baselen exceeding half of MAXNAME could be done earlier\nin the function but doing it late allowed the error message to be\nclearer without extra hassle.\n\nI also wonder if MAXNAME should be increased somewhat.  Section\nidentifiers generated from keys like:\n\nurl./some/really/long/path.insteadOf\n\ncould overrun the current limit.  It's not a common case, of course,\nor this issue would have been found sooner.  Would doubling the\ncurrent limit be out of the question?\n\nThanks\n-Ben\n\n\n\n config.c                |    8 ++++++++\n t/t1303-wacky-config.sh |    4 ++++\n 2 files changed, 12 insertions(+)\n\ndiff --git a/config.c b/config.c\nindex 2b706ea..d3f4854 100644\n--- a/config.c\n+++ b/config.c\n@@ -1276,6 +1276,14 @@ int git_config_parse_key(const char *key, char **store_key, int *baselen_)\n \t}\n \t(*store_key)[i] = 0;\n \n+\tif (baselen > MAXNAME / 2) {\n+\t\t/* ok to destroy this value now since it will be freed */\n+\t\t(*store_key)[baselen] = '\\0';\n+\t\terror(\"section identifier for key is too long (> %d): %s\",\n+\t\t      MAXNAME / 2, *store_key);\n+\t\tgoto out_free_ret_1;\n+\t}\n+\n \treturn 0;\n \n out_free_ret_1:\ndiff --git a/t/t1303-wacky-config.sh b/t/t1303-wacky-config.sh\nindex 46103a1..12f0850 100755\n--- a/t/t1303-wacky-config.sh\n+++ b/t/t1303-wacky-config.sh\n@@ -47,4 +47,8 @@ test_expect_success 'do not crash on special long config line' '\n \tcheck section.key \"$LONG_VALUE\"\n '\n \n+test_expect_success 'do not accept long section identifiers for key names' '\n+\ttest_must_fail git config some.REALLYlongREALLYlongREALLYlongREALLYlongREALLYlongREALLYlongREALLYlongREALLYlongREALLYlongREALLYlongREALLYlongREALLYlongREALLYlong.key value\n+'\n+\n test_done\n-- \n1.7.9.5\n"},{"id":"198499","messageId":"7vpq5yzkf3.fsf@alter.siamese.dyndns.org","threadId":"31463","inReplyTo":"1346978829-4486-1-git-send-email-bwalton@artsci.utoronto.ca","subject":"Re: [PATCH] Prevent git-config from storing section keys that are too long","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-07T01:33:20Z","receivedAt":"2012-09-07T01:33:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Walton <bwalton@artsci.utoronto.ca> writes:\n\n> Key names have a length limit defined by MAXNAME in config.c.  When\n> reading the config file, we reserve half of this limit for the section\n> identifier and the other half for the key name within that section.\n>\n> For example, if setting a key named url.foo.insteadOf, url.foo may use\n> at most half of MANXNAME.\n>\n> The parser will throw an error if this condition is violated.\n>\n> This patch ensures that git-config enforces the same restriction\n> during the creation of a section identifier so that it doesn't allow\n> the generate a configuration file that cannot be re-read later.\n>\n> This patch also adds a test to t1303-wacky-config to catch any future\n> issues with this check.\n>\n> Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>\n> ---\n>\n> Hi All,\n>\n> I happened to notice this while running the test suite in a deeply\n> nested directory...\n>\n> The check for baselen exceeding half of MAXNAME could be done earlier\n> in the function but doing it late allowed the error message to be\n> clearer without extra hassle.\n>\n> I also wonder if MAXNAME should be increased somewhat.  Section\n> identifiers generated from keys like:\n>\n> url./some/really/long/path.insteadOf\n>\n> could overrun the current limit.  It's not a common case, of course,\n> or this issue would have been found sooner.  Would doubling the\n> current limit be out of the question?\n\nIs there a reason to have _any_ limitation?  It is not like we store\nconfiguration data by allocating one file per item (in which case we\nmay be limited by the filesystem limit for direntry size), so if it\nis not too much trouble, I think the right thing to do is to lift\nthe limitation altogether, possibly using strbuf instead of a\nstatically sized array of characters.\n\nOf course, once you write a very long entry using a newer version of\nGit, the resulting configuration file may end up unusable by older\nversion of Git, so a patch to implement such a change may need to be\nbased on older maintenance release (say maint-1.7.9) and then merged\nupwards, but otherwise I do not offhand see a compatibility downside\nof such a change.\n"},{"id":"198500","messageId":"1346984829-sup-4477@pinkfloyd.chass.utoronto.ca","threadId":"31463","inReplyTo":"7vpq5yzkf3.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Prevent git-config from storing section keys that are too long","fromName":"Ben Walton","fromEmail":"bwalton@artsci.utoronto.ca","sentAt":"2012-09-07T02:34:21Z","receivedAt":"2012-09-07T02:34:21Z","isPatch":true,"sender":{"key":"bdwalton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/396061?v=4"},"body":"Excerpts from Junio C Hamano's message of Thu Sep 06 21:33:20 -0400 2012:\n\nHi Junio,\n\n> > identifiers generated from keys like:\n> >\n> > url./some/really/long/path.insteadOf\n> >\n> > could overrun the current limit.  It's not a common case, of course,\n> > or this issue would have been found sooner.  Would doubling the\n> > current limit be out of the question?\n> \n> Is there a reason to have _any_ limitation?  It is not like we store\n> configuration data by allocating one file per item (in which case we\n> may be limited by the filesystem limit for direntry size), so if it\n> is not too much trouble, I think the right thing to do is to lift\n> the limitation altogether, possibly using strbuf instead of a\n> statically sized array of characters.\n\nI thought it made sense to impose some sort of bound here but removing\nthe limit wouldn't encourage the use of ridiculously long names so\nlifting it entirely shouldn't hurt.\n\nAny chosen limit would always be somewhat arbitrary.  I had considered\nextending it to (PATHMAX + x) where x would also be arbitrary as\nthat would allow any valid url./path/max.insteadOf type setting but\nthat didn't seem like a good approach.\n\nRemoving the limit is a much better choice...\n\n> Of course, once you write a very long entry using a newer version of\n> Git, the resulting configuration file may end up unusable by older\n> version of Git, so a patch to implement such a change may need to be\n> based on older maintenance release (say maint-1.7.9) and then merged\n> upwards, but otherwise I do not offhand see a compatibility downside\n> of such a change.\n\nI'm ok with this approach and will put an altered patch together\nshortly.  I think it's fairly unlikely, but not impossible, that\nanyone creating a config file with long key names would be in a\nsituation where someone else couldn't read that same config file.\nI'll still base the change on maint-1.7.9 as suggested though.\n\nThanks\n-Ben\n--\nBen Walton\nSystems Programmer - CHASS\nUniversity of Toronto\nC:416.407.5610 | W:416.978.4302\n"},{"id":"200171","messageId":"1348913979-27431-1-git-send-email-bdwalton@gmail.com","threadId":"31463","inReplyTo":"7vpq5yzkf3.fsf@alter.siamese.dyndns.org","subject":"[PATCH] Remove the hard coded length limit on variable names in config files","fromName":"Ben Walton","fromEmail":"bdwalton@gmail.com","sentAt":"2012-09-29T10:19:39Z","receivedAt":"2012-09-29T10:19:39Z","isPatch":true,"sender":{"key":"bdwalton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/396061?v=4"},"body":"Previously while reading the variable names in config files, there was\na 256 character limit with at most 128 of those characters being used\nby the section header portion of the variable name.  This limitation\nwas only enforced while reading the config files.  It was possible to\nwrite a config file that was not subsequently readable.\n\nInstead of enforcing this limitation for both reading and writing,\nremove it entirely by changing the var member of the config_file\nstruct to a strbuf instead of a fixed length buffer.  Update all of\nthe parsing functions in config.c to use the strbuf instead of the\nstatic buffer.  Send the buf member of the strbuf to external callback\nfunctions to preserve the external api.\n\nSigned-off-by: Ben Walton <bdwalton@gmail.com>\n---\nHi Junio,\n\n(Sorry that this patch took so long to submit.  I've been busy moving.)\n\nI think this should remove the length limitations enforced while reading\nconfiguration file variable names.\n\nThanks\n-Ben\n\n config.c |   50 +++++++++++++++++++++++---------------------------\n 1 file changed, 23 insertions(+), 27 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 40f9c6d..ee860a7 100644\n--- a/config.c\n+++ b/config.c\n@@ -10,8 +10,6 @@\n #include \"strbuf.h\"\n #include \"quote.h\"\n \n-#define MAXNAME (256)\n-\n typedef struct config_file {\n \tstruct config_file *prev;\n \tFILE *f;\n@@ -19,7 +17,7 @@ typedef struct config_file {\n \tint linenr;\n \tint eof;\n \tstruct strbuf value;\n-\tchar var[MAXNAME];\n+\tstruct strbuf var;\n } config_file;\n \n static config_file *cf;\n@@ -191,7 +189,7 @@ static inline int iskeychar(int c)\n \treturn isalnum(c) || c == '-';\n }\n \n-static int get_value(config_fn_t fn, void *data, char *name, unsigned int len)\n+static int get_value(config_fn_t fn, void *data, struct strbuf *name)\n {\n \tint c;\n \tchar *value;\n@@ -203,11 +201,9 @@ static int get_value(config_fn_t fn, void *data, char *name, unsigned int len)\n \t\t\tbreak;\n \t\tif (!iskeychar(c))\n \t\t\tbreak;\n-\t\tname[len++] = tolower(c);\n-\t\tif (len >= MAXNAME)\n-\t\t\treturn -1;\n+\t\tstrbuf_addch(name, tolower(c));\n \t}\n-\tname[len] = 0;\n+\n \twhile (c == ' ' || c == '\\t')\n \t\tc = get_next_char();\n \n@@ -219,10 +215,10 @@ static int get_value(config_fn_t fn, void *data, char *name, unsigned int len)\n \t\tif (!value)\n \t\t\treturn -1;\n \t}\n-\treturn fn(name, value, data);\n+\treturn fn(name->buf, value, data);\n }\n \n-static int get_extended_base_var(char *name, int baselen, int c)\n+static int get_extended_base_var(struct strbuf *name, int c)\n {\n \tdo {\n \t\tif (c == '\\n')\n@@ -233,7 +229,7 @@ static int get_extended_base_var(char *name, int baselen, int c)\n \t/* We require the format to be '[base \"extension\"]' */\n \tif (c != '\"')\n \t\treturn -1;\n-\tname[baselen++] = '.';\n+\tstrbuf_addch(name, '.');\n \n \tfor (;;) {\n \t\tint c = get_next_char();\n@@ -246,34 +242,30 @@ static int get_extended_base_var(char *name, int baselen, int c)\n \t\t\tif (c == '\\n')\n \t\t\t\treturn -1;\n \t\t}\n-\t\tname[baselen++] = c;\n-\t\tif (baselen > MAXNAME / 2)\n-\t\t\treturn -1;\n+\t\tstrbuf_addch(name, c);\n \t}\n \n \t/* Final ']' */\n \tif (get_next_char() != ']')\n \t\treturn -1;\n-\treturn baselen;\n+\treturn name->len;\n }\n \n-static int get_base_var(char *name)\n+static int get_base_var(struct strbuf *name)\n {\n-\tint baselen = 0;\n+\tstrbuf_reset(name);\n \n \tfor (;;) {\n \t\tint c = get_next_char();\n \t\tif (cf->eof)\n \t\t\treturn -1;\n \t\tif (c == ']')\n-\t\t\treturn baselen;\n+\t\t\treturn name->len;\n \t\tif (isspace(c))\n-\t\t\treturn get_extended_base_var(name, baselen, c);\n+\t\t\treturn get_extended_base_var(name, c);\n \t\tif (!iskeychar(c) && c != '.')\n \t\t\treturn -1;\n-\t\tif (baselen > MAXNAME / 2)\n-\t\t\treturn -1;\n-\t\tname[baselen++] = tolower(c);\n+\t\tstrbuf_addch(name, tolower(c));\n \t}\n }\n \n@@ -281,7 +273,7 @@ static int git_parse_file(config_fn_t fn, void *data)\n {\n \tint comment = 0;\n \tint baselen = 0;\n-\tchar *var = cf->var;\n+\tstruct strbuf *var = &cf->var;\n \n \t/* U+FEFF Byte Order Mark in UTF8 */\n \tstatic const unsigned char *utf8_bom = (unsigned char *) \"\\xef\\xbb\\xbf\";\n@@ -320,14 +312,16 @@ static int git_parse_file(config_fn_t fn, void *data)\n \t\t\tbaselen = get_base_var(var);\n \t\t\tif (baselen <= 0)\n \t\t\t\tbreak;\n-\t\t\tvar[baselen++] = '.';\n-\t\t\tvar[baselen] = 0;\n+\t\t\tstrbuf_addch(var, '.');\n \t\t\tcontinue;\n \t\t}\n \t\tif (!isalpha(c))\n \t\t\tbreak;\n-\t\tvar[baselen] = tolower(c);\n-\t\tif (get_value(fn, data, var, baselen+1) < 0)\n+\t\t/* Truncate the var name back to the section header prior to\n+\t\t   grabbing the suffix part of the name and the value */\n+\t\tstrbuf_setlen(var, baselen+1);\n+\t\tstrbuf_addch(var, tolower(c));\n+\t\tif (get_value(fn, data, var) < 0)\n \t\t\tbreak;\n \t}\n \tdie(\"bad config file line %d in %s\", cf->linenr, cf->name);\n@@ -842,12 +836,14 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)\n \t\ttop.linenr = 1;\n \t\ttop.eof = 0;\n \t\tstrbuf_init(&top.value, 1024);\n+\t\tstrbuf_init(&top.var, 1024);\n \t\tcf = &top;\n \n \t\tret = git_parse_file(fn, data);\n \n \t\t/* pop config-file parsing state stack */\n \t\tstrbuf_release(&top.value);\n+\t\tstrbuf_release(&top.var);\n \t\tcf = top.prev;\n \n \t\tfclose(f);\n-- \n1.7.9.5\n"},{"id":"200190","messageId":"5067C51F.6020908@alum.mit.edu","threadId":"31463","inReplyTo":"1348913979-27431-1-git-send-email-bdwalton@gmail.com","subject":"Re: [PATCH] Remove the hard coded length limit on variable names in config files","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-09-30T04:05:51Z","receivedAt":"2012-09-30T04:05:51Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/29/2012 12:19 PM, Ben Walton wrote:\n> Previously while reading the variable names in config files, there was\n> a 256 character limit with at most 128 of those characters being used\n> by the section header portion of the variable name.  This limitation\n> was only enforced while reading the config files.  It was possible to\n> write a config file that was not subsequently readable.\n> \n> Instead of enforcing this limitation for both reading and writing,\n> remove it entirely by changing the var member of the config_file\n> struct to a strbuf instead of a fixed length buffer.  Update all of\n> the parsing functions in config.c to use the strbuf instead of the\n> static buffer.  Send the buf member of the strbuf to external callback\n> functions to preserve the external api.\n> \n> Signed-off-by: Ben Walton <bdwalton@gmail.com>\n> ---\n> Hi Junio,\n> \n> (Sorry that this patch took so long to submit.  I've been busy moving.)\n\nThe patch doesn't apply to the current master; it appears to have been\nbuilt against master 883a2a3504 (2012-02-23) or older.  It will have to\nbe rebased to the current master.\n\nNevertheless I will add a few comments below.\n\nOverall, I like your approach of using strbuf here, as it is simpler to\nuse and less error-prone.  It is also nice to get rid of an arbitrary\nlength limit, especially since it was not consistently enforced.\n\n> I think this should remove the length limitations enforced while reading\n> configuration file variable names.\n> \n> Thanks\n> -Ben\n> \n>  config.c |   50 +++++++++++++++++++++++---------------------------\n>  1 file changed, 23 insertions(+), 27 deletions(-)\n> \n> diff --git a/config.c b/config.c\n> index 40f9c6d..ee860a7 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -10,8 +10,6 @@\n>  #include \"strbuf.h\"\n>  #include \"quote.h\"\n>  \n> -#define MAXNAME (256)\n> -\n>  typedef struct config_file {\n>  \tstruct config_file *prev;\n>  \tFILE *f;\n> @@ -19,7 +17,7 @@ typedef struct config_file {\n>  \tint linenr;\n>  \tint eof;\n>  \tstruct strbuf value;\n> -\tchar var[MAXNAME];\n> +\tstruct strbuf var;\n>  } config_file;\n>  \n>  static config_file *cf;\n> @@ -191,7 +189,7 @@ static inline int iskeychar(int c)\n>  \treturn isalnum(c) || c == '-';\n>  }\n>  \n> -static int get_value(config_fn_t fn, void *data, char *name, unsigned int len)\n> +static int get_value(config_fn_t fn, void *data, struct strbuf *name)\n>  {\n>  \tint c;\n>  \tchar *value;\n> @@ -203,11 +201,9 @@ static int get_value(config_fn_t fn, void *data, char *name, unsigned int len)\n>  \t\t\tbreak;\n>  \t\tif (!iskeychar(c))\n>  \t\t\tbreak;\n> -\t\tname[len++] = tolower(c);\n> -\t\tif (len >= MAXNAME)\n> -\t\t\treturn -1;\n> +\t\tstrbuf_addch(name, tolower(c));\n>  \t}\n> -\tname[len] = 0;\n> +\n>  \twhile (c == ' ' || c == '\\t')\n>  \t\tc = get_next_char();\n>  \n> @@ -219,10 +215,10 @@ static int get_value(config_fn_t fn, void *data, char *name, unsigned int len)\n>  \t\tif (!value)\n>  \t\t\treturn -1;\n>  \t}\n> -\treturn fn(name, value, data);\n> +\treturn fn(name->buf, value, data);\n>  }\n>  \n> -static int get_extended_base_var(char *name, int baselen, int c)\n> +static int get_extended_base_var(struct strbuf *name, int c)\n>  {\n>  \tdo {\n>  \t\tif (c == '\\n')\n> @@ -233,7 +229,7 @@ static int get_extended_base_var(char *name, int baselen, int c)\n>  \t/* We require the format to be '[base \"extension\"]' */\n>  \tif (c != '\"')\n>  \t\treturn -1;\n> -\tname[baselen++] = '.';\n> +\tstrbuf_addch(name, '.');\n>  \n>  \tfor (;;) {\n>  \t\tint c = get_next_char();\n> @@ -246,34 +242,30 @@ static int get_extended_base_var(char *name, int baselen, int c)\n>  \t\t\tif (c == '\\n')\n>  \t\t\t\treturn -1;\n>  \t\t}\n> -\t\tname[baselen++] = c;\n> -\t\tif (baselen > MAXNAME / 2)\n> -\t\t\treturn -1;\n> +\t\tstrbuf_addch(name, c);\n>  \t}\n>  \n>  \t/* Final ']' */\n>  \tif (get_next_char() != ']')\n>  \t\treturn -1;\n> -\treturn baselen;\n> +\treturn name->len;\n>  }\n>  \n> -static int get_base_var(char *name)\n> +static int get_base_var(struct strbuf *name)\n>  {\n> -\tint baselen = 0;\n> +\tstrbuf_reset(name);\n>  \n>  \tfor (;;) {\n>  \t\tint c = get_next_char();\n>  \t\tif (cf->eof)\n>  \t\t\treturn -1;\n>  \t\tif (c == ']')\n> -\t\t\treturn baselen;\n> +\t\t\treturn name->len;\n>  \t\tif (isspace(c))\n> -\t\t\treturn get_extended_base_var(name, baselen, c);\n> +\t\t\treturn get_extended_base_var(name, c);\n>  \t\tif (!iskeychar(c) && c != '.')\n>  \t\t\treturn -1;\n> -\t\tif (baselen > MAXNAME / 2)\n> -\t\t\treturn -1;\n> -\t\tname[baselen++] = tolower(c);\n> +\t\tstrbuf_addch(name, tolower(c));\n>  \t}\n>  }\n>  \n> @@ -281,7 +273,7 @@ static int git_parse_file(config_fn_t fn, void *data)\n>  {\n>  \tint comment = 0;\n>  \tint baselen = 0;\n> -\tchar *var = cf->var;\n> +\tstruct strbuf *var = &cf->var;\n>  \n>  \t/* U+FEFF Byte Order Mark in UTF8 */\n>  \tstatic const unsigned char *utf8_bom = (unsigned char *) \"\\xef\\xbb\\xbf\";\n> @@ -320,14 +312,16 @@ static int git_parse_file(config_fn_t fn, void *data)\n>  \t\t\tbaselen = get_base_var(var);\n>  \t\t\tif (baselen <= 0)\n>  \t\t\t\tbreak;\n> -\t\t\tvar[baselen++] = '.';\n> -\t\t\tvar[baselen] = 0;\n> +\t\t\tstrbuf_addch(var, '.');\n>  \t\t\tcontinue;\n>  \t\t}\n>  \t\tif (!isalpha(c))\n>  \t\t\tbreak;\n> -\t\tvar[baselen] = tolower(c);\n> -\t\tif (get_value(fn, data, var, baselen+1) < 0)\n> +\t\t/* Truncate the var name back to the section header prior to\n> +\t\t   grabbing the suffix part of the name and the value */\n> +\t\tstrbuf_setlen(var, baselen+1);\n> +\t\tstrbuf_addch(var, tolower(c));\n> +\t\tif (get_value(fn, data, var) < 0)\n>  \t\t\tbreak;\n>  \t}\n>  \tdie(\"bad config file line %d in %s\", cf->linenr, cf->name);\n\nThe preferred format for multiline comments in the git project is\n\n    /*\n     * Truncate the var name back to the section header prior to\n     * grabbing the suffix part of the name and the value.\n     */\n\nIt took me a while to figure out what you were doing here.  Let me\nexplain why.\n\nIn the old code, get_base_var() read the string into var and returned\nvar's length (or -1 on error).  The fact that the length of var was\nfirst \"reset\" to zero is somewhat implicit in the fact that no length\nparameter is being passed to get_base_var().\n\nBut in the new version, get_base_var() is passed a strbuf.  Often,\noperations with strbufs append to the strbuf, and this is what I first\nassumed.  It took me a while to realize that get_base_var() calls\nstrbuf_reset() before getting to work.  Moreover, get_base_var() still\nreturns the length of what it found, which is redundant with a strbuf\nand therefore unexpected.  So when the return value of get_base_var() is\nstored into baselen, it is not really obvious that it is the string's\nlength.\n\nTherefore, I suggest\n\n* Call strbuf_reset() directly in get_parse_file() rather than in\nget_base_var()\n\n* Change get_base_var() to return 0 on success (rather than the length\nof the string) and -1 on error (including length==0, which is also an\nerror in this context).\n\n* Change how get_parse_file() initializes baselen to\n\n        if (get_base_var(var) < 0)\n                break;\n        strbuf_addch(var, '.');\n        baselen = var->len;\n\nNote that baselen now includes the trailing dot.  Then later, you don't\nneed the \"+1\":\n\n        /*\n         * Truncate the var name back to (section header plus '.')\n         * prior to grabbing the suffix part of the name and the value\n         */\n        strbuf_setlen(var, baselen);\n        strbuf_addch(var, tolower(c));\n        if (get_value(fn, data, var) < 0)\n        [...]\n\n> @@ -842,12 +836,14 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)\n>  \t\ttop.linenr = 1;\n>  \t\ttop.eof = 0;\n>  \t\tstrbuf_init(&top.value, 1024);\n> +\t\tstrbuf_init(&top.var, 1024);\n>  \t\tcf = &top;\n>  \n>  \t\tret = git_parse_file(fn, data);\n>  \n>  \t\t/* pop config-file parsing state stack */\n>  \t\tstrbuf_release(&top.value);\n> +\t\tstrbuf_release(&top.var);\n>  \t\tcf = top.prev;\n>  \n>  \t\tfclose(f);\n> \n\nFinally, I realize that the MAXNAME constant was not exported and I\ncan't find the old length limits documented anywhere, but I nevertheless\nworry a little bit that one of the users of the config API has a\nbuilt-in assumption that names can never be longer than 256 characters\n(for example, a config_fn_t function might try to store the name into a\nfixed-length buffer).  Hopefully such code would never have been written\nor accepted, but...?  If you have thought about this or audited the\ncallers, please mention that in your commit message.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"200218","messageId":"CAP30j15wKa7wbLyaLo8omHcAh5u7O=Yt8Tjy4bD3V_0nT1zJPQ@mail.gmail.com","threadId":"31463","inReplyTo":"5067C51F.6020908@alum.mit.edu","subject":"Re: [PATCH] Remove the hard coded length limit on variable names in config files","fromName":"Ben Walton","fromEmail":"bdwalton@gmail.com","sentAt":"2012-09-30T18:20:07Z","receivedAt":"2012-09-30T18:20:07Z","isPatch":true,"sender":{"key":"bdwalton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/396061?v=4"},"body":"Hi Michael,\n\n> The patch doesn't apply to the current master; it appears to have been\n> built against master 883a2a3504 (2012-02-23) or older.  It will have to\n> be rebased to the current master.\n\nJunio had asked that it be based on maint so that's what I (thought\nI?) did.  I'm happy to redo it against master if that's better though.\n\n> The preferred format for multiline comments in the git project is\n>\n>     /*\n>      * Truncate the var name back to the section header prior to\n>      * grabbing the suffix part of the name and the value.\n>      */\n\nOops; Will fix.\n\n> In the old code, get_base_var() read the string into var and returned\n> var's length (or -1 on error).  The fact that the length of var was\n> first \"reset\" to zero is somewhat implicit in the fact that no length\n> parameter is being passed to get_base_var().\n>\n> But in the new version, get_base_var() is passed a strbuf.  Often,\n> operations with strbufs append to the strbuf, and this is what I first\n> assumed.  It took me a while to realize that get_base_var() calls\n> strbuf_reset() before getting to work.  Moreover, get_base_var() still\n> returns the length of what it found, which is redundant with a strbuf\n> and therefore unexpected.  So when the return value of get_base_var() is\n> stored into baselen, it is not really obvious that it is the string's\n> length.\n\nOk, that's a fair criticism.  When I was creating the patch, I thought\nthat placing\nthe strbuf_reset in get_base_var() seemed nicer as it matched the\nbaselen = 0 which\neffectively reset the old character array.  Your point is well taken\nthough and I think\nit makes sense to switch things around the way you've suggested.\n\n> Finally, I realize that the MAXNAME constant was not exported and I\n> can't find the old length limits documented anywhere, but I nevertheless\n> worry a little bit that one of the users of the config API has a\n> built-in assumption that names can never be longer than 256 characters\n> (for example, a config_fn_t function might try to store the name into a\n> fixed-length buffer).  Hopefully such code would never have been written\n> or accepted, but...?  If you have thought about this or audited the\n> callers, please mention that in your commit message.\n\nI did look through the code to see if anything was relying on fixed\nsize buffers and I didn't see anything.  I'll update the commit\nmessage with this info too.\n\nI'll send a modified patch shortly.\n\nThanks for the review!\n-Ben\n-- \n---------------------------------------------------------------------------------------------------------------------------\nTake the risk of thinking for yourself.  Much more happiness,\ntruth, beauty and wisdom will come to you that way.\n\n-Christopher Hitchens\n---------------------------------------------------------------------------------------------------------------------------\n"},{"id":"200221","messageId":"1349034276-10690-1-git-send-email-bdwalton@gmail.com","threadId":"31463","inReplyTo":"CAP30j15wKa7wbLyaLo8omHcAh5u7O=Yt8Tjy4bD3V_0nT1zJPQ@mail.gmail.com","subject":"[PATCH] Remove the hard coded length limit on variable names in config files","fromName":"Ben Walton","fromEmail":"bdwalton@gmail.com","sentAt":"2012-09-30T19:44:36Z","receivedAt":"2012-09-30T19:44:36Z","isPatch":true,"sender":{"key":"bdwalton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/396061?v=4"},"body":"Previously while reading the variable names in config files, there was\na 256 character limit with at most 128 of those characters being used\nby the section header portion of the variable name.  This limitation\nwas only enforced while reading the config files.  It was possible to\nwrite a config file that was not subsequently readable.\n\nInstead of enforcing this limitation for both reading and writing,\nremove it entirely by changing the var member of the config_file\nstruct to a strbuf instead of a fixed length buffer.  Update all of\nthe parsing functions in config.c to use the strbuf instead of the\nstatic buffer.\n\nThe parsing functions that returned the base length of the variable\nname now return simply 0 for success and -1 for failure.  The base\nlength information is obtained through the strbuf's len member.\n\nWe now send the buf member of the strbuf to external callback\nfunctions to preserve the external api.  None of the external callers\nrely on the old size limitation for sizing their own buffers so\nremoving the limit should have no externally visible effect.\n\nSigned-off-by: Ben Walton <bdwalton@gmail.com>\n---\n config.c |   59 +++++++++++++++++++++++++++++------------------------------\n 1 file changed, 29 insertions(+), 30 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 08e47e2..24fb2d2 100644\n--- a/config.c\n+++ b/config.c\n@@ -10,8 +10,6 @@\n #include \"strbuf.h\"\n #include \"quote.h\"\n \n-#define MAXNAME (256)\n-\n typedef struct config_file {\n \tstruct config_file *prev;\n \tFILE *f;\n@@ -19,7 +17,7 @@ typedef struct config_file {\n \tint linenr;\n \tint eof;\n \tstruct strbuf value;\n-\tchar var[MAXNAME];\n+\tstruct strbuf var;\n } config_file;\n \n static config_file *cf;\n@@ -260,7 +258,7 @@ static inline int iskeychar(int c)\n \treturn isalnum(c) || c == '-';\n }\n \n-static int get_value(config_fn_t fn, void *data, char *name, unsigned int len)\n+static int get_value(config_fn_t fn, void *data, struct strbuf *name)\n {\n \tint c;\n \tchar *value;\n@@ -272,11 +270,9 @@ static int get_value(config_fn_t fn, void *data, char *name, unsigned int len)\n \t\t\tbreak;\n \t\tif (!iskeychar(c))\n \t\t\tbreak;\n-\t\tname[len++] = tolower(c);\n-\t\tif (len >= MAXNAME)\n-\t\t\treturn -1;\n+\t\tstrbuf_addch(name, tolower(c));\n \t}\n-\tname[len] = 0;\n+\n \twhile (c == ' ' || c == '\\t')\n \t\tc = get_next_char();\n \n@@ -288,10 +284,10 @@ static int get_value(config_fn_t fn, void *data, char *name, unsigned int len)\n \t\tif (!value)\n \t\t\treturn -1;\n \t}\n-\treturn fn(name, value, data);\n+\treturn fn(name->buf, value, data);\n }\n \n-static int get_extended_base_var(char *name, int baselen, int c)\n+static int get_extended_base_var(struct strbuf *name, int c)\n {\n \tdo {\n \t\tif (c == '\\n')\n@@ -302,7 +298,7 @@ static int get_extended_base_var(char *name, int baselen, int c)\n \t/* We require the format to be '[base \"extension\"]' */\n \tif (c != '\"')\n \t\treturn -1;\n-\tname[baselen++] = '.';\n+\tstrbuf_addch(name, '.');\n \n \tfor (;;) {\n \t\tint c = get_next_char();\n@@ -315,37 +311,31 @@ static int get_extended_base_var(char *name, int baselen, int c)\n \t\t\tif (c == '\\n')\n \t\t\t\tgoto error_incomplete_line;\n \t\t}\n-\t\tname[baselen++] = c;\n-\t\tif (baselen > MAXNAME / 2)\n-\t\t\treturn -1;\n+\t\tstrbuf_addch(name, c);\n \t}\n \n \t/* Final ']' */\n \tif (get_next_char() != ']')\n \t\treturn -1;\n-\treturn baselen;\n+\treturn 0;\n error_incomplete_line:\n \tcf->linenr--;\n \treturn -1;\n }\n \n-static int get_base_var(char *name)\n+static int get_base_var(struct strbuf *name)\n {\n-\tint baselen = 0;\n-\n \tfor (;;) {\n \t\tint c = get_next_char();\n \t\tif (cf->eof)\n \t\t\treturn -1;\n \t\tif (c == ']')\n-\t\t\treturn baselen;\n+\t\t\treturn 0;\n \t\tif (isspace(c))\n-\t\t\treturn get_extended_base_var(name, baselen, c);\n+\t\t\treturn get_extended_base_var(name, c);\n \t\tif (!iskeychar(c) && c != '.')\n \t\t\treturn -1;\n-\t\tif (baselen > MAXNAME / 2)\n-\t\t\treturn -1;\n-\t\tname[baselen++] = tolower(c);\n+\t\tstrbuf_addch(name, tolower(c));\n \t}\n }\n \n@@ -353,7 +343,7 @@ static int git_parse_file(config_fn_t fn, void *data)\n {\n \tint comment = 0;\n \tint baselen = 0;\n-\tchar *var = cf->var;\n+\tstruct strbuf *var = &cf->var;\n \n \t/* U+FEFF Byte Order Mark in UTF8 */\n \tstatic const unsigned char *utf8_bom = (unsigned char *) \"\\xef\\xbb\\xbf\";\n@@ -389,17 +379,24 @@ static int git_parse_file(config_fn_t fn, void *data)\n \t\t\tcontinue;\n \t\t}\n \t\tif (c == '[') {\n-\t\t\tbaselen = get_base_var(var);\n-\t\t\tif (baselen <= 0)\n+\t\t\t/* Reset prior to determining a new stem */\n+\t\t\tstrbuf_reset(var);\n+\t\t\tif (get_base_var(var) < 0 || var->len < 1)\n \t\t\t\tbreak;\n-\t\t\tvar[baselen++] = '.';\n-\t\t\tvar[baselen] = 0;\n+\t\t\tstrbuf_addch(var, '.');\n+\t\t\tbaselen = var -> len;\n \t\t\tcontinue;\n \t\t}\n \t\tif (!isalpha(c))\n \t\t\tbreak;\n-\t\tvar[baselen] = tolower(c);\n-\t\tif (get_value(fn, data, var, baselen+1) < 0)\n+\t\t/*\n+\t\t * Truncate the var name back to the section header\n+\t\t * stem prior to grabbing the suffix part of the name\n+\t\t * and the value.\n+\t\t */\n+\t\tstrbuf_setlen(var, baselen);\n+\t\tstrbuf_addch(var, tolower(c));\n+\t\tif (get_value(fn, data, var) < 0)\n \t\t\tbreak;\n \t}\n \tdie(\"bad config file line %d in %s\", cf->linenr, cf->name);\n@@ -899,12 +896,14 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)\n \t\ttop.linenr = 1;\n \t\ttop.eof = 0;\n \t\tstrbuf_init(&top.value, 1024);\n+\t\tstrbuf_init(&top.var, 1024);\n \t\tcf = &top;\n \n \t\tret = git_parse_file(fn, data);\n \n \t\t/* pop config-file parsing state stack */\n \t\tstrbuf_release(&top.value);\n+\t\tstrbuf_release(&top.var);\n \t\tcf = top.prev;\n \n \t\tfclose(f);\n-- \n1.7.9.5\n"},{"id":"200235","messageId":"50690B28.5030408@alum.mit.edu","threadId":"31463","inReplyTo":"CAP30j15wKa7wbLyaLo8omHcAh5u7O=Yt8Tjy4bD3V_0nT1zJPQ@mail.gmail.com","subject":"Re: [PATCH] Remove the hard coded length limit on variable names in config files","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-10-01T03:16:56Z","receivedAt":"2012-10-01T03:16:56Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/30/2012 08:20 PM, Ben Walton wrote:\n>> The patch doesn't apply to the current master; it appears to have been\n>> built against master 883a2a3504 (2012-02-23) or older.  It will have to\n>> be rebased to the current master.\n> \n> Junio had asked that it be based on maint so that's what I (thought\n> I?) did.  I'm happy to redo it against master if that's better though.\n\nThat explains it.  Sorry, I hadn't seen that conversation.  (In the\nfuture, if a patch applies against something other than master, please\nadd a note in the cover letter or in the patch comment after the \"--\".)\n\nIt is OK with me to leave the patch against maint, if that is what Junio\nwanted.  It would help, though, if you rebase it to the latest maint\n(the conflict seems easy to fix).\n\nThanks,\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"200285","messageId":"7vvceuasy9.fsf@alter.siamese.dyndns.org","threadId":"31463","inReplyTo":"1349034276-10690-1-git-send-email-bdwalton@gmail.com","subject":"Re: [PATCH] Remove the hard coded length limit on variable names in config files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-10-01T19:33:02Z","receivedAt":"2012-10-01T19:33:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Walton <bdwalton@gmail.com> writes:\n\n> Previously while reading the variable names in config files, there was\n> a 256 character limit with at most 128 of those characters being used\n> by the section header portion of the variable name.  This limitation\n> was only enforced while reading the config files.  It was possible to\n> write a config file that was not subsequently readable.\n>\n> Instead of enforcing this limitation for both reading and writing,\n> remove it entirely by changing the var member of the config_file\n> struct to a strbuf instead of a fixed length buffer.  Update all of\n> the parsing functions in config.c to use the strbuf instead of the\n> static buffer.\n>\n> The parsing functions that returned the base length of the variable\n> name now return simply 0 for success and -1 for failure.  The base\n> length information is obtained through the strbuf's len member.\n>\n> We now send the buf member of the strbuf to external callback\n> functions to preserve the external api.  None of the external callers\n> rely on the old size limitation for sizing their own buffers so\n> removing the limit should have no externally visible effect.\n>\n> Signed-off-by: Ben Walton <bdwalton@gmail.com>\n> ---\n>  config.c |   59 +++++++++++++++++++++++++++++------------------------------\n>  1 file changed, 29 insertions(+), 30 deletions(-)\n\nMakes sense, and I found the patch very readable.\n\nThanks, both.\n"}]}