Re: [PATCH v4 08/16] pkt-line: fix -Wsign-compare warning on 32 bit platform

From: Jeff King <peff@peff.net>
Date: Dec 8, 2024, 19:57 UTC
Message-ID: <20241208195703.GA1231962@coredump.intra.peff.net>
In-Reply-To: <20241206-pks-sign-compare-v4-8-0344c6dfb219@pks.im>

On Fri, Dec 06, 2024 at 11:27:23AM +0100, Patrick Steinhardt wrote:

Show 8 quoted lines

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

Show 5 quoted lines

> 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