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@xxxxxxxxxxxxxxx> Patch looks good. -Brandon -- 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