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

[shejialuo <shejialuo@gmail.com>]

On Mon, Dec 02, 2024 at 01:04:32PM +0100, Patrick Steinhardt wrote:
> Hi,
> 
> 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.
> 
> Now simply enabling the warning globally does not fly due to the stated
> reason above that we simply have too many sites where we use the wrong
> integer types. Instead, this patch series introduces a new macro that
> allows us to explicitly mark files that generate such warnings. Like
> this, we can adapt the codebase over time and hopefully make this class
> of vulnerabilities harder to land.
> 
> Changes in v2:
> 
>   - Explode the 6th patch into multiple patches with more careful
>     analysis and refactorings
> 
>   - Drop the conversion of "gettext.c". To do it properly we'd have to
>     fix the return type of `utf8_strwidth()`, which we have already
>     marked as a todo.
> 
>   - Link to v1: https://lore.kernel.org/r/20241129-pks-sign-compare-v1-0-fc406b984bc9@pks.im
> 
> There are a couple of trivial conflicts with kn/midx-wo-the-repository,
> but I don't think it makes sense to make that a dependency of this
> sereis. Let me know in case you disagree and I'll change the base of
> this series.
> 
> Thanks!
> 
> Patrick

The range-diff looks good to me.