Skip to content

Commit

Permalink
Start compiling with -Wsign-compare
Browse files Browse the repository at this point in the history
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

Changes in v3:

  - Include Junio's fix for 32 bit platforms.

  - Improve a commit message.

  - Link to v2: https://lore.kernel.org/r/20241202-pks-sign-compare-v2-0-e7f0ad92a749@pks.im

Changes in v4:

  - Remove needless cast in "csum-file.c".

  - Refactor "pkt-line.c" to be safer regarding integer conversions.

  - Link to v3: https://lore.kernel.org/r/20241205-pks-sign-compare-v3-0-e211ee089717@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
series. Let me know in case you disagree and I'll change the base of
this series.

Thanks!

Patrick

To: git@vger.kernel.org
Cc: shejialuo <shejialuo@gmail.com>
Cc: Jeff King <peff@peff.net>
Cc: Junio C Hamano <gitster@pobox.com>

--- b4-submit-tracking ---
# This section is used internally by b4 prep for tracking purposes.
{
  "series": {
    "revision": 4,
    "change-id": "20241128-pks-sign-compare-9cf7412bce1a",
    "prefixes": [],
    "history": {
      "v1": [
        "20241129-pks-sign-compare-v1-0-fc406b984bc9@pks.im"
      ],
      "v2": [
        "20241202-pks-sign-compare-v2-0-e7f0ad92a749@pks.im"
      ],
      "v3": [
        "20241205-pks-sign-compare-v3-0-e211ee089717@pks.im"
      ]
    }
  }
}
  • Loading branch information
pks-t committed Nov 28, 2024
1 parent cc01bad commit 83ae1ed
Showing 0 changed files with 0 additions and 0 deletions.

0 comments on commit 83ae1ed

Please sign in to comment.