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

From: Jeff King <peff@peff.net>
Date: Dec 1, 2024, 22:29 UTC
Message-ID: <20241201222913.GH145938@coredump.intra.peff.net>
In-Reply-To: <20241129-pks-sign-compare-v1-0-fc406b984bc9@pks.im>

On Fri, Nov 29, 2024 at 02:13:21PM +0100, Patrick Steinhardt wrote:

Show 12 quoted lines

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

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

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 for getting the ball rolling on this.

-Peff