[v3,00/15] Start compiling with `-Wsign-compare`

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 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 --- Junio C Hamano (1): sign-compare: 32-bit support Patrick Steinhardt (14): git-compat-util: introduce macros to disable "-Wsign-compare" warnings compat/regex: explicitly ignore "-Wsign-compare" warnings compat/win32: fix -Wsign-compare warning in "wWinMain()" global: mark code units that generate warnings with `-Wsign-compare` config.mak.dev: drop `-Wno-sign-compare` diff.h: fix index used to loop through unsigned integer global: trivial conversions to fix `-Wsign-compare` warnings daemon: fix loops that have mismatching integer types daemon: fix type of `max_connections` gpg-interface: address -Wsign-comparison warnings builtin/blame: fix type of `length` variable when emitting object ID builtin/patch-id: fix type of `get_one_patchid()` scalar: address -Wsign-compare warnings t/helper: don't depend on implicit wraparound add-interactive.c | 1 + add-patch.c | 1 + advice.c | 7 ++----- apply.c | 1 + archive.c | 1 + attr.c | 1 + base85.c | 3 +-- bisect.c | 1 + blame.c | 1 + bloom.c | 2 ++ builtin/add.c | 9 +++++---- builtin/am.c | 1 + builtin/bisect.c | 2 ++ builtin/blame.c | 11 +++++++++-- builtin/branch.c | 2 ++ builtin/cat-file.c | 3 +++ builtin/checkout--worker.c | 2 ++ builtin/checkout-index.c | 3 +++ builtin/checkout.c | 2 ++ builtin/clean.c | 3 +++ builtin/clone.c | 3 +++ builtin/commit.c | 3 +++ builtin/describe.c | 2 ++ builtin/diff-files.c | 3 +++ builtin/diff-index.c | 2 ++ builtin/diff-tree.c | 1 + builtin/diff.c | 3 +++ builtin/difftool.c | 5 ++++- builtin/fast-export.c | 3 +++ builtin/fast-import.c | 2 ++ builtin/fetch-pack.c | 2 ++ builtin/fetch.c | 3 +++ builtin/for-each-repo.c | 5 +++-- builtin/fsmonitor--daemon.c | 2 ++ builtin/gc.c | 3 +++ builtin/grep.c | 3 +++ builtin/help.c | 6 +++--- builtin/index-pack.c | 2 ++ builtin/log.c | 3 +++ builtin/ls-files.c | 3 +++ builtin/mailsplit.c | 6 ++++-- builtin/merge-file.c | 2 ++ builtin/merge-index.c | 2 ++ builtin/merge-ours.c | 2 ++ builtin/merge-tree.c | 6 +++--- builtin/merge.c | 3 +++ builtin/mv.c | 2 ++ builtin/name-rev.c | 2 ++ builtin/pack-objects.c | 2 ++ builtin/pack-redundant.c | 4 ++-- builtin/pack-refs.c | 1 + builtin/patch-id.c | 16 +++++++++------- builtin/prune.c | 2 ++ builtin/pull.c | 4 ++-- builtin/push.c | 6 ++++-- builtin/range-diff.c | 1 + builtin/rebase.c | 3 +++ builtin/receive-pack.c | 2 ++ builtin/reflog.c | 1 + builtin/remote.c | 2 ++ builtin/repack.c | 2 ++ builtin/replay.c | 4 +++- builtin/rerere.c | 9 +++++---- builtin/reset.c | 2 ++ builtin/rev-list.c | 2 ++ builtin/rev-parse.c | 3 +++ builtin/revert.c | 1 + builtin/rm.c | 3 +++ builtin/shortlog.c | 1 + builtin/show-branch.c | 2 ++ builtin/show-index.c | 2 ++ builtin/sparse-checkout.c | 2 ++ builtin/stash.c | 7 +++---- builtin/submodule--helper.c | 8 ++++---- builtin/tag.c | 3 +++ builtin/unpack-objects.c | 2 ++ builtin/update-index.c | 3 +++ builtin/update-ref.c | 2 ++ builtin/var.c | 5 +++-- builtin/worktree.c | 2 ++ bulk-checkin.c | 1 + bundle-uri.c | 1 + bundle.c | 1 + cache-tree.c | 1 + chunk-format.c | 1 + color.c | 2 ++ column.c | 2 ++ combine-diff.c | 1 + commit-graph.c | 1 + commit-reach.c | 1 + commit.c | 3 +-- compat/fsmonitor/fsm-listen-darwin.c | 3 +-- compat/mingw.c | 1 + compat/poll/poll.c | 2 ++ compat/regex/regex.c | 2 ++ compat/terminal.c | 3 +-- compat/win32/headless.c | 17 +++++++++-------- compat/win32mmap.c | 2 ++ compat/winansi.c | 2 ++ config.c | 1 + config.mak.dev | 1 - connect.c | 1 + convert.c | 1 + credential.c | 1 + csum-file.c | 2 +- daemon.c | 31 +++++++++++++------------------ date.c | 2 ++ decorate.c | 3 +++ delta-islands.c | 1 + diagnose.c | 7 +++---- diff-delta.c | 2 ++ diff-lib.c | 1 + diff-no-index.c | 2 ++ diff.c | 1 + diff.h | 3 +-- diffcore-order.c | 1 + diffcore-pickaxe.c | 3 +++ diffcore-rename.c | 3 +-- diffcore-rotate.c | 1 + dir.c | 1 + entry.c | 4 ++-- ewah/ewah_bitmap.c | 5 ++--- ewah/ewah_io.c | 3 +++ ewah/ewah_rlw.c | 3 +++ fetch-pack.c | 1 + fmt-merge-msg.c | 1 + fsmonitor.c | 1 + gettext.c | 2 ++ git-compat-util.h | 10 ++++++++++ git.c | 20 +++++++------------- gpg-interface.c | 14 ++++++-------- graph.c | 1 + grep.c | 2 ++ help.c | 1 + help.h | 8 +++----- hex.c | 6 ++---- http-backend.c | 1 + http-push.c | 3 +-- http-walker.c | 1 + http.c | 1 + imap-send.c | 1 + json-writer.c | 2 ++ kwset.c | 2 ++ line-log.c | 2 ++ list-objects-filter-options.c | 4 +--- list-objects.c | 7 ++----- log-tree.c | 1 + ls-refs.c | 4 +--- mailinfo.c | 1 + mailmap.c | 1 + match-trees.c | 1 + mem-pool.c | 2 ++ merge-ll.c | 1 + merge-ort.c | 1 + merge-recursive.c | 1 + merge.c | 4 ++-- midx-write.c | 1 + midx.c | 1 + name-hash.c | 1 + notes-merge.c | 1 + notes.c | 1 + object-file-convert.c | 1 + object-file.c | 1 + object-name.c | 1 + object.c | 1 + pack-bitmap-write.c | 1 + pack-bitmap.c | 1 + pack-check.c | 1 + packfile.c | 1 + parallel-checkout.c | 1 + path.c | 4 ++-- pathspec.c | 1 + pkt-line.c | 5 ++--- preload-index.c | 1 + pretty.c | 1 + progress.c | 1 + pseudo-merge.c | 1 + quote.c | 2 ++ range-diff.c | 1 + read-cache.c | 1 + ref-filter.c | 1 + reflog.c | 1 + refs.c | 1 + refs/debug.c | 3 +-- refs/files-backend.c | 1 + refs/iterator.c | 2 ++ refs/packed-backend.c | 1 + refspec.c | 1 + reftable/system.h | 2 ++ remote-curl.c | 1 + remote.c | 1 + rerere.c | 1 + resolve-undo.c | 1 + revision.c | 1 + run-command.c | 1 + scalar.c | 6 +++--- send-pack.c | 5 ++--- sequencer.c | 1 + serve.c | 7 ++----- server-info.c | 1 + setup.c | 1 + shallow.c | 1 + sideband.c | 1 + sparse-index.c | 1 + split-index.c | 1 + strbuf.c | 2 ++ string-list.c | 2 ++ strvec.c | 3 +-- submodule-config.c | 1 + submodule.c | 1 + symlinks.c | 2 ++ t/helper/test-bloom.c | 9 ++------- t/helper/test-cache-tree.c | 1 + t/helper/test-config.c | 1 + t/helper/test-csprng.c | 3 +-- t/helper/test-drop-caches.c | 2 ++ t/helper/test-dump-fsmonitor.c | 3 +-- t/helper/test-dump-split-index.c | 3 +-- t/helper/test-dump-untracked-cache.c | 6 +++--- t/helper/test-genrandom.c | 2 +- t/helper/test-genzeros.c | 2 ++ t/helper/test-hash-speed.c | 5 ++--- t/helper/test-mergesort.c | 2 ++ t/helper/test-parse-options.c | 5 ++--- t/helper/test-path-utils.c | 1 + t/helper/test-reach.c | 3 +-- t/helper/test-ref-store.c | 3 +-- t/helper/test-run-command.c | 2 ++ t/helper/test-string-list.c | 2 ++ t/helper/test-tool.c | 3 +-- t/helper/test-trace2.c | 1 + t/unit-tests/lib-reftable.c | 2 ++ t/unit-tests/t-example-decorate.c | 4 ++-- t/unit-tests/t-prio-queue.c | 2 +- t/unit-tests/t-reftable-readwrite.c | 2 ++ t/unit-tests/t-reftable-stack.c | 2 ++ t/unit-tests/t-trailer.c | 2 ++ t/unit-tests/test-lib.c | 2 ++ tag.c | 1 + tmp-objdir.c | 3 +-- trace.c | 1 + trace2.c | 2 ++ trace2/tr2_sysenv.c | 2 ++ trace2/tr2_tgt_event.c | 2 ++ trace2/tr2_tgt_perf.c | 2 ++ trailer.c | 3 +-- transport-helper.c | 13 ++++++------- transport.c | 13 ++++--------- tree-diff.c | 1 + unix-socket.c | 2 ++ unpack-trees.c | 1 + upload-pack.c | 1 + urlmatch.c | 2 ++ usage.c | 3 ++- userdiff.c | 1 + utf8.c | 2 ++ version.c | 3 +-- versioncmp.c | 3 +-- worktree.c | 1 + wrapper.c | 3 +++ ws.c | 3 +++ wt-status.c | 1 + xdiff-interface.c | 1 + xdiff/xdiffi.c | 1 + xdiff/xinclude.h | 2 ++ 265 files changed, 503 insertions(+), 221 deletions(-) Range-diff versus v2: 1: c5d30a71fb = 1: f3866ee766 git-compat-util: introduce macros to disable "-Wsign-compare" warnings 2: e320657277 = 2: 5b694e2b06 compat/regex: explicitly ignore "-Wsign-compare" warnings 3: a5b71acdf3 = 3: 2d04ae0627 compat/win32: fix -Wsign-compare warning in "wWinMain()" 4: fdd24ef3dc ! 4: 1ef0944284 global: mark code units that generate warnings with `-Wsign-compare` @@ credential.c #include "git-compat-util.h" #include "abspath.h" + ## csum-file.c ## +@@ + */ + + #define USE_THE_REPOSITORY_VARIABLE ++#define DISABLE_SIGN_COMPARE_WARNINGS + + #include "git-compat-util.h" + #include "progress.h" + ## daemon.c ## @@ #define USE_THE_REPOSITORY_VARIABLE 5: 547ca812ec = 5: 678673ffc8 config.mak.dev: drop `-Wno-sign-compare` 6: 53615491cd = 6: 5342906326 diff.h: fix index used to loop through unsigned integer -: ---------- > 7: be0a36edbe sign-compare: 32-bit support 7: 5220623358 = 8: ac39ac5d02 global: trivial conversions to fix `-Wsign-compare` warnings 8: 9096129c17 = 9: c7fbaf15e8 daemon: fix loops that have mismatching integer types 9: add7fca26e = 10: cfb144047d daemon: fix type of `max_connections` 10: ac1cd2564d = 11: 2b8171aa1e gpg-interface: address -Wsign-comparison warnings 11: 6bab14c587 = 12: 2e5bd44217 builtin/blame: fix type of `length` variable when emitting object ID 12: 6b6a63b276 ! 13: ff78b07003 builtin/patch-id: fix type of `get_one_patchid()` @@ Commit message In `get_one_patchid()` we assign either the result of `strlen()` or `remove_space()` to `len`. But while the former correctly returns a `size_t`, the latter returns an `int` to indicate the length of the - string even though it cannot ever return a negative value. This causes a - warning with "-Wsign-conversion". + stripped string even though it cannot ever return a negative value. This + causes a warning with "-Wsign-conversion". In fact, even `get_one_patchid()` itself is also using an integer as return value even though it always returns the length of the patch, and 13: 2a66935837 = 14: 7fab5dd6ab scalar: address -Wsign-compare warnings 14: 8a846a0c13 = 15: 7f02e005ef t/helper: don't depend on implicit wraparound --- base-commit: cc01bad4a9f566cf4453c7edd6b433851b0835e2 change-id: 20241128-pks-sign-compare-9cf7412bce1a

Message ID	20241205-pks-sign-compare-v3-0-e211ee089717@pks.im (mailing list archive)
Headers	show Received: from fhigh-a6-smtp.messagingengine.com (fhigh-a6-smtp.messagingengine.com [103.168.172.157]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 368711A8F84 for <git@vger.kernel.org>; Thu, 5 Dec 2024 09:36:50 +0000 (UTC) Feedback-ID: i197146af:Fastmail From: Patrick Steinhardt <ps@pks.im> Subject: [PATCH v3 00/15] Start compiling with `-Wsign-compare` Date: Thu, 05 Dec 2024 10:36:22 +0100 Message-Id: <20241205-pks-sign-compare-v3-0-e211ee089717@pks.im> Precedence: bulk MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit In-Reply-To: <20241129-pks-sign-compare-v1-0-fc406b984bc9@pks.im> References: <20241129-pks-sign-compare-v1-0-fc406b984bc9@pks.im> To: git@vger.kernel.org Cc: shejialuo <shejialuo@gmail.com>, Jeff King <peff@peff.net>, Junio C Hamano <gitster@pobox.com>
Series	Start compiling with `-Wsign-compare` \| expand [v3,00/15] Start compiling with `-Wsign-compare` [v3,01/15] git-compat-util: introduce macros to disable "-Wsign-compare" warnings [v3,02/15] compat/regex: explicitly ignore "-Wsign-compare" warnings [v3,03/15] compat/win32: fix -Wsign-compare warning in "wWinMain()" [v3,04/15] global: mark code units that generate warnings with `-Wsign-compare` [v3,05/15] config.mak.dev: drop `-Wno-sign-compare` [v3,06/15] diff.h: fix index used to loop through unsigned integer [v3,07/15] sign-compare: 32-bit support [v3,08/15] global: trivial conversions to fix `-Wsign-compare` warnings [v3,09/15] daemon: fix loops that have mismatching integer types [v3,10/15] daemon: fix type of `max_connections` [v3,11/15] gpg-interface: address -Wsign-comparison warnings [v3,12/15] builtin/blame: fix type of `length` variable when emitting object ID [v3,13/15] builtin/patch-id: fix type of `get_one_patchid()` [v3,14/15] scalar: address -Wsign-compare warnings [v3,15/15] t/helper: don't depend on implicit wraparound

[v3,00/15] Start compiling with `-Wsign-compare`

Message