Am 05.07.23 um 22:33 schrieb Junio C Hamano: > René Scharfe <l.s.r@xxxxxx> 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.. > 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é
