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

From: Brandon Casey <brandon.casey.ctr@nrlssc.navy.mil>
Date: Aug 29, 2011, 23:15 UTC
Message-ID: <Zt95k-c1_rj2Kpnuk9FpbHYmWLPBvoSLSqoYn1QCwVcBMyhNOcqhbbTXqG_MPUWLyda3Drs3WPmiEc1u4aNIhFY9vm0Hs0J6fHsKZpVH_9Q@cipher.nrlssc.navy.mil>
In-Reply-To: <c8d8686c1813885a36d8f4cada218686989df236.1314651926.git.trast@student.ethz.ch>

On 08/29/2011 04:16 PM, Thomas Rast wrote:

Show 17 quoted lines

> 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().

Show 21 quoted lines

>     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