{"thread":{"id":"28255","subject":"[PATCH] strbuf_grow(): maintain nul-termination even for new buffer","startedAt":"2011-08-29T21:16:12Z","lastAt":"2011-08-29T23:15:24Z","messageCount":4,"participants":["Thomas Rast","Erik Faye-Lund","Junio C Hamano","Brandon Casey"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"174500","messageId":"c8d8686c1813885a36d8f4cada218686989df236.1314651926.git.trast@student.ethz.ch","threadId":"28255","inReplyTo":null,"subject":"[PATCH] strbuf_grow(): maintain nul-termination even for new buffer","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-08-29T21:16:12Z","receivedAt":"2011-08-29T21:16:12Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"In the case where sb is initialized to the slopbuf (through\nstrbuf_init(sb,0) or STRBUF_INIT), strbuf_grow() loses the terminating\nnul: it grows the buffer, but gives ALLOC_GROW a NULL source to avoid\nit being freed.  So ALLOC_GROW does not copy anything to the new\nmemory area.\n\nThis subtly broke the call to strbuf_getline in read_next_command()\n[fast-import.c:1855], which goes\n\n    strbuf_detach(&command_buf, NULL);  # command_buf is now = STRBUF_INIT\n    stdin_eof = strbuf_getline(&command_buf, stdin, '\\n');\n    if (stdin_eof)\n            return EOF;\n\nIn strbuf_getwholeline, this did\n\n    strbuf_grow(sb, 0);  # loses nul-termination\n    if (feof(fp))\n            return EOF;\n    strbuf_reset(sb);    # this would have nul-terminated!\n\nValgrind found this because fast-import subsequently uses prefixcmp()\non command_buf.buf, which after the EOF exit contains only\nuninitialized memory.\n\nArguably strbuf_getwholeline is also broken, in that it touches the\nbuffer before deciding whether to do any work.  However, it seems more\nfutureproof to not let the strbuf API lose the nul-termination by its\nown fault.\n\nSo make sure that strbuf_grow() puts in a nul even if it has nowhere\nto copy it from.  This makes strbuf_grow(sb, 0) a semantic no-op as\nfar as readers of the buffer are concerned.\n\nAlso remove the nul-termination added by strbuf_init, which is made\nredudant.\n\nSigned-off-by: Thomas Rast <trast@student.ethz.ch>\n---\n\nOnly found this now because the bug is only triggered by the tests\nadded in 4cedb78 (fast-import: add input format tests, 2011-08-11).\n\n\n strbuf.c |    9 +++++----\n 1 files changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex 1a7df12..4556f96 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -30,10 +30,8 @@ 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@@ -65,12 +63,15 @@ void strbuf_attach(struct strbuf *sb, void *buf, size_t len, size_t alloc)\n \n void strbuf_grow(struct strbuf *sb, size_t extra)\n {\n+\tint new_buf = !sb->alloc;\n \tif (unsigned_add_overflows(extra, 1) ||\n \t    unsigned_add_overflows(sb->len, extra + 1))\n \t\tdie(\"you want to use way too much memory\");\n-\tif (!sb->alloc)\n+\tif (new_buf)\n \t\tsb->buf = NULL;\n \tALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);\n+\tif (new_buf)\n+\t\tsb->buf[0] = '\\0';\n }\n \n void strbuf_trim(struct strbuf *sb)\n-- \n1.7.7.rc0.370.gdcae57\n"},{"id":"174517","messageId":"CABPQNSZmMPSbjECJTAiDZ4OVf2Yue=eDnx1q178aDaNeJ52n1w@mail.gmail.com","threadId":"28255","inReplyTo":"c8d8686c1813885a36d8f4cada218686989df236.1314651926.git.trast@student.ethz.ch","subject":"Re: [PATCH] strbuf_grow(): maintain nul-termination even for new buffer","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-08-29T22:41:28Z","receivedAt":"2011-08-29T22:41:28Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Mon, Aug 29, 2011 at 11:16 PM, Thomas Rast <trast@student.ethz.ch> wrote:\n> In the case where sb is initialized to the slopbuf (through\n> strbuf_init(sb,0) or STRBUF_INIT), strbuf_grow() loses the terminating\n> nul: it grows the buffer, but gives ALLOC_GROW a NULL source to avoid\n> it being freed.  So ALLOC_GROW does not copy anything to the new\n> memory area.\n>\n> This subtly broke the call to strbuf_getline in read_next_command()\n> [fast-import.c:1855], which goes\n>\n>    strbuf_detach(&command_buf, NULL);  # command_buf is now = STRBUF_INIT\n>    stdin_eof = strbuf_getline(&command_buf, stdin, '\\n');\n>    if (stdin_eof)\n>            return EOF;\n>\n> In strbuf_getwholeline, this did\n>\n>    strbuf_grow(sb, 0);  # loses nul-termination\n>    if (feof(fp))\n>            return EOF;\n>    strbuf_reset(sb);    # this would have nul-terminated!\n>\n> Valgrind found this because fast-import subsequently uses prefixcmp()\n> on command_buf.buf, which after the EOF exit contains only\n> uninitialized memory.\n>\n> Arguably strbuf_getwholeline is also broken, in that it touches the\n> buffer before deciding whether to do any work.  However, it seems more\n> futureproof to not let the strbuf API lose the nul-termination by its\n> own fault.\n>\n> So make sure that strbuf_grow() puts in a nul even if it has nowhere\n> to copy it from.  This makes strbuf_grow(sb, 0) a semantic no-op as\n> far as readers of the buffer are concerned.\n>\n> Also remove the nul-termination added by strbuf_init, which is made\n> redudant.\n>\n> Signed-off-by: Thomas Rast <trast@student.ethz.ch>\n> ---\n>\n> Only found this now because the bug is only triggered by the tests\n> added in 4cedb78 (fast-import: add input format tests, 2011-08-11).\n>\n>\n>  strbuf.c |    9 +++++----\n>  1 files changed, 5 insertions(+), 4 deletions(-)\n>\n> diff --git a/strbuf.c b/strbuf.c\n> index 1a7df12..4556f96 100644\n> --- a/strbuf.c\n> +++ b/strbuf.c\n> @@ -30,10 +30,8 @@ void strbuf_init(struct strbuf *sb, size_t hint)\n>  {\n>        sb->alloc = sb->len = 0;\n>        sb->buf = strbuf_slopbuf;\n> -       if (hint) {\n> +       if (hint)\n>                strbuf_grow(sb, hint);\n> -               sb->buf[0] = '\\0';\n> -       }\n>  }\n>\n>  void strbuf_release(struct strbuf *sb)\n\nThis gave me a bit of deja-vu, and indeed:\n5e7a5d9 strbuf: make sure buffer is zero-terminated\n\n> @@ -65,12 +63,15 @@ void strbuf_attach(struct strbuf *sb, void *buf, size_t len, size_t alloc)\n>\n>  void strbuf_grow(struct strbuf *sb, size_t extra)\n>  {\n> +       int new_buf = !sb->alloc;\n>        if (unsigned_add_overflows(extra, 1) ||\n>            unsigned_add_overflows(sb->len, extra + 1))\n>                die(\"you want to use way too much memory\");\n> -       if (!sb->alloc)\n> +       if (new_buf)\n>                sb->buf = NULL;\n>        ALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);\n> +       if (new_buf)\n> +               sb->buf[0] = '\\0';\n>  }\n>\n>  void strbuf_trim(struct strbuf *sb)\n\nLooks sensible to me.\n"},{"id":"174520","messageId":"7vk49v4xyd.fsf@alter.siamese.dyndns.org","threadId":"28255","inReplyTo":"c8d8686c1813885a36d8f4cada218686989df236.1314651926.git.trast@student.ethz.ch","subject":"Re: [PATCH] strbuf_grow(): maintain nul-termination even for new buffer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-29T23:09:14Z","receivedAt":"2011-08-29T23:09:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast <trast@student.ethz.ch> writes:\n\n> So make sure that strbuf_grow() puts in a nul even if it has nowhere\n> to copy it from.  This makes strbuf_grow(sb, 0) a semantic no-op as\n> far as readers of the buffer are concerned.\n\nMakes sense, thanks.\n\n> Also remove the nul-termination added by strbuf_init, which is made\n> redudant.\n\nOk.\n\nThis is a tangent but if we do not have hint, we point at strbuf_slopbuf[]\nwhich is:\n\n    /*\n     * Used as the default ->buf value, so that people can always assume\n     * buf is non NULL and ->buf is NUL terminated even for a freshly\n     * initialized strbuf.\n     */\n    char strbuf_slopbuf[1];\n\nWhile nobody should be writing into it, we do not really enforce the\nconstness of this buffer.\n\nI wonder if it would be worth making this into \"const char []\" and have\nthe complier/linker move it to read-only section to catch potential bugs.\n"},{"id":"174521","messageId":"Zt95k-c1_rj2Kpnuk9FpbHYmWLPBvoSLSqoYn1QCwVcBMyhNOcqhbbTXqG_MPUWLyda3Drs3WPmiEc1u4aNIhFY9vm0Hs0J6fHsKZpVH_9Q@cipher.nrlssc.navy.mil","threadId":"28255","inReplyTo":"c8d8686c1813885a36d8f4cada218686989df236.1314651926.git.trast@student.ethz.ch","subject":"Re: [PATCH] strbuf_grow(): maintain nul-termination even for new buffer","fromName":"Brandon Casey","fromEmail":"brandon.casey.ctr@nrlssc.navy.mil","sentAt":"2011-08-29T23:15:24Z","receivedAt":"2011-08-29T23:15:24Z","isPatch":true,"sender":{"key":"brandon.casey.ctr@nrlssc.navy.mil","avatar":null},"body":"On 08/29/2011 04:16 PM, Thomas Rast wrote:\n> In the case where sb is initialized to the slopbuf (through\n> strbuf_init(sb,0) or STRBUF_INIT), strbuf_grow() loses the terminating\n> nul: it grows the buffer, but gives ALLOC_GROW a NULL source to avoid\n> it being freed.  So ALLOC_GROW does not copy anything to the new\n> memory area.\n> \n> This subtly broke the call to strbuf_getline in read_next_command()\n> [fast-import.c:1855], which goes\n> \n>     strbuf_detach(&command_buf, NULL);  # command_buf is now = STRBUF_INIT\n>     stdin_eof = strbuf_getline(&command_buf, stdin, '\\n');\n>     if (stdin_eof)\n>             return EOF;\n> \n> In strbuf_getwholeline, this did\n> \n>     strbuf_grow(sb, 0);  # loses nul-termination\n\nI'm thinking this call to strbuf_grow() predates the decision to\nrequire that the buf component of a strbuf should always be valid\nnul-terminated string.  It was likely made here solely to force\nallocation of buf which may have been NULL.\n\nI think this line can safely be removed from strbuf_getwholeline().\n\n>     if (feof(fp))\n>             return EOF;\n>     strbuf_reset(sb);    # this would have nul-terminated!\n> \n> Valgrind found this because fast-import subsequently uses prefixcmp()\n> on command_buf.buf, which after the EOF exit contains only\n> uninitialized memory.\n> \n> Arguably strbuf_getwholeline is also broken, in that it touches the\n> buffer before deciding whether to do any work.  However, it seems more\n> futureproof to not let the strbuf API lose the nul-termination by its\n> own fault.\n> \n> So make sure that strbuf_grow() puts in a nul even if it has nowhere\n> to copy it from.  This makes strbuf_grow(sb, 0) a semantic no-op as\n> far as readers of the buffer are concerned.\n> \n> Also remove the nul-termination added by strbuf_init, which is made\n> redudant.\n> \n> Signed-off-by: Thomas Rast <trast@student.ethz.ch>\n\nPatch looks good.\n\n-Brandon\n"}]}