Re: [PATCH 00/10] Start compiling with `-Wsign-compare`

[Patrick Steinhardt <ps@pks.im>]

On Sun, Dec 01, 2024 at 05:29:13PM -0500, Jeff King wrote:
> On Fri, Nov 29, 2024 at 02:13:21PM +0100, Patrick Steinhardt wrote:
> 
> > when compiling with DEVELOPER=YesPlease, we explicitly disable the
> > "-Wsign-compare" warning. This is mostly because our code base is full
> > of cases where we don't bother at all whether something should be signed
> > or unsigned, and enabling the warning would thus cause tons of warnings
> > to pop up.
> > 
> > Unfortunately, disabling this warning also masks real issues. There have
> > been multiple CVEs in the Git project that would have been flagged by
> > this warning (e.g. CVE-2022-39260, CVE-2022-41903 and several fixes in
> > the vicinity of these CVEs). Furthermore, the final audit report by
> > X41 D-Sec, who are the ones who have discovered some of the CVEs, hinted
> > that it might be a good idea to become more strict in this context.
> 
> Yeah, this is something I've wanted to do for a long time. Your subject
> line got me all excited that it was done, so I was a little disappointed
> to see that it's the start of a long transition. :)

For most of the files it only requires a couple of trivial changes to
get things working. But overall we have been quite lenient with how we
intermix integer types, so there's simply too many warnings to address
in a single step.

> Still, I think it is good to start, and the way you've laid it out seems
> pretty reasonable to me.
> 
> Regarding those CVEs, I suspect that -Wconversion is at least as
> interesting there, as it catches direct assignments that may truncate
> (I think those two were a little more complex, but a common issue is
> then using the truncated computation to allocate a too-small buffer).

Yeah, -Wconversion is on my radar as another step. I hope it will go
smoother when we have already addressed -Wsign-compare, as that require
us to fix a bunch of the same causes for warnings.

We could also try to combine this into a single step, but think it might
be easier if we handle these transitions separately.

> But we have to start somewhere, and this may be a more tractable place.
> The patches themselves looked mostly good to me, though the one with the
> casts raised my eyebrows a bit (I left further comments there).

Thanks, will have a look.

Patrick