Value size limits on git config files

Value size limits on git config files

[Jeff Adamson]

When trying to incorporate large aliases into my ~/.gitconfig file via
`git config --global "!$(cat myscript)"`, I determined that there
appears to be a limit to the size of a value.  When exceeded, git
prints out "fatal: bad config file line 82 in ~/.gitconfig".  I was
able to strip enough comments and such from myscript that it then no
longer invalidated the config once the value was less than 1024 chars.
 (Minor aside, I am doing this as a small experiment to potentially
aid in synchronizing all my git behaviors across machines by inlining
them into the config file.  Whether this is a good path to explore or
not is a separate issue.)

My questions are:
   Is this 1024 char limit on config file values documented somewhere?
   Should it be considered a bug that git config can write out config
files which it can not then read (e.g. it should be changed to either
enforce the value-length limit during writing, or the parser updated
to handle large values)?


Thanks,
Jeff

Re: Value size limits on git config files

[Erik Faye-Lund]

On Tue, Apr 5, 2011 at 6:29 PM, Jeff Adamson <jwa@urbancode.com> wrote:
> When trying to incorporate large aliases into my ~/.gitconfig file via
> `git config --global "!$(cat myscript)"`, I determined that there
> appears to be a limit to the size of a value.  When exceeded, git
> prints out "fatal: bad config file line 82 in ~/.gitconfig".  I was
> able to strip enough comments and such from myscript that it then no
> longer invalidated the config once the value was less than 1024 chars.
>  (Minor aside, I am doing this as a small experiment to potentially
> aid in synchronizing all my git behaviors across machines by inlining
> them into the config file.  Whether this is a good path to explore or
> not is a separate issue.)
>
> My questions are:
>    Is this 1024 char limit on config file values documented somewhere?
>    Should it be considered a bug that git config can write out config
> files which it can not then read (e.g. it should be changed to either
> enforce the value-length limit during writing, or the parser updated
> to handle large values)?
>

It's due to use of a fixed-size buffer. This patch fixes it for me:

diff --git a/config.c b/config.c
index 0abcada..bc6ea49 100644
--- a/config.c
+++ b/config.c
@@ -133,23 +133,20 @@ static int get_next_char(void)

 static char *parse_value(void)
 {
-	static char value[1024];
-	int quote = 0, comment = 0, len = 0, space = 0;
+	struct strbuf value = STRBUF_INIT;
+	int quote = 0, comment = 0, space = 0;

 	for (;;) {
 		int c = get_next_char();
-		if (len >= sizeof(value) - 1)
-			return NULL;
 		if (c == '\n') {
 			if (quote)
 				return NULL;
-			value[len] = 0;
-			return value;
+			return strbuf_detach(&value, NULL);
 		}
 		if (comment)
 			continue;
 		if (isspace(c) && !quote) {
-			if (len)
+			if (value.len)
 				space++;
 			continue;
 		}
@@ -160,7 +157,7 @@ static char *parse_value(void)
 			}
 		}
 		for (; space; space--)
-			value[len++] = ' ';
+			strbuf_addch(&value, ' ');
 		if (c == '\\') {
 			c = get_next_char();
 			switch (c) {
@@ -180,16 +177,17 @@ static char *parse_value(void)
 				break;
 			/* Reject unknown escape sequences */
 			default:
+				strbuf_release(&value);
 				return NULL;
 			}
-			value[len++] = c;
+			strbuf_addch(&value, c);
 			continue;
 		}
 		if (c == '"') {
 			quote = 1-quote;
 			continue;
 		}
-		value[len++] = c;
+		strbuf_addch(&value, c);
 	}
 }

@@ -200,7 +198,7 @@ static inline int iskeychar(int c)

 static int get_value(config_fn_t fn, void *data, char *name, unsigned int len)
 {
-	int c;
+	int c, ret;
 	char *value;

 	/* Get the full name */
@@ -226,7 +224,9 @@ static int get_value(config_fn_t fn, void *data,
char *name, unsigned int len)
 		if (!value)
 			return -1;
 	}
-	return fn(name, value, data);
+	ret = fn(name, value, data);
+	free(value);
+	return ret;
 }

 static int get_extended_base_var(char *name, int baselen, int c)

Re: Value size limits on git config files

[Drew Northup]

On Tue, 2011-04-05 at 19:01 +0200, Erik Faye-Lund wrote:
> On Tue, Apr 5, 2011 at 6:29 PM, Jeff Adamson <jwa@urbancode.com> wrote:
> I was
> > able to strip enough comments and such from myscript that it then no
> > longer invalidated the config once the value was less than 1024 chars.

> It's due to use of a fixed-size buffer. This patch fixes it for me:
> 
> diff --git a/config.c b/config.c
> index 0abcada..bc6ea49 100644
> --- a/config.c
> +++ b/config.c
> @@ -133,23 +133,20 @@ static int get_next_char(void)
> 
>  static char *parse_value(void)
>  {
> -	static char value[1024];
> -	int quote = 0, comment = 0, len = 0, space = 0;
> +	struct strbuf value = STRBUF_INIT;
> +	int quote = 0, comment = 0, space = 0;

Eric,
You're doing a lot more here than just making a simple char buffer
larger...

>  	for (;;) {
>  		int c = get_next_char();
> -		if (len >= sizeof(value) - 1)
> -			return NULL;
>  		if (c == '\n') {
>  			if (quote)
>  				return NULL;
> -			value[len] = 0;
> -			return value;
> +			return strbuf_detach(&value, NULL);

...ditto...

>  		}
>  		if (comment)
>  			continue;
>  		if (isspace(c) && !quote) {
> -			if (len)
> +			if (value.len)

...ditto...

>  				space++;
>  			continue;
>  		}
> @@ -160,7 +157,7 @@ static char *parse_value(void)
>  			}
>  		}
>  		for (; space; space--)
> -			value[len++] = ' ';
> +			strbuf_addch(&value, ' ');

...ditto...

(The rest cut for discussion...)

The the first question that needs to be asked is: Is there a reason why
it was still only 1kiB long?
The second is why adopt the struct here and not use an expanded char[]
element?

I'm not saying this is wrong by any means, but it is a lot more than
just a simple change in the length of a char buffer.

-- 
-Drew Northup
________________________________________________
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59

Re: Value size limits on git config files

[Erik Faye-Lund]

On Tue, Apr 5, 2011 at 7:35 PM, Drew Northup <drew.northup@maine.edu> wrote:
>
> On Tue, 2011-04-05 at 19:01 +0200, Erik Faye-Lund wrote:
>> On Tue, Apr 5, 2011 at 6:29 PM, Jeff Adamson <jwa@urbancode.com> wrote:
>> I was
>> > able to strip enough comments and such from myscript that it then no
>> > longer invalidated the config once the value was less than 1024 chars.
>
>> It's due to use of a fixed-size buffer. This patch fixes it for me:
>>
>> diff --git a/config.c b/config.c
>> index 0abcada..bc6ea49 100644
>> --- a/config.c
>> +++ b/config.c
>> @@ -133,23 +133,20 @@ static int get_next_char(void)
>>
>>  static char *parse_value(void)
>>  {
>> -     static char value[1024];
>> -     int quote = 0, comment = 0, len = 0, space = 0;
>> +     struct strbuf value = STRBUF_INIT;
>> +     int quote = 0, comment = 0, space = 0;
>
> Eric,
> You're doing a lot more here than just making a simple char buffer
> larger...
>

I'm not quite sure why you're telling me this. After all, I wrote the
patch - of course I know that.

>>       for (;;) {
>>               int c = get_next_char();
>> -             if (len >= sizeof(value) - 1)
>> -                     return NULL;
>>               if (c == '\n') {
>>                       if (quote)
>>                               return NULL;
>> -                     value[len] = 0;
>> -                     return value;
>> +                     return strbuf_detach(&value, NULL);
>
> ...ditto...
>
>>               }
>>               if (comment)
>>                       continue;
>>               if (isspace(c) && !quote) {
>> -                     if (len)
>> +                     if (value.len)
>
> ...ditto...
>
>>                               space++;
>>                       continue;
>>               }
>> @@ -160,7 +157,7 @@ static char *parse_value(void)
>>                       }
>>               }
>>               for (; space; space--)
>> -                     value[len++] = ' ';
>> +                     strbuf_addch(&value, ' ');
>
> ...ditto...
>
> (The rest cut for discussion...)
>
> The the first question that needs to be asked is: Is there a reason why
> it was still only 1kiB long?
> The second is why adopt the struct here and not use an expanded char[]
> element?
>

Yeah, and I don't know the answer to those questions.

But I do know how to fix the problem, so I posted a patch

> I'm not saying this is wrong by any means, but it is a lot more than
> just a simple change in the length of a char buffer.
>

We die on config-lines that we fail to parse. Increasing the size of
the buffer is just playing cat and mouse. And besides, we can save
arbitrary large values. If we're able to write config files we're
unable to parse, then we're violating the robustness principle.

IMO, I think this is the right approach. If you disagree, feel free to
complain when I submit the patch (after I get confirmation that this
was the culprit for Jeff, or some amount of time has passed).