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@xxxxxx
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@xxxxxx
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
