threads / patch / 27055

v2, 2 partsstrbuf: make sure buffer is zero-terminated

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

## tl;dr

7 messages between Apr 10, 2011 and Apr 11, 2011. Diffs are folded; open one to read it.

replies: 6people: 3as markdown or json

Erik Faye-Lund· Apr 10, 2011, 20:54 UTC · lore

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(-)
Show changes to strbuf.c +3 −1
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
Erik Faye-Lund· Apr 10, 2011, 20:54 UTC · re: Erik Faye-Lund · lore

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

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(-)
Show changes to 2 files +9 −11

config.c, t/t1303-wacky-config.sh

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
Jeff King· Apr 10, 2011, 20:58 UTC · re: Erik Faye-Lund · lore

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

On Sun, Apr 10, 2011 at 10:54:18PM +0200, Erik Faye-Lund wrote:
Show 10 quoted lines
> 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
Jeff King· Apr 10, 2011, 20:57 UTC · re: Erik Faye-Lund · lore

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

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
Erik Faye-Lund· Apr 10, 2011, 21:04 UTC · re: Jeff King · lore

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

On Sun, Apr 10, 2011 at 10:57 PM, Jeff King <peff@peff.net> wrote:
Show 6 quoted lines
> 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.
Show 5 quoted lines
> 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.

Jeff King· Apr 10, 2011, 21:05 UTC · re: Erik Faye-Lund · lore

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

On Sun, Apr 10, 2011 at 11:04:26PM +0200, Erik Faye-Lund wrote:
Show 11 quoted lines
> > 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
Junio C Hamano· Apr 11, 2011, 21:49 UTC · re: Erik Faye-Lund · lore

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

Erik Faye-Lund <kusmabite@gmail.com> writes:
Show 5 quoted lines
> 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.

← back to recent threads