Re: [PATCH v8 07/11] pkt-line: add functions to read/write flush terminated packet streams

On 26 Sep 2016, at 22:23, Lars Schneider [off-list ref] wrote:

On 25 Sep 2016, at 15:46, Jakub Narębski [off-list ref] wrote:

quoted

W dniu 20.09.2016 o 21:02, larsxschneider@gmail.com pisze:

quoted

From: Lars Schneider <redacted>

quoted

quoted

+		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