Skip to content

Commit 2be22c6

Browse files
committed
codeql: disable a couple of non-critical queries for now
A couple of CodeQL's queries are opinionated in a way that is obviously not shared by Git's source code's state, and apparently intentionally so. For example, the "For loop variable changed in body" query as well as the "No trivial switch statements" one result in too many results that are apparently intentional in Git's source code. Let's not worry about those, then. Also, Git has plenty of instances where variables shadow other variables. Other valid yet not quite critical issues identified by CodeQL include complex conditionals and nested switch statements spanning several pages. We probably want to address these issues at some stage, but they are not as critical as other problems pointed out by CodeQL, so let's silence those queries for now and take care of them at a later stage. Signed-off-by: Johannes Schindelin <[email protected]>
1 parent dc89f56 commit 2be22c6

File tree

2 files changed

+133
-2
lines changed

2 files changed

+133
-2
lines changed

.github/codeql/codeql-config.yml

Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,131 @@
1+
name: "CodeQL config"
2+
3+
queries:
4+
- uses: security-extended
5+
6+
query-filters:
7+
- exclude:
8+
# yes, this extra indentation is intentional
9+
# too common in Git's source code
10+
id: cpp/trivial-switch
11+
- exclude:
12+
id: cpp/loop-variable-changed
13+
- exclude:
14+
# we override this locally with a modified version
15+
id: cpp/non-constant-format
16+
- exclude:
17+
# Git does not consider this a problem
18+
id: cpp/irregular-enum-init
19+
- exclude:
20+
# Git has many long functions, this alert would match too many
21+
id: cpp/poorly-documented-function
22+
- exclude:
23+
# In Git, there is a lot of commented-out code
24+
id: cpp/commented-out-code
25+
- exclude:
26+
# While it is true that long switch cases are hard to read and
27+
# validate, Git has way too many for us to allow this query to
28+
# churn out alerts left and right
29+
id: cpp/long-switch
30+
- exclude:
31+
# CodeQL does not expect Git to heed the umask(), but it does
32+
id: cpp/world-writable-file-creation
33+
- exclude:
34+
# Git uses the construct `if (<not this>) ; else ...` often, to
35+
# avoid an extra indentation level. CodeQL does not like that.
36+
id: cpp/empty-block
37+
- exclude:
38+
# This rule unfortunately triggers some false positives, e.g.
39+
# where Git tries to redact URLs or where Git specifically
40+
# asks for a password upon GIT_SSL_CERT_PASSWORD_PROTECTED.
41+
id: cpp/user-controlled-bypass
42+
- exclude:
43+
# This rule fails to recognize that xmallocz() _specifically_
44+
# makes room for a trailing NUL, and instead assumes that this
45+
# function behaves like malloc(), which does not.
46+
id: cpp/invalid-pointer-deref
47+
- exclude:
48+
# CodeQL fails to recognize that xmallocz() accounts for the NUL,
49+
# instead assuming malloc() semantics.
50+
id: cpp/no-space-for-terminator
51+
- exclude:
52+
# Git does exchange plain-text passwords via stdin/stdout e.g.
53+
# with helpers in the credential protocol, or in credential-cache.
54+
# This rule, though, assumes that writing to _any_ file descriptor
55+
# is unsafe.
56+
id: cpp/cleartext-storage-file
57+
- exclude:
58+
# When storing the value of the environment variable `PWD` as the
59+
# current directory in absolute_pathdup(), or when allocating memory
60+
# for a binary patch where the size is specified in the patch itself,
61+
# CodeQL assumes that this can lead to a denial of service because
62+
# of an unbounded size, but Git's code works as designed here.
63+
id: cpp/uncontrolled-allocation-size
64+
- exclude:
65+
# lock_repo_for_gc() has admittedly obtuse logic to parse the
66+
# process ID out of the `gc.pid` file, which is correct, but
67+
# due to its construction throws a false positive here.
68+
id: cpp/missing-check-scanf
69+
- exclude:
70+
# discard_cache_entry() overwrites the name in a FLEX_ARRAY struct
71+
# if GIT_TEST_VALIDATE_INDEX_CACHE_ENTRIES is set, which CodeQL fails
72+
# to recognize as valid.
73+
id: cpp/overrun-write
74+
- exclude:
75+
# Since `time_t` can be signed or unsigned, there is unfortunately
76+
# no way to avoid letting this rule report a potential
77+
id: cpp/integer-multiplication-cast-to-long
78+
- exclude:
79+
# There are many, many legitimate code paths in Git where a path is
80+
# constructed from an environment variable, e.g. GIT_DIR. Let's suppress
81+
# this slightly overzealous query.
82+
id: cpp/path-injection
83+
- exclude:
84+
# Git has 99 instances of this at the time of writing :-(
85+
id: cpp/declaration-hides-variable
86+
- exclude:
87+
id: cpp/declaration-hides-parameter
88+
- exclude:
89+
id: cpp/local-variable-hides-global-variable
90+
- exclude:
91+
id: cpp/complex-condition
92+
- exclude:
93+
# Nested, long-winded switch statements are hard to read and hard
94+
# to reason about. Looking at you, `format_commit_one()`.
95+
id: cpp/complex-block
96+
- exclude:
97+
# There are four instances of this at time of writing, all intentional.
98+
# However, it is very easy to introduce unintentional re-use of loop
99+
# variable names, therefore we will most likely want to either change these
100+
# instances or add suppressions.
101+
id: cpp/nested-loops-with-same-variable
102+
- exclude:
103+
# zOMG so many FIXMEs
104+
id: cpp/fixme-comment
105+
- exclude:
106+
# Git assumes quite a bit about the user's control of the current worktree
107+
# Therefore, it kind of assumes that TOCTOU issues are not a thing when
108+
# it comes to files.
109+
id: cpp/toctou-race-condition
110+
- exclude:
111+
# Too many results in Git where the code was, however, intentionally written
112+
# the way it is.
113+
id: cpp/stack-address-escape
114+
- exclude:
115+
id: cpp/inconsistent-null-check
116+
- exclude:
117+
# This would trigger alerts in the functions in `help.c` that want to open
118+
# external programs to show manual pages.
119+
id: cpp/uncontrolled-process-operation
120+
- exclude:
121+
# The code in t/unit-tests/u-ctype.c implicitly exercises the `sane_istest()`
122+
# macro extensively, and CodeQL seems to miss the cast to `(unsigned char)`,
123+
# thereby mistaking the accesses for being past the end of the array (which
124+
# is incorrect).
125+
#
126+
# Ideally, we would exclude test programs from CodeQL anyways, but
127+
# unfortunately there is no Makefile rule in Git's code base to build only
128+
# the production code, and CodeQL's `paths-ignore` directive described at
129+
# https://docs.github.com/en/code-security/code-scanning/creating-an-advanced-setup-for-code-scanning/customizing-your-advanced-setup-for-code-scanning#specifying-directories-to-scan
130+
# unfortunately is _ignored_ for compiled languages.
131+
id: cpp/overflow-buffer

.github/workflows/codeql.yml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ jobs:
2121

2222
steps:
2323
- name: Checkout repository
24-
uses: actions/checkout@v3
24+
uses: actions/checkout@v4
2525

2626
- name: Install dependencies
2727
run: ci/install-dependencies.sh
@@ -34,7 +34,7 @@ jobs:
3434
uses: github/codeql-action/init@v3
3535
with:
3636
languages: ${{ matrix.language }}
37-
queries: security-extended
37+
config-file: ./.github/codeql/codeql-config.yml
3838

3939
- name: Build
4040
if: matrix.language == 'cpp'

0 commit comments

Comments
 (0)