Re: [PATCH] pkt-line: don't check string length in packet_length()

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 5, 2023, 05:56 UTC
Message-ID: <xmqqttui3nqn.fsf@gitster.g>
In-Reply-To: <89d58db7-6a01-b3fa-54f0-19d5a3819eb3@web.de>

René Scharfe <l.s.r@web.de> writes:

Show 5 quoted lines

> hex2chr() takes care not to run over the end of a short string.
> 101736a14c (pkt-line: extern packet_length(), 2020-05-19) turned the
> input parameter of packet_length() from a string pointer into an array
> of known length, making string length checks unnecessary.  Get rid of
> them by using hexval() directly.

I am puzzled about the part of the above description on "making string length checks unnecessary". The two callers we currently have both do pass char[4], but the compiler would not stop us from passing a pointer to a memory region of an unknown size; if we butcher one of the current callers

diff --git c/pkt-line.c w/pkt-line.c
index 6e022029ca..e1c49baefd 100644
--- c/pkt-line.c
+++ w/pkt-line.c
@@ -421,7 +421,7 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
 		return PACKET_READ_EOF;
 	}
 
-	len = packet_length(linelen);
+	len = packet_length(buffer);
 
 	if (len < 0) {
 		if (options & PACKET_READ_GENTLE_ON_READ_ERROR)

where "buffer" is just a random piece of memory passed to the caller
and there is no such guarantee like "it at least is 4 bytes long",
we would just slurp garbage and run past the end of the buffer.

> The resulting branchless code is simpler and it becomes easier to see
> that the function mirrors set_packet_header().

I do like the resulting code, but I feel a bit uneasy to sell this
change as "the code becomes more streamlined without losing safety".
It looks more like "this change is safe for our two callers; those
adding more callers in the future are better be very careful", no?