| Message ID | 20241206-pks-sign-compare-v4-8-0344c6dfb219@pks.im (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Start compiling with `-Wsign-compare` | expand |
On Fri, Dec 06, 2024 at 11:27:23AM +0100, Patrick Steinhardt wrote: > Similar to the preceding commit, we get a warning in `get_packet_data()` > on 32 bit platforms due to our lenient use of `ssize_t`. This function > is kind of curious though: we accept an `unsigned size` of bytes to > read, then store the actual number of bytes read in an `ssize_t` and > return it as an `int`. This is a whole lot of integer conversions, and > in theory these can cause us to overflow when the passed-in size is > larger than `ssize_t`, which on 32 bit platforms is implemented as an > `int`. I think part of the reason this code is so lenient is that we know that pktline lengths will always fit into a 16-bit integer anyway. But I like that we can (after your patch) look at this function in isolation and immediately see there are no integer problems. > None of the callers of that function even care about the number of bytes > we have read, so returning that number is moot anyway. Refactor the > function such that it only returns an error code, which plugs the > potential overflow. While at it, convert the passed-in size parameter to > be of type `size_t`. This seems like a good approach to me. Whenever I think "but no callers do X", I always try to double-check: "might new callers want to do X"? And in this case, I think the answer is "no". They are asking to read a whole packet, and it is an error if we don't come up with whole thing. Showing the partial garbage we did get is unlikely outside of debug tracing (and in that case we'd probably put the tracing inside this function anyway). > pkt-line.c | 20 +++++++++++--------- > 1 file changed, 11 insertions(+), 9 deletions(-) The patch itself looks good. -Peff
Jeff King <peff@peff.net> writes: > ... Whenever I think "but no callers > do X", I always try to double-check: "might new callers want to do X"? > And in this case, I think the answer is "no". A very good piece of advice for those who are watching from sidelines. I agree that a return value that tries to tell more than "yup, I read everything as instructed" would not help in this case. > They are asking to read a > whole packet, and it is an error if we don't come up with whole thing. > Showing the partial garbage we did get is unlikely outside of debug > tracing (and in that case we'd probably put the tracing inside this > function anyway). > >> pkt-line.c | 20 +++++++++++--------- >> 1 file changed, 11 insertions(+), 9 deletions(-) > > The patch itself looks good. > > -Peff
diff --git a/pkt-line.c b/pkt-line.c index 90ea2b6974b1d0957cfdc5e2f9a2c30720723f12..2dc29d5b68e45740a7f4e473b7c7ffa57bc09528 100644 --- a/pkt-line.c +++ b/pkt-line.c @@ -340,30 +340,32 @@ int write_packetized_from_buf_no_flush_count(const char *src_in, size_t len, } static int get_packet_data(int fd, char **src_buf, size_t *src_size, - void *dst, unsigned size, int options) + void *dst, size_t size, int options) { - ssize_t ret; + size_t bytes_read; if (fd >= 0 && src_buf && *src_buf) BUG("multiple sources given to packet_read"); /* Read up to "size" bytes from our source, whatever it is. */ if (src_buf && *src_buf) { - ret = size < *src_size ? size : *src_size; - memcpy(dst, *src_buf, ret); - *src_buf += ret; - *src_size -= ret; + bytes_read = size < *src_size ? size : *src_size; + memcpy(dst, *src_buf, bytes_read); + *src_buf += bytes_read; + *src_size -= bytes_read; } else { - ret = read_in_full(fd, dst, size); + ssize_t ret = read_in_full(fd, dst, size); if (ret < 0) { if (options & PACKET_READ_GENTLE_ON_READ_ERROR) return error_errno(_("read error")); die_errno(_("read error")); } + + bytes_read = (size_t) ret; } /* And complain if we didn't get enough bytes to satisfy the read. */ - if (ret != size) { + if (bytes_read != size) { if (options & PACKET_READ_GENTLE_ON_EOF) return -1; @@ -372,7 +374,7 @@ static int get_packet_data(int fd, char **src_buf, size_t *src_size, die(_("the remote end hung up unexpectedly")); } - return ret; + return 0; } int packet_length(const char lenbuf_hex[4], size_t size)
Similar to the preceding commit, we get a warning in `get_packet_data()` on 32 bit platforms due to our lenient use of `ssize_t`. This function is kind of curious though: we accept an `unsigned size` of bytes to read, then store the actual number of bytes read in an `ssize_t` and return it as an `int`. This is a whole lot of integer conversions, and in theory these can cause us to overflow when the passed-in size is larger than `ssize_t`, which on 32 bit platforms is implemented as an `int`. None of the callers of that function even care about the number of bytes we have read, so returning that number is moot anyway. Refactor the function such that it only returns an error code, which plugs the potential overflow. While at it, convert the passed-in size parameter to be of type `size_t`. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- pkt-line.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)