René Scharfe <l.s.r@xxxxxx> 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.
> 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?
