From: Erik Faye-Lund <kusmabite@gmail.com>
Date: Mon, 29 Aug 2011 22:41:28 GMT
Subject: Re: [PATCH] strbuf_grow(): maintain nul-termination even for new buffer
Message-ID: <CABPQNSZmMPSbjECJTAiDZ4OVf2Yue=eDnx1q178aDaNeJ52n1w@mail.gmail.com>
In-Reply-To: <c8d8686c1813885a36d8f4cada218686989df236.1314651926.git.trast@student.ethz.ch>

On Mon, Aug 29, 2011 at 11:16 PM, Thomas Rast <trast@student.ethz.ch> 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 <trast@student.ethz.ch>
> ---
>
> 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.