# [PATCH v2 1/2] strbuf: make sure buffer is zero-terminated 7 messages from 2011-04-10 to 2011-04-11. Participants: Erik Faye-Lund, Jeff King, Junio C Hamano. Thread: https://gitlist.dev/t/27055 ## Erik Faye-Lund, 2011-04-10 20:54 Subject: [PATCH v2 1/2] strbuf: make sure buffer is zero-terminated Message-ID: <1302468858-7376-1-git-send-email-kusmabite@gmail.com> URL: https://gitlist.dev/e/1302468858-7376-1-git-send-email-kusmabite%40gmail.com ``` 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 --- 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 ``` ## Erik Faye-Lund, 2011-04-10 20:54 Subject: [PATCH v2 2/2] config: support values longer than 1023 bytes Message-ID: <1302468858-7376-2-git-send-email-kusmabite@gmail.com> URL: https://gitlist.dev/e/1302468858-7376-2-git-send-email-kusmabite%40gmail.com In-Reply-To: <1302468858-7376-1-git-send-email-kusmabite@gmail.com> ``` 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 --- 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 ``` ## Jeff King, 2011-04-10 20:57 Subject: Re: [PATCH v2 1/2] strbuf: make sure buffer is zero-terminated Message-ID: <20110410205711.GA2069@sigill.intra.peff.net> URL: https://gitlist.dev/e/20110410205711.GA2069%40sigill.intra.peff.net In-Reply-To: <1302468858-7376-1-git-send-email-kusmabite@gmail.com> ``` 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 ``` ## Jeff King, 2011-04-10 20:58 Subject: Re: [PATCH v2 2/2] config: support values longer than 1023 bytes Message-ID: <20110410205851.GB2069@sigill.intra.peff.net> URL: https://gitlist.dev/e/20110410205851.GB2069%40sigill.intra.peff.net In-Reply-To: <1302468858-7376-2-git-send-email-kusmabite@gmail.com> ``` 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 Looks fine to me. Acked-by: Jeff King -Peff ``` ## Erik Faye-Lund, 2011-04-10 21:04 Subject: Re: [PATCH v2 1/2] strbuf: make sure buffer is zero-terminated Message-ID: URL: https://gitlist.dev/e/BANLkTinKzatY-1kxLJ73TeDyMW175pr77A%40mail.gmail.com In-Reply-To: <20110410205711.GA2069@sigill.intra.peff.net> ``` On Sun, Apr 10, 2011 at 10:57 PM, Jeff King 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. ``` ## Jeff King, 2011-04-10 21:05 Subject: Re: [PATCH v2 1/2] strbuf: make sure buffer is zero-terminated Message-ID: <20110410210534.GA2522@sigill.intra.peff.net> URL: https://gitlist.dev/e/20110410210534.GA2522%40sigill.intra.peff.net In-Reply-To: ``` 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 ``` ## Junio C Hamano, 2011-04-11 21:49 Subject: Re: [PATCH v2 1/2] strbuf: make sure buffer is zero-terminated Message-ID: <7v8vvgtq55.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v8vvgtq55.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Erik Faye-Lund 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. ```