{"thread":{"id":"27055","subject":"[PATCH v2 1/2] strbuf: make sure buffer is zero-terminated","startedAt":"2011-04-10T20:54:17Z","lastAt":"2011-04-11T21:49:26Z","messageCount":7,"participants":["Erik Faye-Lund","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"165579","messageId":"1302468858-7376-1-git-send-email-kusmabite@gmail.com","threadId":"27055","inReplyTo":null,"subject":"[PATCH v2 1/2] strbuf: make sure buffer is zero-terminated","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-04-10T20:54:17Z","receivedAt":"2011-04-10T20:54:17Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"strbuf_init does not zero-terminate the initial buffer when hint is\nnon-zero. Fix this so strbuf_attach does not return garbage.\n\nSigned-off-by: Erik Faye-Lund \n---\n strbuf.c | 4 +++-\n 1 files changed, 3 insertions(+), 1 deletions(-)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex 77444a9..09c43ae 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -30,8 +30,10 @@ void strbuf_init(struct strbuf *sb, size_t hint)\n {\n \tsb->alloc = sb->len = 0;\n \tsb->buf = strbuf_slopbuf;\n-\tif (hint)\n+\tif (hint) {\n \t\tstrbuf_grow(sb, hint);\n+\t\tsb->buf[0] = '\\0';\n+\t}\n }\n \n void strbuf_release(struct strbuf *sb)\n-- \n1.7.4.msysgit.0.916.ga2194\n"},{"id":"165580","messageId":"1302468858-7376-2-git-send-email-kusmabite@gmail.com","threadId":"27055","inReplyTo":"1302468858-7376-1-git-send-email-kusmabite@gmail.com","subject":"[PATCH v2 2/2] config: support values longer than 1023 bytes","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-04-10T20:54:18Z","receivedAt":"2011-04-10T20:54:18Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"parse_value in config.c has a static buffer of 1024 bytes that it\nparse the value into. This can sometimes be a problem when a\nconfig file contains very long values.\n\nIt's particularly amusing that git-config already is able to write\nsuch files, so it should probably be able to read them as well.\n\nFix this by using a strbuf instead of a fixed-size buffer.\n\nSigned-off-by: Erik Faye-Lund \n---\n\nUpdated as suggested by Peff.\n\n config.c | 18 ++++++++----------\n t/t1303-wacky-config.sh | 2 +-\n 2 files changed, 9 insertions(+), 11 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 0abcada..b7fc2c5 100644\n--- a/config.c\n+++ b/config.c\n@@ -133,23 +133,21 @@ static int get_next_char(void)\n \n static char *parse_value(void)\n {\n-\tstatic char value[1024];\n-\tint quote = 0, comment = 0, len = 0, space = 0;\n+\tstatic struct strbuf value = STRBUF_INIT;\n+\tint quote = 0, comment = 0, space = 0;\n \n+\tstrbuf_reset(&value);\n \tfor (;;) {\n \t\tint c = get_next_char();\n-\t\tif (len >= sizeof(value) - 1)\n-\t\t\treturn NULL;\n \t\tif (c == '\\n') {\n \t\t\tif (quote)\n \t\t\t\treturn NULL;\n-\t\t\tvalue[len] = 0;\n-\t\t\treturn value;\n+\t\t\treturn value.buf;\n \t\t}\n \t\tif (comment)\n \t\t\tcontinue;\n \t\tif (isspace(c) && !quote) {\n-\t\t\tif (len)\n+\t\t\tif (value.len)\n \t\t\t\tspace++;\n \t\t\tcontinue;\n \t\t}\n@@ -160,7 +158,7 @@ static char *parse_value(void)\n \t\t\t}\n \t\t}\n \t\tfor (; space; space--)\n-\t\t\tvalue[len++] = ' ';\n+\t\t\tstrbuf_addch(&value, ' ');\n \t\tif (c == '\\\\') {\n \t\t\tc = get_next_char();\n \t\t\tswitch (c) {\n@@ -182,14 +180,14 @@ static char *parse_value(void)\n \t\t\tdefault:\n \t\t\t\treturn NULL;\n \t\t\t}\n-\t\t\tvalue[len++] = c;\n+\t\t\tstrbuf_addch(&value, c);\n \t\t\tcontinue;\n \t\t}\n \t\tif (c == '\"') {\n \t\t\tquote = 1-quote;\n \t\t\tcontinue;\n \t\t}\n-\t\tvalue[len++] = c;\n+\t\tstrbuf_addch(&value, c);\n \t}\n }\n \ndiff --git a/t/t1303-wacky-config.sh b/t/t1303-wacky-config.sh\nindex 080117c..46103a1 100755\n--- a/t/t1303-wacky-config.sh\n+++ b/t/t1303-wacky-config.sh\n@@ -44,7 +44,7 @@ LONG_VALUE=$(printf \"x%01021dx a\" 7)\n test_expect_success 'do not crash on special long config line' '\n \tsetup &&\n \tgit config section.key \"$LONG_VALUE\" &&\n-\tcheck section.key \"fatal: bad config file line 2 in .git/config\"\n+\tcheck section.key \"$LONG_VALUE\"\n '\n \n test_done\n-- \n1.7.4.msysgit.0.916.ga2194\n"},{"id":"165581","messageId":"20110410205711.GA2069@sigill.intra.peff.net","threadId":"27055","inReplyTo":"1302468858-7376-1-git-send-email-kusmabite@gmail.com","subject":"Re: [PATCH v2 1/2] strbuf: make sure buffer is zero-terminated","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-10T20:57:11Z","receivedAt":"2011-04-10T20:57:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 10, 2011 at 10:54:17PM +0200, Erik Faye-Lund wrote:\n\n> strbuf_init does not zero-terminate the initial buffer when hint is\n> non-zero. Fix this so strbuf_attach does not return garbage.\n\ns/attach/detach/ I think. It is more than just detach, though; the\nNUL-termination is supposed to be an invariant of strbuf, so any code\njust looking at \"sb.buf\" would be broken. So your code looks fine, but\nyou may want to mention that in the commit message.\n\n-Peff\n"},{"id":"165582","messageId":"20110410205851.GB2069@sigill.intra.peff.net","threadId":"27055","inReplyTo":"1302468858-7376-2-git-send-email-kusmabite@gmail.com","subject":"Re: [PATCH v2 2/2] config: support values longer than 1023 bytes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-10T20:58:52Z","receivedAt":"2011-04-10T20:58:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 10, 2011 at 10:54:18PM +0200, Erik Faye-Lund wrote:\n\n> parse_value in config.c has a static buffer of 1024 bytes that it\n> parse the value into. This can sometimes be a problem when a\n> config file contains very long values.\n> \n> It's particularly amusing that git-config already is able to write\n> such files, so it should probably be able to read them as well.\n> \n> Fix this by using a strbuf instead of a fixed-size buffer.\n> \n> Signed-off-by: Erik Faye-Lund \n\nLooks fine to me.\n\nAcked-by: Jeff King \n\n-Peff\n"},{"id":"165583","messageId":"BANLkTinKzatY-1kxLJ73TeDyMW175pr77A@mail.gmail.com","threadId":"27055","inReplyTo":"20110410205711.GA2069@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/2] strbuf: make sure buffer is zero-terminated","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-04-10T21:04:26Z","receivedAt":"2011-04-10T21:04:26Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Sun, Apr 10, 2011 at 10:57 PM, Jeff King wrote:\n> On Sun, Apr 10, 2011 at 10:54:17PM +0200, Erik Faye-Lund wrote:\n>\n>> strbuf_init does not zero-terminate the initial buffer when hint is\n>> non-zero. Fix this so strbuf_attach does not return garbage.\n>\n> s/attach/detach/ I think.\n\nIndeed, thanks for catching it.\n\n> It is more than just detach, though; the\n> NUL-termination is supposed to be an invariant of strbuf, so any code\n> just looking at \"sb.buf\" would be broken. So your code looks fine, but\n> you may want to mention that in the commit message.\n>\n\nYou're right. How about something like this instead?\n\nstrbuf_init does not zero-terminate the initial buffer when hint is\nnon-zero. Fix this so we can rely on the string to be zero-terminated\neven if we haven't filled it with anything yet.\n"},{"id":"165584","messageId":"20110410210534.GA2522@sigill.intra.peff.net","threadId":"27055","inReplyTo":"BANLkTinKzatY-1kxLJ73TeDyMW175pr77A@mail.gmail.com","subject":"Re: [PATCH v2 1/2] strbuf: make sure buffer is zero-terminated","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-10T21:05:35Z","receivedAt":"2011-04-10T21:05:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 10, 2011 at 11:04:26PM +0200, Erik Faye-Lund wrote:\n\n> > It is more than just detach, though; the\n> > NUL-termination is supposed to be an invariant of strbuf, so any code\n> > just looking at \"sb.buf\" would be broken. So your code looks fine, but\n> > you may want to mention that in the commit message.\n> >\n> \n> You're right. How about something like this instead?\n> \n> strbuf_init does not zero-terminate the initial buffer when hint is\n> non-zero. Fix this so we can rely on the string to be zero-terminated\n> even if we haven't filled it with anything yet.\n\nSure, that's fine.\n\n-Peff\n"},{"id":"165656","messageId":"7v8vvgtq55.fsf@alter.siamese.dyndns.org","threadId":"27055","inReplyTo":"BANLkTinKzatY-1kxLJ73TeDyMW175pr77A@mail.gmail.com","subject":"Re: [PATCH v2 1/2] strbuf: make sure buffer is zero-terminated","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-04-11T21:49:26Z","receivedAt":"2011-04-11T21:49:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Erik Faye-Lund writes:\n\n> You're right. How about something like this instead?\n>\n> strbuf_init does not zero-terminate the initial buffer when hint is\n> non-zero. Fix this so we can rely on the string to be zero-terminated\n> even if we haven't filled it with anything yet.\n\nSounds sensible. Thanks, both.\n"}]}