On 26 Sep 2016, at 22:23, Lars Schneider <larsxschneider@xxxxxxxxx> wrote: > > On 25 Sep 2016, at 15:46, Jakub Narębski <jnareb@xxxxxxxxx> wrote: > >> W dniu 20.09.2016 o 21:02, larsxschneider@xxxxxxxxx pisze: >>> From: Lars Schneider <larsxschneider@xxxxxxxxx> > > >>> + 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); >> >> A question (which perhaps was answered during the development of this >> patch series): why is this +1 in PKTLINE_DATA_MAXLEN+1 here? > > Nice catch. I think this is wrong: > https://github.com/git/git/blob/6fe1b1407ed91823daa5d487abe457ff37463349/pkt-line.c#L196 > > It should be "if (len > size)" ... then we don't need the "+1" here. > (but I need to think a bit more about this) 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 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? Thanks, Lars