Re: Value size limits on git config files

[Erik Faye-Lund <kusmabite@gmail.com>]

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).