On Tue, Sep 27, 2016 at 10:14:16AM +0200, Lars Schneider wrote: > >>> + strbuf_grow(sb_out, PKTLINE_DATA_MAXLEN+1); > >>> + paket_len = packet_read(fd_in, NULL, NULL, > >>> + sb_out->buf + sb_out->len, PKTLINE_DATA_MAXLEN+1, options); > [...] > After looking at it with fresh eyes I think the existing code is probably correct, > but maybe a bit confusing. > > packet_read() adds a '\0' at the end of the destination buffer: > https://github.com/git/git/blob/21f862b498925194f8f1ebe8203b7a7df756555b/pkt-line.c#L206 > > That is why the destination buffer needs to be one byte larger than the expected content. > > However, in this particular case that wouldn't be necessary because the destination > buffer is a 'strbuf' that allocates an extra byte for '\0' at the end. But we are not > supposed to write to this extra byte: > https://github.com/git/git/blob/21f862b498925194f8f1ebe8203b7a7df756555b/strbuf.h#L25-L31 Right. The allocation happens as part of strbuf_grow(), but whatever fills the buffer is expected to write the actual NUL (either manually, or by calling strbuf_setlen(). I see the bit you quoted warns not to touch the extra byte yourself, though I wonder if that is a bit heavy-handed (I guess it would matter if we moved the extra 1-byte growth into strbuf_setlen(), but I find that a rather unlikely change). That being said, why don't you just use LARGE_PACKET_MAX here? It is already the accepted size for feeding to packet_read(), and we know it has enough space to hold a NUL terminator. Yes, we may over-allocate by 4 bytes, but that isn't really relevant. Strbufs over-allocate anyway. So just: for (;;) { strbuf_grow(sb_out, LARGE_PACKET_MAX); packet_len = packet_read(fd_in, NULL, NULL, sb_out->buf + sb_out->len, LARGE_PACKET_MAX, options); if (packet_len <= 0) break; /* * no need for strbuf_setlen() here; packet_read always adds a * NUL terminator. */ sb_out->len += packet_len; } You _could_ make the final line of the loop use strbuf_setlen(); it would just overwrite something we already know is a NUL (and we know that no extra allocation is necessary). Also, using LARGE_PACKET_MAX fixes the fact that this patch is using PKTLINE_DATA_MAXLEN before it is actually defined. :) You might want to occasionally run: git rebase -x make to make sure all of your incremental steps are valid (or even "make test" if you are extremely patient; I often do that once after a big round of complex interactive-rebase reordering). > I see two options: > > > (1) I leave the +1 as is and add a comment why the extra byte is necessary. > > Pro: No change in existing code necessary > Con: The destination buffer has two '\0' at the end. > > > (2) I add an option PACKET_READ_DISABLE_NUL_TERMINATION. If the option is > set then no '\0' byte is added to the end. > > Pro: Correct solution, no byte wasted. > Con: Change in existing code required. > > > Any preference? Of the two, I prefer (1), though I like what I suggested above even more (big surprise, I know). -Peff