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,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) {