[PATCH] strbuf_grow(): maintain nul-termination even for new buffer

[PATCH] strbuf_grow(): maintain nul-termination even for new buffer

[Thomas Rast]

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)
@@ -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

Re: [PATCH] strbuf_grow(): maintain nul-termination even for new buffer

[Erik Faye-Lund]

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.

Re: [PATCH] strbuf_grow(): maintain nul-termination even for new buffer

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> 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.

Re: [PATCH] strbuf_grow(): maintain nul-termination even for new buffer

[Brandon Casey]

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 <trast@student.ethz.ch>

Patch looks good.

-Brandon