# [PATCH] strbuf_grow(): maintain nul-termination even for new buffer 4 messages from 2011-08-29 to 2011-08-29. Participants: Thomas Rast, Erik Faye-Lund, Junio C Hamano, Brandon Casey. Thread: https://gitlist.dev/t/28255 ## Thomas Rast, 2011-08-29 21:16 Subject: [PATCH] strbuf_grow(): maintain nul-termination even for new buffer Message-ID: URL: https://gitlist.dev/e/c8d8686c1813885a36d8f4cada218686989df236.1314651926.git.trast%40student.ethz.ch ``` In the case where sb is initialized to the slopbuf (through strbuf_init(sb,0) or STRBUF_INIT), strbuf_grow() loses the terminating nul: it grows the buffer, but gives ALLOC_GROW a NULL source to avoid it being freed. So ALLOC_GROW does not copy anything to the new memory area. This subtly broke the call to strbuf_getline in read_next_command() [fast-import.c:1855], which goes strbuf_detach(&command_buf, NULL); # command_buf is now = STRBUF_INIT stdin_eof = strbuf_getline(&command_buf, stdin, '\n'); if (stdin_eof) return EOF; In strbuf_getwholeline, this did strbuf_grow(sb, 0); # loses nul-termination if (feof(fp)) return EOF; strbuf_reset(sb); # this would have nul-terminated! Valgrind found this because fast-import subsequently uses prefixcmp() on command_buf.buf, which after the EOF exit contains only uninitialized memory. Arguably strbuf_getwholeline is also broken, in that it touches the buffer before deciding whether to do any work. However, it seems more futureproof to not let the strbuf API lose the nul-termination by its own fault. So make sure that strbuf_grow() puts in a nul even if it has nowhere to copy it from. This makes strbuf_grow(sb, 0) a semantic no-op as far as readers of the buffer are concerned. Also remove the nul-termination added by strbuf_init, which is made redudant. Signed-off-by: Thomas Rast --- Only found this now because the bug is only triggered by the tests added in 4cedb78 (fast-import: add input format tests, 2011-08-11). strbuf.c | 9 +++++---- 1 files changed, 5 insertions(+), 4 deletions(-) diff --git a/strbuf.c b/strbuf.c index 1a7df12..4556f96 100644 --- a/strbuf.c +++ b/strbuf.c @@ -30,10 +30,8 @@ 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) @@ -65,12 +63,15 @@ void strbuf_attach(struct strbuf *sb, void *buf, size_t len, size_t alloc) void strbuf_grow(struct strbuf *sb, size_t extra) { + int new_buf = !sb->alloc; if (unsigned_add_overflows(extra, 1) || unsigned_add_overflows(sb->len, extra + 1)) die("you want to use way too much memory"); - if (!sb->alloc) + if (new_buf) sb->buf = NULL; ALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc); + if (new_buf) + sb->buf[0] = '\0'; } void strbuf_trim(struct strbuf *sb) -- 1.7.7.rc0.370.gdcae57 ``` ## Erik Faye-Lund, 2011-08-29 22:41 Subject: Re: [PATCH] strbuf_grow(): maintain nul-termination even for new buffer Message-ID: URL: https://gitlist.dev/e/CABPQNSZmMPSbjECJTAiDZ4OVf2Yue%3DeDnx1q178aDaNeJ52n1w%40mail.gmail.com In-Reply-To: ``` On Mon, Aug 29, 2011 at 11:16 PM, Thomas Rast wrote: > In the case where sb is initialized to the slopbuf (through > strbuf_init(sb,0) or STRBUF_INIT), strbuf_grow() loses the terminating > nul: it grows the buffer, but gives ALLOC_GROW a NULL source to avoid > it being freed.  So ALLOC_GROW does not copy anything to the new > memory area. > > This subtly broke the call to strbuf_getline in read_next_command() > [fast-import.c:1855], which goes > >    strbuf_detach(&command_buf, NULL);  # command_buf is now = STRBUF_INIT >    stdin_eof = strbuf_getline(&command_buf, stdin, '\n'); >    if (stdin_eof) >            return EOF; > > In strbuf_getwholeline, this did > >    strbuf_grow(sb, 0);  # loses nul-termination >    if (feof(fp)) >            return EOF; >    strbuf_reset(sb);    # this would have nul-terminated! > > Valgrind found this because fast-import subsequently uses prefixcmp() > on command_buf.buf, which after the EOF exit contains only > uninitialized memory. > > Arguably strbuf_getwholeline is also broken, in that it touches the > buffer before deciding whether to do any work.  However, it seems more > futureproof to not let the strbuf API lose the nul-termination by its > own fault. > > So make sure that strbuf_grow() puts in a nul even if it has nowhere > to copy it from.  This makes strbuf_grow(sb, 0) a semantic no-op as > far as readers of the buffer are concerned. > > Also remove the nul-termination added by strbuf_init, which is made > redudant. > > Signed-off-by: Thomas Rast > --- > > Only found this now because the bug is only triggered by the tests > added in 4cedb78 (fast-import: add input format tests, 2011-08-11). > > >  strbuf.c |    9 +++++---- >  1 files changed, 5 insertions(+), 4 deletions(-) > > diff --git a/strbuf.c b/strbuf.c > index 1a7df12..4556f96 100644 > --- a/strbuf.c > +++ b/strbuf.c > @@ -30,10 +30,8 @@ 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) This gave me a bit of deja-vu, and indeed: 5e7a5d9 strbuf: make sure buffer is zero-terminated > @@ -65,12 +63,15 @@ void strbuf_attach(struct strbuf *sb, void *buf, size_t len, size_t alloc) > >  void strbuf_grow(struct strbuf *sb, size_t extra) >  { > +       int new_buf = !sb->alloc; >        if (unsigned_add_overflows(extra, 1) || >            unsigned_add_overflows(sb->len, extra + 1)) >                die("you want to use way too much memory"); > -       if (!sb->alloc) > +       if (new_buf) >                sb->buf = NULL; >        ALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc); > +       if (new_buf) > +               sb->buf[0] = '\0'; >  } > >  void strbuf_trim(struct strbuf *sb) Looks sensible to me. ``` ## Junio C Hamano, 2011-08-29 23:09 Subject: Re: [PATCH] strbuf_grow(): maintain nul-termination even for new buffer Message-ID: <7vk49v4xyd.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vk49v4xyd.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Thomas Rast writes: > So make sure that strbuf_grow() puts in a nul even if it has nowhere > to copy it from. This makes strbuf_grow(sb, 0) a semantic no-op as > far as readers of the buffer are concerned. Makes sense, thanks. > Also remove the nul-termination added by strbuf_init, which is made > redudant. Ok. This is a tangent but if we do not have hint, we point at strbuf_slopbuf[] which is: /* * Used as the default ->buf value, so that people can always assume * buf is non NULL and ->buf is NUL terminated even for a freshly * initialized strbuf. */ char strbuf_slopbuf[1]; While nobody should be writing into it, we do not really enforce the constness of this buffer. I wonder if it would be worth making this into "const char []" and have the complier/linker move it to read-only section to catch potential bugs. ``` ## Brandon Casey, 2011-08-29 23:15 Subject: Re: [PATCH] strbuf_grow(): maintain nul-termination even for new buffer Message-ID: URL: https://gitlist.dev/e/Zt95k-c1_rj2Kpnuk9FpbHYmWLPBvoSLSqoYn1QCwVcBMyhNOcqhbbTXqG_MPUWLyda3Drs3WPmiEc1u4aNIhFY9vm0Hs0J6fHsKZpVH_9Q%40cipher.nrlssc.navy.mil In-Reply-To: ``` On 08/29/2011 04:16 PM, Thomas Rast wrote: > In the case where sb is initialized to the slopbuf (through > strbuf_init(sb,0) or STRBUF_INIT), strbuf_grow() loses the terminating > nul: it grows the buffer, but gives ALLOC_GROW a NULL source to avoid > it being freed. So ALLOC_GROW does not copy anything to the new > memory area. > > This subtly broke the call to strbuf_getline in read_next_command() > [fast-import.c:1855], which goes > > strbuf_detach(&command_buf, NULL); # command_buf is now = STRBUF_INIT > stdin_eof = strbuf_getline(&command_buf, stdin, '\n'); > if (stdin_eof) > return EOF; > > In strbuf_getwholeline, this did > > strbuf_grow(sb, 0); # loses nul-termination I'm thinking this call to strbuf_grow() predates the decision to require that the buf component of a strbuf should always be valid nul-terminated string. It was likely made here solely to force allocation of buf which may have been NULL. I think this line can safely be removed from strbuf_getwholeline(). > if (feof(fp)) > return EOF; > strbuf_reset(sb); # this would have nul-terminated! > > Valgrind found this because fast-import subsequently uses prefixcmp() > on command_buf.buf, which after the EOF exit contains only > uninitialized memory. > > Arguably strbuf_getwholeline is also broken, in that it touches the > buffer before deciding whether to do any work. However, it seems more > futureproof to not let the strbuf API lose the nul-termination by its > own fault. > > So make sure that strbuf_grow() puts in a nul even if it has nowhere > to copy it from. This makes strbuf_grow(sb, 0) a semantic no-op as > far as readers of the buffer are concerned. > > Also remove the nul-termination added by strbuf_init, which is made > redudant. > > Signed-off-by: Thomas Rast Patch looks good. -Brandon ```