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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Aug 9, 2022, 10:37 UTC
Message-ID: <663onqs0-465s-023o-9s25-p2193ss5so59@tzk.qr>
In-Reply-To: <xmqqr11zoe6i.fsf@gitster.g>

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