Merge branch 'codeql'

This patch series has been long in the making, ever since Johannes
Nicolai and myself spiked this in November/December 2020.

Signed-off-by: Johannes Schindelin <[email protected]>

Author: dscho

.github/codeql/codeql-config.yml

@@ -0,0 +1,134 @@
+name: "CodeQL config"
+
+queries:
+  - uses: security-extended
+
+paths-ignore:
+  - gitweb/**/*.js # GitWeb is not distributed
+
+query-filters:
+  - exclude:
+    # yes, this extra indentation is intentional
+      # too common in Git's source code
+      id: cpp/trivial-switch
+  - exclude:
+      id: cpp/loop-variable-changed
+  - exclude:
+      # we override this locally with a modified version
+      id: cpp/non-constant-format
+  - exclude:
+      # Git does not consider this a problem
+      id: cpp/irregular-enum-init
+  - exclude:
+      # Git has many long functions, this alert would match too many
+      id: cpp/poorly-documented-function
+  - exclude:
+      # In Git, there is a lot of commented-out code
+      id: cpp/commented-out-code
+  - exclude:
+      # While it is true that long switch cases are hard to read and
+      # validate, Git has way too many for us to allow this query to
+      # churn out alerts left and right
+      id: cpp/long-switch
+  - exclude:
+      # CodeQL does not expect Git to heed the umask(), but it does
+      id: cpp/world-writable-file-creation
+  - exclude:
+      # Git uses the construct `if (<not this>) ; else ...` often, to
+      # avoid an extra indentation level. CodeQL does not like that.
+      id: cpp/empty-block
+  - exclude:
+      # This rule unfortunately triggers some false positives, e.g.
+      # where Git tries to redact URLs or where Git specifically
+      # asks for a password upon GIT_SSL_CERT_PASSWORD_PROTECTED.
+      id: cpp/user-controlled-bypass
+  - exclude:
+      # This rule fails to recognize that xmallocz() _specifically_
+      # makes room for a trailing NUL, and instead assumes that this
+      # function behaves like malloc(), which does not.
+      id: cpp/invalid-pointer-deref
+  - exclude:
+      # CodeQL fails to recognize that xmallocz() accounts for the NUL,
+      # instead assuming malloc() semantics.
+      id: cpp/no-space-for-terminator
+  - exclude:
+      # Git does exchange plain-text passwords via stdin/stdout e.g.
+      # with helpers in the credential protocol, or in credential-cache.
+      # This rule, though, assumes that writing to _any_ file descriptor
+      # is unsafe.
+      id: cpp/cleartext-storage-file
+  - exclude:
+      # When storing the value of the environment variable `PWD` as the
+      # current directory in absolute_pathdup(), or when allocating memory
+      # for a binary patch where the size is specified in the patch itself,
+      # CodeQL assumes that this can lead to a denial of service because
+      # of an unbounded size, but Git's code works as designed here.
+      id: cpp/uncontrolled-allocation-size
+  - exclude:
+      # lock_repo_for_gc() has admittedly obtuse logic to parse the
+      # process ID out of the `gc.pid` file, which is correct, but
+      # due to its construction throws a false positive here.
+      id: cpp/missing-check-scanf
+  - exclude:
+      # discard_cache_entry() overwrites the name in a FLEX_ARRAY struct
+      # if GIT_TEST_VALIDATE_INDEX_CACHE_ENTRIES is set, which CodeQL fails
+      # to recognize as valid.
+      id: cpp/overrun-write
+  - exclude:
+      # Since `time_t` can be signed or unsigned, there is unfortunately
+      # no way to avoid letting this rule report a potential
+      id: cpp/integer-multiplication-cast-to-long
+  - exclude:
+      # There are many, many legitimate code paths in Git where a path is
+      # constructed from an environment variable, e.g. GIT_DIR. Let's suppress
+      # this slightly overzealous query.
+      id: cpp/path-injection
+  - exclude:
+      # Git has 99 instances of this at the time of writing :-(
+      id: cpp/declaration-hides-variable
+  - exclude:
+      id: cpp/declaration-hides-parameter
+  - exclude:
+      id: cpp/local-variable-hides-global-variable
+  - exclude:
+      id: cpp/complex-condition
+  - exclude:
+      # Nested, long-winded switch statements are hard to read and hard
+      # to reason about. Looking at you, `format_commit_one()`.
+      id: cpp/complex-block
+  - exclude:
+      # There are four instances of this at time of writing, all intentional.
+      # However, it is very easy to introduce unintentional re-use of loop
+      # variable names, therefore we will most likely want to either change these
+      # instances or add suppressions.
+      id: cpp/nested-loops-with-same-variable
+  - exclude:
+      # zOMG so many FIXMEs
+      id: cpp/fixme-comment
+  - exclude:
+      # Git assumes quite a bit about the user's control of the current worktree
+      # Therefore, it kind of assumes that TOCTOU issues are not a thing when
+      # it comes to files.
+      id: cpp/toctou-race-condition
+  - exclude:
+      # Too many results in Git where the code was, however, intentionally written
+      # the way it is.
+      id: cpp/stack-address-escape
+  - exclude:
+      id: cpp/inconsistent-null-check
+  - exclude:
+      # This would trigger alerts in the functions in `help.c` that want to open
+      # external programs to show manual pages.
+      id: cpp/uncontrolled-process-operation
+  - exclude:
+      # The code in t/unit-tests/u-ctype.c implicitly exercises the `sane_istest()`
+      # macro extensively, and CodeQL seems to miss the cast to `(unsigned char)`,
+      # thereby mistaking the accesses for being past the end of the array (which
+      # is incorrect).
+      #
+      # Ideally, we would exclude test programs from CodeQL anyways, but
+      # unfortunately there is no Makefile rule in Git's code base to build only
+      # the production code, and CodeQL's `paths-ignore` directive described at
+      # 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
+      # unfortunately is _ignored_ for compiled languages.
+      id: cpp/overflow-buffer

.github/workflows/codeql.yml

@@ -0,0 +1,64 @@
+name: "CodeQL"
+
+on:
+  push:
+  pull_request:
+  workflow_dispatch:
+
+jobs:
+  analyze:
+    name: Analyze
+    runs-on: ubuntu-latest
+    permissions:
+      actions: read
+      contents: read
+      security-events: write
+
+    strategy:
+      fail-fast: false
+      matrix:
+        language: ["cpp", "javascript"]
+
+    steps:
+      - name: Checkout repository
+        uses: actions/checkout@v4
+
+      - name: Install dependencies
+        run: ci/install-dependencies.sh
+        if: matrix.language == 'cpp'
+        env:
+          jobname: codeql
+
+      # Initializes the CodeQL tools for scanning.
+      - name: Initialize CodeQL
+        uses: github/codeql-action/init@v3
+        with:
+          languages: ${{ matrix.language }}
+          config-file: ./.github/codeql/codeql-config.yml
+
+      - name: Build
+        if: matrix.language == 'cpp'
+        run: |
+          cat /proc/cpuinfo
+          make -j$(nproc)
+
+      - name: Perform CodeQL Analysis
+        uses: github/codeql-action/analyze@v3
+        with:
+          upload: False
+          output: sarif-results
+
+      - name: debug
+        shell: bash
+        run: ls -la sarif-results
+
+      - name: publish sarif for debugging
+        uses: actions/upload-artifact@v4
+        with:
+          name: sarif-results-${{ matrix.language }}
+          path: sarif-results
+
+      - name: Upload SARIF
+        uses: github/codeql-action/upload-sarif@v3
+        with:
+         sarif_file: sarif-results/${{ matrix.language }}.sarif

.gitignore

@@ -258,3 +258,5 @@ Release/
 CMakeSettings.json
 /contrib/libgit-rs/target
 /contrib/libgit-sys/target
+/.github/codeql/.cache/
+/.github/codeql/codeql-pack.lock.yml

branch.c

@@ -224,6 +224,8 @@ static int inherit_tracking(struct tracking *tracking, const char *orig_ref)
 	skip_prefix(orig_ref, "refs/heads/", &bare_ref);
 
 	branch = branch_get(bare_ref);
+	if (!branch)
+		BUG("could not get branch for '%s", bare_ref);
 	if (!branch->remote_name) {
 		warning(_("asked to inherit tracking from '%s', but no remote is set"),
 			bare_ref);

builtin/am.c

@@ -434,33 +434,33 @@ static void am_load(struct am_state *state)
 	}
 
 	read_state_file(&sb, state, "keep", 1);
-	if (!strcmp(sb.buf, "t"))
+	if (!strcmp(sb.buf, "t")) // CodeQL [SM01932] justification: CodeQL is wrong here because the value is read from a file via strbuf_read() which does NUL-terminate the string, something CodeQL fails to understand
 		state->keep = KEEP_TRUE;
-	else if (!strcmp(sb.buf, "b"))
+	else if (!strcmp(sb.buf, "b")) // CodeQL [SM01932] justification: CodeQL is wrong here because the value is read from a file via strbuf_read() which does NUL-terminate the string, something CodeQL fails to understand
 		state->keep = KEEP_NON_PATCH;
 	else
 		state->keep = KEEP_FALSE;
 
 	read_state_file(&sb, state, "messageid", 1);
-	state->message_id = !strcmp(sb.buf, "t");
+	state->message_id = !strcmp(sb.buf, "t"); // CodeQL [SM01932] justification: CodeQL is wrong here because the value is read from a file via strbuf_read() which does NUL-terminate the string, something CodeQL fails to understand
 
 	read_state_file(&sb, state, "scissors", 1);
-	if (!strcmp(sb.buf, "t"))
+	if (!strcmp(sb.buf, "t")) // CodeQL [SM01932] justification: CodeQL is wrong here because the value is read from a file via strbuf_read() which does NUL-terminate the string, something CodeQL fails to understand
 		state->scissors = SCISSORS_TRUE;
-	else if (!strcmp(sb.buf, "f"))
+	else if (!strcmp(sb.buf, "f")) // CodeQL [SM01932] justification: CodeQL is wrong here because the value is read from a file via strbuf_read() which does NUL-terminate the string, something CodeQL fails to understand
 		state->scissors = SCISSORS_FALSE;
 	else
 		state->scissors = SCISSORS_UNSET;
 
 	read_state_file(&sb, state, "quoted-cr", 1);
 	if (!*sb.buf)
 		state->quoted_cr = quoted_cr_unset;
-	else if (mailinfo_parse_quoted_cr_action(sb.buf, &state->quoted_cr) != 0)
+	else if (mailinfo_parse_quoted_cr_action(sb.buf, &state->quoted_cr) != 0) // CodeQL [SM01932] justification: CodeQL is wrong here because the value is read from a file via strbuf_read() which does NUL-terminate the string, something CodeQL fails to understand
 		die(_("could not parse %s"), am_path(state, "quoted-cr"));
 
 	read_state_file(&sb, state, "apply-opt", 1);
 	strvec_clear(&state->git_apply_opts);
-	if (sq_dequote_to_strvec(sb.buf, &state->git_apply_opts) < 0)
+	if (sq_dequote_to_strvec(sb.buf, &state->git_apply_opts) < 0) // CodeQL [SM01932] justification: CodeQL is wrong here because the value is read from a file via strbuf_read() which does NUL-terminate the string, something CodeQL fails to understand
 		die(_("could not parse %s"), am_path(state, "apply-opt"));
 
 	state->rebasing = !!file_exists(am_path(state, "rebasing"));

builtin/cat-file.c

@@ -106,7 +106,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)
 	struct object_id oid;
 	enum object_type type;
 	char *buf;
-	unsigned long size;
+	unsigned long size = 0;
 	struct object_context obj_context = {0};
 	struct object_info oi = OBJECT_INFO_INIT;
 	unsigned flags = OBJECT_INFO_LOOKUP_REPLACE;

builtin/clone.c

@@ -120,7 +120,7 @@ static const char *get_repo_path_1(struct strbuf *path, int *is_bundle)
 				continue;
 			len = read_in_full(fd, signature, 8);
 			close(fd);
-			if (len != 8 || strncmp(signature, "gitdir: ", 8))
+			if (len != 8 || strncmp(signature, "gitdir: ", 8)) // CodeQL [SM01932] justification: CodeQL is wrong here because the value is read from a file via strbuf_read() which does NUL-terminate the string, something CodeQL fails to understand
 				continue;
 			dst = read_gitfile(path->buf);
 			if (dst) {

builtin/commit.c

@@ -2104,7 +2104,7 @@ int cmd_commit(int argc,
 		if (!stat(git_path_merge_mode(the_repository), &statbuf)) {
 			if (strbuf_read_file(&sb, git_path_merge_mode(the_repository), 0) < 0)
 				die_errno(_("could not read MERGE_MODE"));
-			if (!strcmp(sb.buf, "no-ff"))
+			if (!strcmp(sb.buf, "no-ff")) // CodeQL [SM01932] justification: CodeQL is wrong here because the value is read from a file via strbuf_read() which does NUL-terminate the string, something CodeQL fails to understand
 				allow_fast_forward = 0;
 		}
 		if (allow_fast_forward)

builtin/describe.c

@@ -324,6 +324,8 @@ static void describe_commit(struct object_id *oid, struct strbuf *dst)
 	unsigned int unannotated_cnt = 0;
 
 	cmit = lookup_commit_reference(the_repository, oid);
+	if (!cmit)
+		die(_("could not look up commit '%s'"), oid_to_hex(oid));
 
 	n = find_commit_name(&cmit->object.oid);
 	if (n && (tags || all || n->prio == 2)) {

builtin/fetch.c

@@ -555,7 +555,7 @@ static struct ref *get_ref_map(struct remote *remote,
 		if (remote &&
 		    (remote->fetch.nr ||
 		     /* Note: has_merge implies non-NULL branch->remote_name */
-		     (has_merge && !strcmp(branch->remote_name, remote->name)))) {
+		     (has_merge && branch && !strcmp(branch->remote_name, remote->name)))) {
 			for (i = 0; i < remote->fetch.nr; i++) {
 				get_fetch_map(remote_refs, &remote->fetch.items[i], &tail, 0);
 				if (remote->fetch.items[i].dst &&
@@ -573,6 +573,7 @@ static struct ref *get_ref_map(struct remote *remote,
 			 * Note: has_merge implies non-NULL branch->remote_name
 			 */
 			if (has_merge &&
+			    branch &&
 			    !strcmp(branch->remote_name, remote->name))
 				add_merge_config(&ref_map, remote_refs, branch, &tail);
 		} else if (!prefetch) {
@@ -2580,6 +2581,11 @@ int cmd_fetch(int argc,
 			die(_("must supply remote when using --negotiate-only"));
 		gtransport = prepare_transport(remote, 1);
 		if (gtransport->smart_options) {
+			/*
+			 * Intentionally assign the address of a local variable
+			 * to a non-local struct's field.
+			 * codeql[cpp/stack-address-escape]
+			 */
 			gtransport->smart_options->acked_commits = &acked_commits;
 		} else {
 			warning(_("protocol does not support --negotiate-only, exiting"));