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

["René Scharfe" <l.s.r@web.de>]

Am 05.07.23 um 07:56 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> 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.

Indeed, GCC warns if you give it a short array, but not if you pass a
pointer: https://godbolt.org/z/T3xfE5W9n.  Clang doesn't care at all.

So that was wishful thinking on my part.  Sorry.  :-/

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

With no way to enforce passing an array of a certain size to a function
the only safe options I see are keeping the length check, using a macro
or inlining the calculation.  Hmm.

René