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.
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
---
Patrick Steinhardt (10):
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`
global: fix unsigned integer promotions in ternary statements
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`
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 | 5 ++++-
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 | 3 ++-
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 +
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 | 5 +++--
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 | 3 +--
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 | 4 +++-
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 | 2 +-
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 ++
264 files changed, 491 insertions(+), 215 deletions(-)
---
base-commit: cc01bad4a9f566cf4453c7edd6b433851b0835e2
change-id: 20241128-pks-sign-compare-9cf7412bce1a
