| Message ID | 20241129-pks-sign-compare-v1-0-fc406b984bc9@pks.im (mailing list archive) |
|---|---|
| Headers | show |
| Series | Start compiling with `-Wsign-compare` | expand |
On Fri, Nov 29, 2024 at 02:13:21PM +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. > > 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 I have read the whole patches. Only left few comments in one patch. The others are looking good to me. Thanks, Jialuo
On Fri, Nov 29, 2024 at 02:13:21PM +0100, Patrick Steinhardt wrote: > 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. Yeah, this is something I've wanted to do for a long time. Your subject line got me all excited that it was done, so I was a little disappointed to see that it's the start of a long transition. :) Still, I think it is good to start, and the way you've laid it out seems pretty reasonable to me. Regarding those CVEs, I suspect that -Wconversion is at least as interesting there, as it catches direct assignments that may truncate (I think those two were a little more complex, but a common issue is then using the truncated computation to allocate a too-small buffer). But we have to start somewhere, and this may be a more tractable place. The patches themselves looked mostly good to me, though the one with the casts raised my eyebrows a bit (I left further comments there). Thanks for getting the ball rolling on this. -Peff
On Sun, Dec 01, 2024 at 05:29:13PM -0500, Jeff King wrote: > On Fri, Nov 29, 2024 at 02:13:21PM +0100, Patrick Steinhardt wrote: > > > 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. > > Yeah, this is something I've wanted to do for a long time. Your subject > line got me all excited that it was done, so I was a little disappointed > to see that it's the start of a long transition. :) For most of the files it only requires a couple of trivial changes to get things working. But overall we have been quite lenient with how we intermix integer types, so there's simply too many warnings to address in a single step. > Still, I think it is good to start, and the way you've laid it out seems > pretty reasonable to me. > > Regarding those CVEs, I suspect that -Wconversion is at least as > interesting there, as it catches direct assignments that may truncate > (I think those two were a little more complex, but a common issue is > then using the truncated computation to allocate a too-small buffer). Yeah, -Wconversion is on my radar as another step. I hope it will go smoother when we have already addressed -Wsign-compare, as that require us to fix a bunch of the same causes for warnings. We could also try to combine this into a single step, but think it might be easier if we handle these transitions separately. > But we have to start somewhere, and this may be a more tractable place. > The patches themselves looked mostly good to me, though the one with the > casts raised my eyebrows a bit (I left further comments there). Thanks, will have a look. Patrick
On Sat, Nov 30, 2024 at 06:55:39PM +0800, shejialuo wrote: > On Fri, Nov 29, 2024 at 02:13:21PM +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. > > > > 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 > > I have read the whole patches. Only left few comments in one patch. The > others are looking good to me. Thanks for your review! Patrick
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