[PATCH v2 1/2] strbuf: make sure buffer is zero-terminated

[PATCH v2 1/2] strbuf: make sure buffer is zero-terminated

[Erik Faye-Lund]

strbuf_init does not zero-terminate the initial buffer when hint is
non-zero. Fix this so strbuf_attach does not return garbage.

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 strbuf.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/strbuf.c b/strbuf.c
index 77444a9..09c43ae 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -30,8 +30,10 @@ 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)
-- 
1.7.4.msysgit.0.916.ga2194

[PATCH v2 2/2] config: support values longer than 1023 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 fixed-size buffer.

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

Updated as suggested by Peff.

 config.c                |   18 ++++++++----------
 t/t1303-wacky-config.sh |    2 +-
 2 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/config.c b/config.c
index 0abcada..b7fc2c5 100644
--- a/config.c
+++ b/config.c
@@ -133,23 +133,21 @@ static int get_next_char(void)
 
 static char *parse_value(void)
 {
-	static char value[1024];
-	int quote = 0, comment = 0, len = 0, space = 0;
+	static struct strbuf value = STRBUF_INIT;
+	int quote = 0, comment = 0, space = 0;
 
+	strbuf_reset(&value);
 	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.buf;
 		}
 		if (comment)
 			continue;
 		if (isspace(c) && !quote) {
-			if (len)
+			if (value.len)
 				space++;
 			continue;
 		}
@@ -160,7 +158,7 @@ static char *parse_value(void)
 			}
 		}
 		for (; space; space--)
-			value[len++] = ' ';
+			strbuf_addch(&value, ' ');
 		if (c == '\\') {
 			c = get_next_char();
 			switch (c) {
@@ -182,14 +180,14 @@ static char *parse_value(void)
 			default:
 				return NULL;
 			}
-			value[len++] = c;
+			strbuf_addch(&value, c);
 			continue;
 		}
 		if (c == '"') {
 			quote = 1-quote;
 			continue;
 		}
-		value[len++] = c;
+		strbuf_addch(&value, 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.916.ga2194

Re: [PATCH v2 1/2] strbuf: make sure buffer is zero-terminated

[Jeff King]

On Sun, Apr 10, 2011 at 10:54:17PM +0200, Erik Faye-Lund wrote:

> strbuf_init does not zero-terminate the initial buffer when hint is
> non-zero. Fix this so strbuf_attach does not return garbage.

s/attach/detach/ I think. It is more than just detach, though; the
NUL-termination is supposed to be an invariant of strbuf, so any code
just looking at "sb.buf" would be broken. So your code looks fine, but
you may want to mention that in the commit message.

-Peff

Re: [PATCH v2 2/2] config: support values longer than 1023 bytes

[Jeff King]

On Sun, Apr 10, 2011 at 10:54:18PM +0200, Erik Faye-Lund 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 fixed-size buffer.
> 
> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>

Looks fine to me.

Acked-by: Jeff King <peff@peff.net>

-Peff

Re: [PATCH v2 1/2] strbuf: make sure buffer is zero-terminated

[Erik Faye-Lund]

On Sun, Apr 10, 2011 at 10:57 PM, Jeff King <peff@peff.net> wrote:
> On Sun, Apr 10, 2011 at 10:54:17PM +0200, Erik Faye-Lund wrote:
>
>> strbuf_init does not zero-terminate the initial buffer when hint is
>> non-zero. Fix this so strbuf_attach does not return garbage.
>
> s/attach/detach/ I think.

Indeed, thanks for catching it.

> It is more than just detach, though; the
> NUL-termination is supposed to be an invariant of strbuf, so any code
> just looking at "sb.buf" would be broken. So your code looks fine, but
> you may want to mention that in the commit message.
>

You're right. How about something like this instead?

strbuf_init does not zero-terminate the initial buffer when hint is
non-zero. Fix this so we can rely on the string to be zero-terminated
even if we haven't filled it with anything yet.

Re: [PATCH v2 1/2] strbuf: make sure buffer is zero-terminated

[Jeff King]

On Sun, Apr 10, 2011 at 11:04:26PM +0200, Erik Faye-Lund wrote:

> > It is more than just detach, though; the
> > NUL-termination is supposed to be an invariant of strbuf, so any code
> > just looking at "sb.buf" would be broken. So your code looks fine, but
> > you may want to mention that in the commit message.
> >
> 
> You're right. How about something like this instead?
> 
> strbuf_init does not zero-terminate the initial buffer when hint is
> non-zero. Fix this so we can rely on the string to be zero-terminated
> even if we haven't filled it with anything yet.

Sure, that's fine.

-Peff

Re: [PATCH v2 1/2] strbuf: make sure buffer is zero-terminated

[Junio C Hamano]

Erik Faye-Lund <kusmabite@gmail.com> writes:

> You're right. How about something like this instead?
>
> strbuf_init does not zero-terminate the initial buffer when hint is
> non-zero. Fix this so we can rely on the string to be zero-terminated
> even if we haven't filled it with anything yet.

Sounds sensible.  Thanks, both.