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

From: René Scharfe <l.s.r@web.de>
Date: Jul 5, 2023, 21:11 UTC
Message-ID: <6f984150-5a98-b2b8-1791-b101f5953b7a@web.de>
In-Reply-To: <xmqq4jmiyu7s.fsf@gitster.g>

Am 05.07.23 um 22:33 schrieb Junio C Hamano:

Show 10 quoted lines

> René Scharfe <l.s.r@web.de> writes:
>
>>> 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.

Three more ideas: Wrap the buffer in a custom struct, pass the four bytes as a uint32_t or as four separate char parameters. I better stop now..

Show 5 quoted lines

> We keep repeating "length check" because that is what the comment in
> the function says, but even if the caller has 4-byte, that 4-byte
> substring at the beginning is what it read from the untrusted side
> over the network.  We should be checking if we have 4 hexadecimal
> length even if we are not running beyond the end of the buffer, no?

Sure, that's done. If any of the four characters is not a hexadecimal digit then packet_length() returns a negative value, before and after the change.

> So it may be that the comment needs to be fixed more than the code.

Which comment specifically? The one in pkt-line.h doesn't mention what happens if you pass in a short buffer -- leaving it undefined is OK, I think. And in and around pkt-line.c::packet_length() I don't see any comment?

René