Run CodeQL as part of the CI

.github/codeql/codeql-config.yml

@@ -0,0 +1,131 @@
+name: "CodeQL config"
+
+queries:
+  - uses: security-extended
+
+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"]
+
+    steps:
+      - name: Checkout repository
+        uses: actions/checkout@v3
+
+      - 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
+          path: sarif-results
+
+      - name: Upload SARIF
+        uses: github/codeql-action/upload-sarif@v3
+        with:
+         sarif_file: sarif-results/cpp.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/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/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) {
@@ -2563,6 +2564,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"));

builtin/push.c

@@ -90,7 +90,7 @@ static void refspec_append_mapped(struct refspec *refspec, const char *ref,
 	if (push_default == PUSH_DEFAULT_UPSTREAM &&
 	    skip_prefix(matched->name, "refs/heads/", &branch_name)) {
 		struct branch *branch = branch_get(branch_name);
-		if (branch->merge_nr == 1 && branch->merge[0]->src) {
+		if (branch && branch->merge_nr == 1 && branch->merge[0]->src) {
 			refspec_appendf(refspec, "%s:%s",
 					ref, branch->merge[0]->src);
 			return;

builtin/stash.c

@@ -284,7 +284,7 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)
 	memset(&opts, 0, sizeof(opts));
 
 	tree = parse_tree_indirect(i_tree);
-	if (parse_tree(tree))
+	if (!tree || parse_tree(tree))
 		return -1;
 
 	init_tree_desc(t, &tree->object.oid, tree->buffer, tree->size);
@@ -1395,6 +1395,11 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b
 		goto done;
 	} else {
 		head_commit = lookup_commit(the_repository, &info->b_commit);
+		if (!head_commit) {
+			ret = error(_("could not look up commit '%s'"),
+				    oid_to_hex (&info->b_commit));
+			goto done;
+		}
 	}
 
 	if (!check_changes(ps, include_untracked, &untracked_files)) {

builtin/submodule--helper.c

@@ -1934,6 +1934,9 @@ static int determine_submodule_update_strategy(struct repository *r,
 	const char *val;
 	int ret;
 
+	if (!sub)
+		return error(_("could not retrieve submodule information for path '%s'"), path);
+
 	key = xstrfmt("submodule.%s.update", sub->name);
 
 	if (update) {

ci/install-dependencies.sh

@@ -119,7 +119,7 @@ ClangFormat)
 	sudo apt-get -q update
 	sudo apt-get -q -y install clang-format
 	;;
-StaticAnalysis)
+StaticAnalysis|codeql)
 	sudo apt-get -q update
 	sudo apt-get -q -y install coccinelle libcurl4-openssl-dev libssl-dev \
 		libexpat-dev gettext make

commit-graph.c

@@ -2560,6 +2560,7 @@ int write_commit_graph(struct object_directory *odb,
 		struct commit_graph *g = ctx.r->objects->commit_graph;
 
 		while (g) {
+			/* Intentional: codeql[cpp/stack-address-escape] */
 			g->topo_levels = &topo_levels;
 			g = g->base_graph;
 		}
@@ -2796,6 +2797,11 @@ static int verify_one_commit_graph(struct repository *r,
 			the_repository->hash_algo);
 
 		graph_commit = lookup_commit(r, &cur_oid);
+		if (!graph_commit) {
+			graph_report(_("failed to look up commit %s for commit-graph"),
+				     oid_to_hex(&cur_oid));
+			continue;
+		}
 		odb_commit = (struct commit *)create_object(r, &cur_oid, alloc_commit_node(r));
 		if (repo_parse_commit_internal(r, odb_commit, 0, 0)) {
 			graph_report(_("failed to parse commit %s from object database for commit-graph"),

commit.c

@@ -189,7 +189,7 @@ void unparse_commit(struct repository *r, const struct object_id *oid)
 {
 	struct commit *c = lookup_commit(r, oid);
 
-	if (!c->object.parsed)
+	if (!c || !c->object.parsed)
 		return;
 	free_commit_list(c->parents);
 	c->parents = NULL;

fetch-pack.c

@@ -155,7 +155,7 @@ static struct commit *deref_without_lazy_fetch(const struct object_id *oid,
 			struct tag *tag = (struct tag *)
 				parse_object(the_repository, oid);
 
-			if (!tag->tagged)
+			if (!tag || !tag->tagged)
 				return NULL;
 			if (mark_tags_complete_and_check_obj_db)
 				tag->object.flags |= COMPLETE;

fsck.c

@@ -925,7 +925,7 @@ static int fsck_commit(const struct object_id *oid,
 {
 	struct object_id tree_oid, parent_oid;
 	unsigned author_count;
-	int err;
+	int err = 0;
 	const char *buffer_begin = buffer;
 	const char *buffer_end = buffer + size;
 	const char *p;

object-name.c

@@ -1108,7 +1108,7 @@ static enum get_oid_result get_parent(struct repository *r,
 	if (ret)
 		return ret;
 	commit = lookup_commit_reference(r, &oid);
-	if (repo_parse_commit(r, commit))
+	if (!commit || repo_parse_commit(r, commit))
 		return MISSING_OBJECT;
 	if (!idx) {
 		oidcpy(result, &commit->object.oid);

pack-mtimes.c

@@ -28,7 +28,7 @@ static int load_pack_mtimes_file(char *mtimes_file,
 	int fd, ret = 0;
 	struct stat st;
 	uint32_t *data = NULL;
-	size_t mtimes_size, expected_size;
+	size_t mtimes_size = 0, expected_size;
 	struct mtimes_header header;
 
 	fd = git_open(mtimes_file);

pack-revindex.c

@@ -211,7 +211,7 @@ static int load_revindex_from_disk(const struct git_hash_algo *algo,
 	int fd, ret = 0;
 	struct stat st;
 	void *data = NULL;
-	size_t revindex_size;
+	size_t revindex_size = 0;
 	struct revindex_header *hdr;
 
 	if (git_env_bool(GIT_TEST_REV_INDEX_DIE_ON_DISK, 0))

revision.c

@@ -3368,6 +3368,9 @@ static int leave_one_treesame_to_parent(struct rev_info *revs, struct commit *co
 	struct commit_list *p;
 	unsigned n;
 
+	if (!ts)
+		return 0;
+
 	for (p = commit->parents, n = 0; p; p = p->next, n++) {
 		if (ts->treesame[n]) {
 			if (p->item->object.flags & TMP_MARK) {