Re: [PATCH] connect: address -Wsign-compare warnings

[Patrick Steinhardt <ps@pks.im>]

On Sat, Jan 18, 2025 at 06:18:30AM +0900, Mike Hommey wrote:
> On Fri, Jan 17, 2025 at 09:26:32AM -0800, Junio C Hamano wrote:
> > Mike Hommey <mh@glandium.org> writes:
> > 
> > > Most of the warnings were about loop variables being declared as ints
> > > with a condition using a size_t, whereby switching the variable to
> > > size_t fixes the warning.
> > >
> > > One other case was comparing the result of strlen to an int passed
> > > as an argument, which turns out could just as well be passed as a
> > > size_t, albeit trickling to other functions.
> > 
> > As long as the blast radius is limited (like this one, which most of
> > the cascades were within the callchain of file-scope statics), and
> > the changes of type is going in the right direction (in this case, I
> > see all are using size_t for length that may come from or compared
> > with the result of strlen(), which falls into that category), such a
> > change is very much welcomed.
> > 
> > Even if the primary objective is to squelch the -Wsign-compare and
> > even if we are talking about a line in packet_reader object, which
> > would not exceed 64k bytes and using size_t is way overkill, that
> > is.  I personally do not think -Wsign-compare cleanliness is buying
> > us all that much, compared to the amount of code churn.  But this
> > one is well within the level that I can tolerate ;-).

It does generate quite a bit of churn indeed. But it also made us look a
lot closer in many places where such warnings are generated, and we
found multiple sites already where unexpected values can cause us to do
weird stuff, including going out of bounds. So if this allows us to
detect (or even better avoid introducing) even a single out-of-bounds
read/write that can be exploited I'm happy.

I think overall it's going to be a net win in the long term as it forces
us to think more carefullly about types, which we haven't really been
doing until now. And this is a frequent observation during code reviews,
so it also gets a tiny fraction of reviewer's time back.

> Do you want me to address Patrick's comments?

I don't mind it too much, the end result would be the same anyway. Just
keep it in mind for future patch series.

Patrick