[PATCH] config: support values longer than 1024 bytes

[PATCH] config: support values longer than 1024 bytes

[Erik Faye-Lund]

parse_value in config.c has a static buffer of 1024 bytes that it
parse the value into. This can sometimes be a problem when a
config file contains very long values.

It's particularly amusing that git-config already is able to write
such files, so it should probably be able to read them as well.

Fix this by using a strbuf instead of a static buffer.

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---

Here's a proper-ish submission with a commit message and all.

The rather awkward return statement with strdup("") is because
strbuf_detach returns NULL when there's nothing allocated. Even
worse, it returns an uninitialized string if the string has been
initialized with a non-zero 'hint'.

Perhaps I should change it to return a heap-allocated, empty
string in those cases instead of working around it here?

 config.c                |   26 ++++++++++++++------------
 t/t1303-wacky-config.sh |    2 +-
 2 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/config.c b/config.c
index 0abcada..f882f7c 100644
--- a/config.c
+++ b/config.c
@@ -133,23 +133,22 @@ 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 value.len ?
+			    strbuf_detach(&value, NULL) :
+			    strdup("");
 		}
 		if (comment)
 			continue;
 		if (isspace(c) && !quote) {
-			if (len)
+			if (value.len)
 				space++;
 			continue;
 		}
@@ -160,7 +159,7 @@ static char *parse_value(void)
 			}
 		}
 		for (; space; space--)
-			value[len++] = ' ';
+			strbuf_addch(&value, ' ');
 		if (c == '\\') {
 			c = get_next_char();
 			switch (c) {
@@ -180,16 +179,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 +200,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 +226,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)
diff --git a/t/t1303-wacky-config.sh b/t/t1303-wacky-config.sh
index 080117c..46103a1 100755
--- a/t/t1303-wacky-config.sh
+++ b/t/t1303-wacky-config.sh
@@ -44,7 +44,7 @@ LONG_VALUE=$(printf "x%01021dx a" 7)
 test_expect_success 'do not crash on special long config line' '
 	setup &&
 	git config section.key "$LONG_VALUE" &&
-	check section.key "fatal: bad config file line 2 in .git/config"
+	check section.key "$LONG_VALUE"
 '
 
 test_done
-- 
1.7.4.msysgit.0.168.g33778

Re: [PATCH] config: support values longer than 1024 bytes

[Erik Faye-Lund]

Ugh, I should learn not to send out patches way past my bed-time. This
should have been marked as RFC, and the subject should probably say
"1023 bytes", since one byte was needed for zero-termination.

Sorry for the noise.

On Wed, Apr 6, 2011 at 1:30 AM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
> parse_value in config.c has a static buffer of 1024 bytes that it
> parse the value into. This can sometimes be a problem when a
> config file contains very long values.
>
> It's particularly amusing that git-config already is able to write
> such files, so it should probably be able to read them as well.
>
> Fix this by using a strbuf instead of a static buffer.
>
> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
> ---
>
> Here's a proper-ish submission with a commit message and all.
>
> The rather awkward return statement with strdup("") is because
> strbuf_detach returns NULL when there's nothing allocated. Even
> worse, it returns an uninitialized string if the string has been
> initialized with a non-zero 'hint'.
>
> Perhaps I should change it to return a heap-allocated, empty
> string in those cases instead of working around it here?
>
>  config.c                |   26 ++++++++++++++------------
>  t/t1303-wacky-config.sh |    2 +-
>  2 files changed, 15 insertions(+), 13 deletions(-)
>
> diff --git a/config.c b/config.c
> index 0abcada..f882f7c 100644
> --- a/config.c
> +++ b/config.c
> @@ -133,23 +133,22 @@ 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 value.len ?
> +                           strbuf_detach(&value, NULL) :
> +                           strdup("");

Should have used xstrdup()...

>                }
>                if (comment)
>                        continue;
>                if (isspace(c) && !quote) {
> -                       if (len)
> +                       if (value.len)
>                                space++;
>                        continue;
>                }
> @@ -160,7 +159,7 @@ static char *parse_value(void)
>                        }
>                }
>                for (; space; space--)
> -                       value[len++] = ' ';
> +                       strbuf_addch(&value, ' ');
>                if (c == '\\') {
>                        c = get_next_char();
>                        switch (c) {
> @@ -180,16 +179,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 +200,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 +226,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)
> diff --git a/t/t1303-wacky-config.sh b/t/t1303-wacky-config.sh
> index 080117c..46103a1 100755
> --- a/t/t1303-wacky-config.sh
> +++ b/t/t1303-wacky-config.sh
> @@ -44,7 +44,7 @@ LONG_VALUE=$(printf "x%01021dx a" 7)
>  test_expect_success 'do not crash on special long config line' '
>        setup &&
>        git config section.key "$LONG_VALUE" &&
> -       check section.key "fatal: bad config file line 2 in .git/config"
> +       check section.key "$LONG_VALUE"
>  '
>
>  test_done
> --
> 1.7.4.msysgit.0.168.g33778
>
>

Re: [PATCH] config: support values longer than 1024 bytes

[Jeff King]

On Wed, Apr 06, 2011 at 01:30:03AM +0200, Erik Faye-Lund wrote:

> The rather awkward return statement with strdup("") is because
> strbuf_detach returns NULL when there's nothing allocated. Even
> worse, it returns an uninitialized string if the string has been
> initialized with a non-zero 'hint'.

That seems like two bugs in the strbuf code.

I would expect strbuf_detach to _always_ return an allocated buffer,
even if it is xstrdup(""). Though the code in strbuf_detach is explicit
about returning NULL rather than slopbuf, so perhaps there is a case
where it is useful.

But for the other, one of the invariants of strbuf is that the string is
always NUL-terminated. So I would expect strbuf_init to properly
NUL-terminate after growing based on the hint.

>  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;

One thing you could about the strbuf_detach thing is to just use a
single static strbuf instead of the static array, and just reset its
length back to zero on each call. So:

  static struct strbuf value = STRBUF_INIT;
  ...
  strbuf_reset(&value);

> -			value[len] = 0;
> -			return value;
> +			return value.len ?
> +			    strbuf_detach(&value, NULL) :
> +			    strdup("");

Then this just becomes:

  return value.buf;

>  			default:
> +				strbuf_release(&value);
>  				return NULL;

And you can drop the release.

> @@ -226,7 +226,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;

And drop this hunk, since callers no longer need to free.

I do wonder, though, if we could be reusing the unquote_c_style()
function in quote.c. They are obviously similar, but I haven't checked
if there is more going on in the config code.

-Peff

Re: [PATCH] config: support values longer than 1024 bytes

[Erik Faye-Lund]

On Wed, Apr 6, 2011 at 2:52 AM, Jeff King <peff@peff.net> wrote:
> On Wed, Apr 06, 2011 at 01:30:03AM +0200, Erik Faye-Lund wrote:
>
>> The rather awkward return statement with strdup("") is because
>> strbuf_detach returns NULL when there's nothing allocated. Even
>> worse, it returns an uninitialized string if the string has been
>> initialized with a non-zero 'hint'.
>
> That seems like two bugs in the strbuf code.
>
> I would expect strbuf_detach to _always_ return an allocated buffer,
> even if it is xstrdup(""). Though the code in strbuf_detach is explicit
> about returning NULL rather than slopbuf, so perhaps there is a case
> where it is useful.

Yeah, this is why I didn't just go for it. Looking through history, it
seems that the reason why it explicitly returns NULL when there's
nothing allocated is that it used to do so "by artifact" before
b315c5c ("strbuf change: be sure ->buf is never ever NULL."), and it
looks like the author didn't want to change the semantics. Fair
enough, changing this takes a lot of code-reading to verify that there
isn't any call-sites that depend on this behavior.

>
> But for the other, one of the invariants of strbuf is that the string is
> always NUL-terminated. So I would expect strbuf_init to properly
> NUL-terminate after growing based on the hint.

I agree. An unterminated yet non-NULL return from strbuf_detach is
just dangerous behavior. Something like this should probably be
applied:

---8<---
diff --git a/strbuf.c b/strbuf.c
index 77444a9..538035a 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -24,14 +24,16 @@ int suffixcmp(const char *str, const char *suffix)
  * buf is non NULL and ->buf is NUL terminated even for a freshly
  * initialized strbuf.
  */
-char strbuf_slopbuf[1];
+char strbuf_slopbuf[1] = { '\0' };

 void strbuf_init(struct strbuf *sb, size_t hint)
 {
 	sb->alloc = sb->len = 0;
 	sb->buf = strbuf_slopbuf;
-	if (hint)
+	if (hint) {
 		strbuf_grow(sb, hint);
+		sb->buf[0] = '\0';
+	}
 }

 void strbuf_release(struct strbuf *sb)
---8<---

>
>>  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;
>
> One thing you could about the strbuf_detach thing is to just use a
> single static strbuf instead of the static array, and just reset its
> length back to zero on each call. So:
>
>  static struct strbuf value = STRBUF_INIT;
>  ...
>  strbuf_reset(&value);
>
>> -                     value[len] = 0;
>> -                     return value;
>> +                     return value.len ?
>> +                         strbuf_detach(&value, NULL) :
>> +                         strdup("");
>
> Then this just becomes:
>
>  return value.buf;
>
>>                       default:
>> +                             strbuf_release(&value);
>>                               return NULL;
>
> And you can drop the release.
>
>> @@ -226,7 +226,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;
>
> And drop this hunk, since callers no longer need to free.
>

Yeah, and this was what I tried first.

I dropped that approach because I misread the code of strbuf_setlen so
I thought it ended up freeing the buffer (which would have lost the
gain we get from making it static). This is wrong, it does not do
that. But this brings a new issue: leaving potentially huge blocks of
memory (especially since this patch is about long lines) allocated
inside a function can be a bit nasty. But it's probably not a big deal
here, the blocks are probably small enough to "leak" without worries,
unless you have a downright evil config file (which I suspect could
only happen due to disk-corruption, in which case you're going to have
much much bigger problems). And that problem can also be fixed by
giving the buffer file-local scope, and adding a cleanup-function that
gets called when config-parsing is finished. But it's probably not
even worth the hassle.

I guess it's my background from handheld/low-power device development
that makes me worry too much about problems like these :P

In other words: I think you're right, it's a much better approach.
Less allocations, less penalty on the start-up time for every little
git-command.

> I do wonder, though, if we could be reusing the unquote_c_style()
> function in quote.c. They are obviously similar, but I haven't checked
> if there is more going on in the config code.

Hmm, this is an interesting suggestion. It would be a part of a bigger
change though: unquote_c_style requires it's input to be in memory,
while parse_value uses a function called get_next_char to feed the
parser. So we'd either have to read the entire file into memory, or
find some way to read the file line-by-line while handling \n-escaping
correctly.

It also seems like there's differences in what kind of escaping and
normalization the two functions handle; unquote_c_style handles more
escaped character sequences, while parse_value normalize all
non-escaped space-characters ('\t' et. al) into SP. This might not be
such a big problem in reality.

Re: [PATCH] config: support values longer than 1024 bytes

[Jeff King]

On Wed, Apr 06, 2011 at 11:10:42AM +0200, Erik Faye-Lund wrote:

> > But for the other, one of the invariants of strbuf is that the string is
> > always NUL-terminated. So I would expect strbuf_init to properly
> > NUL-terminate after growing based on the hint.
> 
> I agree. An unterminated yet non-NULL return from strbuf_detach is
> just dangerous behavior. Something like this should probably be
> applied:
> 
> ---8<---
> diff --git a/strbuf.c b/strbuf.c
> index 77444a9..538035a 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -24,14 +24,16 @@ int suffixcmp(const char *str, const char *suffix)
>   * buf is non NULL and ->buf is NUL terminated even for a freshly
>   * initialized strbuf.
>   */
> -char strbuf_slopbuf[1];
> +char strbuf_slopbuf[1] = { '\0' };

This hunk is redundant. slopbuf will already be initialized to 0.

>  void strbuf_init(struct strbuf *sb, size_t hint)
>  {
>  	sb->alloc = sb->len = 0;
>  	sb->buf = strbuf_slopbuf;
> -	if (hint)
> +	if (hint) {
>  		strbuf_grow(sb, hint);
> +		sb->buf[0] = '\0';
> +	}
>  }

But this one is the right fix.

> that. But this brings a new issue: leaving potentially huge blocks of
> memory (especially since this patch is about long lines) allocated
> inside a function can be a bit nasty. But it's probably not a big deal

Yeah. It's just one block, though, and in the normal case it is probably
only about 80 characters. So it is more efficient than what's there now. :)

Somebody could have some gigantic value, though, and yes, we'll grow to
the biggest one and never free that memory. You could also have
parse_value take a strbuf parameter to output into, and then free it
after config reading is done.

> In other words: I think you're right, it's a much better approach.
> Less allocations, less penalty on the start-up time for every little
> git-command.

I doubt the efficiency increase is measurable. We end up xstrdup'ing
quite a few of the values in the config callbacks anyway. I would do
whatever seems most natural for reading/writing the code.

> > I do wonder, though, if we could be reusing the unquote_c_style()
> > function in quote.c. They are obviously similar, but I haven't checked
> > if there is more going on in the config code.
> 
> Hmm, this is an interesting suggestion. It would be a part of a bigger
> change though: unquote_c_style requires it's input to be in memory,
> while parse_value uses a function called get_next_char to feed the
> parser. So we'd either have to read the entire file into memory, or
> find some way to read the file line-by-line while handling \n-escaping
> correctly.
> 
> It also seems like there's differences in what kind of escaping and
> normalization the two functions handle; unquote_c_style handles more
> escaped character sequences, while parse_value normalize all
> non-escaped space-characters ('\t' et. al) into SP. This might not be
> such a big problem in reality.

This was just a random thought that I had, and I didn't investigate it
how hard it would be.  If it turns out to be too much trouble, just
forget it.

-Peff

Re: [PATCH] config: support values longer than 1024 bytes

[Erik Faye-Lund]

On Wed, Apr 6, 2011 at 5:35 PM, Jeff King <peff@peff.net> wrote:
> On Wed, Apr 06, 2011 at 11:10:42AM +0200, Erik Faye-Lund wrote:
>
>> > But for the other, one of the invariants of strbuf is that the string is
>> > always NUL-terminated. So I would expect strbuf_init to properly
>> > NUL-terminate after growing based on the hint.
>>
>> I agree. An unterminated yet non-NULL return from strbuf_detach is
>> just dangerous behavior. Something like this should probably be
>> applied:
>>
>> ---8<---
>> diff --git a/strbuf.c b/strbuf.c
>> index 77444a9..538035a 100644
>> --- a/strbuf.c
>> +++ b/strbuf.c
>> @@ -24,14 +24,16 @@ int suffixcmp(const char *str, const char *suffix)
>>   * buf is non NULL and ->buf is NUL terminated even for a freshly
>>   * initialized strbuf.
>>   */
>> -char strbuf_slopbuf[1];
>> +char strbuf_slopbuf[1] = { '\0' };
>
> This hunk is redundant. slopbuf will already be initialized to 0.

Right, silly me. I somehow thought that implicit zero-initialization
only applied to static variables, but K&R tells me I was wrong. Thanks
for pointing it out :)

>
>>  void strbuf_init(struct strbuf *sb, size_t hint)
>>  {
>>       sb->alloc = sb->len = 0;
>>       sb->buf = strbuf_slopbuf;
>> -     if (hint)
>> +     if (hint) {
>>               strbuf_grow(sb, hint);
>> +             sb->buf[0] = '\0';
>> +     }
>>  }
>
> But this one is the right fix.
>

OK, I'll turn this into a two-patch series, then.

>> that. But this brings a new issue: leaving potentially huge blocks of
>> memory (especially since this patch is about long lines) allocated
>> inside a function can be a bit nasty. But it's probably not a big deal
>
> Yeah. It's just one block, though, and in the normal case it is probably
> only about 80 characters. So it is more efficient than what's there now. :)
>
> Somebody could have some gigantic value, though, and yes, we'll grow to
> the biggest one and never free that memory. You could also have
> parse_value take a strbuf parameter to output into, and then free it
> after config reading is done.
>
>> In other words: I think you're right, it's a much better approach.
>> Less allocations, less penalty on the start-up time for every little
>> git-command.
>
> I doubt the efficiency increase is measurable. We end up xstrdup'ing
> quite a few of the values in the config callbacks anyway. I would do
> whatever seems most natural for reading/writing the code.
>

I think I'll just leave the single leak in. Since it would allocate
the buffer lazily, it would really only "waste" more memory than the
existing implementation when the old implementation would fail. So my
conscience is clear ;)

>> > I do wonder, though, if we could be reusing the unquote_c_style()
>> > function in quote.c. They are obviously similar, but I haven't checked
>> > if there is more going on in the config code.
>>
>> Hmm, this is an interesting suggestion. It would be a part of a bigger
>> change though: unquote_c_style requires it's input to be in memory,
>> while parse_value uses a function called get_next_char to feed the
>> parser. So we'd either have to read the entire file into memory, or
>> find some way to read the file line-by-line while handling \n-escaping
>> correctly.
>>
>> It also seems like there's differences in what kind of escaping and
>> normalization the two functions handle; unquote_c_style handles more
>> escaped character sequences, while parse_value normalize all
>> non-escaped space-characters ('\t' et. al) into SP. This might not be
>> such a big problem in reality.
>
> This was just a random thought that I had, and I didn't investigate it
> how hard it would be.  If it turns out to be too much trouble, just
> forget it.
>

Yeah. I think I'll skip it, since it isn't dead obvious.

Thanks for the input, I'll cook up a new version soon.