On 26 Sep 2016, at 22:23, Lars Schneider <larsxschnei...@gmail.com> wrote:

> On 25 Sep 2016, at 15:46, Jakub Narębski <jna...@gmail.com> wrote:
>> W dniu 20.09.2016 o 21:02, larsxschnei...@gmail.com pisze:
>>> From: Lars Schneider <larsxschnei...@gmail.com>
>>> +           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 
but maybe a bit confusing.

packet_read() adds a '\0' at the end of the destination buffer:

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 
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:

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?


Reply via email to