Re: [PATCH v3 2/3] t0021: implementation the rot13-filter.pl script in C

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

On Mon, 1 Aug 2022, Junio C Hamano wrote:

>  * read_capabilities() feeds the buffer taken from
>    packet_read_line(), so buf[size] should be NUL terminated
>    already.

Could you help me agree?

In `packet_read_line()`, we call `packet_read()` with the
`PACKET_READ_CHOMP_NEWLINE` option, but we do not NUL-terminate the
buffer.

See https://github.com/git/git/blob/v2.37.1/pkt-line.c#L488-L494

In `packet_read()`, we call `packet_read_with_status()`, but do not
NUL-terminate the buffer.

See https://github.com/git/git/blob/v2.37.1/pkt-line.c#L478-L486

In `packet_read_with_status()`, I see that we call `get_packet_data()`
which does not NUL-terminate the buffer. Then we parse the length via
`packet_length()` which does not NUL-terminate the buffer.

Then, crucially, if the packet length is smaller than 3, we set the length
that is returned to 0 and return early indicating the conditions
`PACKET_READ_FLUSH`, `PACKET_READ_DELIM`, or `PACKET_READ_RESPONSE_END`,
which are ignored by `packet_read()`.

In this instance, the buffer is not NUL-terminated, I think. But if you
see that I missed something, I would like to know.

See https://github.com/git/git/blob/v2.37.1/pkt-line.c#L399-L476

And yes, in the case that there is a regular payload,
https://github.com/git/git/blob/v2.37.1/pkt-line.c#L456 NUL-terminates the
buffer.

And the proposed `get_value()` function would avoid returning a not
NUL-terminated buffer by virtue of using the `skip_prefix_mem()` function
with a non-empty prefix but a zero length buffer.

Therefore it is _still_ safe to skip the `buf[size] = '\0';` assignment
despite what I wrote above, even if it adds yet another piece of code to
Git's source code which is harder than necessary to reason about.

After all, it took me half an hour to research and write up this mail,
when reading `buf[size] = '\0';` would have taken all of two seconds to
verify that the code is safe.

Ciao,
Dscho