On Mon, Aug 29, 2011 at 11:16 PM, Thomas Rast <trast@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> ---
>
> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html