From 604e67ee64167efd5282b9efabbcb6e72eeaf5e6 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 16 Dec 2022 23:27:05 +0100 Subject: [PATCH 01/14] revision: defensive programming On the off chance that `lookup_decoration()` cannot find anything, let `leave_one_treesame_to_parent()` return gracefully instead of crashing. Pointed out by CodeQL. Signed-off-by: Johannes Schindelin --- revision.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/revision.c b/revision.c index c4390f0938cbde..59eae4eb8ba897 100644 --- a/revision.c +++ b/revision.c @@ -3359,6 +3359,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) { From 35c4870e2c101c9ef72d1657c8f6dd077cecc5fa Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 16 Dec 2022 23:35:08 +0100 Subject: [PATCH 02/14] get_parent(): defensive programming CodeQL points out that `lookup_commit_reference()` can return NULL values. Signed-off-by: Johannes Schindelin --- object-name.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/object-name.c b/object-name.c index 76749fbfe652bf..ca54dad2f4c8ec 100644 --- a/object-name.c +++ b/object-name.c @@ -1106,7 +1106,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); From 9cafbd36817aabbe18197938c409edaddf7dcab2 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 16 Dec 2022 23:38:02 +0100 Subject: [PATCH 03/14] fetch-pack: defensive programming CodeQL points out that `parse_object()` can return NULL values. Signed-off-by: Johannes Schindelin --- fetch-pack.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fetch-pack.c b/fetch-pack.c index 1ed5e11dd56857..4cbcb0c14c4889 100644 --- a/fetch-pack.c +++ b/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; From dcc04afb6cb0f0383c95d7fc35d492f0e1d256a9 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 16 Dec 2022 23:41:12 +0100 Subject: [PATCH 04/14] unparse_commit(): defensive programming CodeQL points out that `lookup_commit()` can return NULL values. Signed-off-by: Johannes Schindelin --- commit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/commit.c b/commit.c index 6efdb03997d9a0..1cbc798e32ba71 100644 --- a/commit.c +++ b/commit.c @@ -187,7 +187,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; From 721402d9d8a76d3564aee0403ee046a2bd035dc9 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 16 Dec 2022 23:44:08 +0100 Subject: [PATCH 05/14] verify_commit_graph(): defensive programming CodeQL points out that `lookup_commit()` can return NULL values. Signed-off-by: Johannes Schindelin --- commit-graph.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/commit-graph.c b/commit-graph.c index 1021ccb983d4ee..b3696736d248b6 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -2786,6 +2786,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"), From 130251bc324cc90ab19f82552888071f974c35b6 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 16 Dec 2022 23:49:59 +0100 Subject: [PATCH 06/14] stash: defensive programming CodeQL points out that `parse_tree_indirect()` can return NULL values. Signed-off-by: Johannes Schindelin --- builtin/stash.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/stash.c b/builtin/stash.c index dbaa999cf171a7..23c4bbd3e21e2d 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -285,7 +285,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); From 81e873ea2ce5180006be9ad0121d2d5aafb44730 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 16 Dec 2022 23:51:59 +0100 Subject: [PATCH 07/14] stash: defensive programming CodeQL points out that `lookup_commit()` can return NULL values. Signed-off-by: Johannes Schindelin --- builtin/stash.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/builtin/stash.c b/builtin/stash.c index 23c4bbd3e21e2d..8efcd31d6c614a 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -1396,6 +1396,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)) { From 8d1efe06d9ee7a00e8408e37deea88c469f835ea Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 16 Dec 2022 23:53:40 +0100 Subject: [PATCH 08/14] push: defensive programming CodeQL points out that `branch_get()` can return NULL values. Signed-off-by: Johannes Schindelin --- builtin/push.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/push.c b/builtin/push.c index 92d530e5c4dff0..db698c1034243f 100644 --- a/builtin/push.c +++ b/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; From d8397662bcf72445dd06a09a6615b066c9166cf4 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 16 Dec 2022 23:55:55 +0100 Subject: [PATCH 09/14] fetch: defensive programming CodeQL points out that `branch_get()` can return NULL values. Signed-off-by: Johannes Schindelin --- builtin/fetch.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index 95fd0018b981fb..e7523c53140735 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -553,7 +553,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 && @@ -571,6 +571,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) { From 10393c2f951e30e6b8529ff4fafb4394c3840485 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 16 Dec 2022 23:57:16 +0100 Subject: [PATCH 10/14] describe: defensive programming CodeQL points out that `lookup_commit_reference()` can return NULL values. Signed-off-by: Johannes Schindelin --- builtin/describe.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/builtin/describe.c b/builtin/describe.c index e2e73f3d757cab..455ca193ebd367 100644 --- a/builtin/describe.c +++ b/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)) { From dec21f89ffd06d8b307e7f46191be3824bd99b96 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sat, 17 Dec 2022 00:02:33 +0100 Subject: [PATCH 11/14] inherit_tracking(): defensive programming CodeQL points out that `branch_get()` can return NULL values. Note that the error path in this instance calls `BUG()`, not `die()`, for two reasons: 1. The code lives in `libgit.a` and calling `die()` from within those library functions is a bad practice that needs to be reduced, rather than increased. 2. The `inherit_tracking()` function really should only be called with the name of an existing branch, therefore a `NULL` return value would indeed constitute a bug in Git's code. Signed-off-by: Johannes Schindelin --- branch.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/branch.c b/branch.c index 91297d55ac9f60..a10b6119b214a9 100644 --- a/branch.c +++ b/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); From 223a0053eb7f94dfd28db8f528b73a489f907444 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 16 Dec 2022 23:20:54 +0100 Subject: [PATCH 12/14] submodule: check return value of `submodule_from_path()` As pointed out by CodeQL, it could be NULL and we usually check for that. Signed-off-by: Johannes Schindelin --- builtin/submodule--helper.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index c1a8029714bfe9..55826b82407cf4 100644 --- a/builtin/submodule--helper.c +++ b/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) { From 137f6c45af74c718e7acf0e3a569633291dcbc43 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 16 Dec 2022 23:22:50 +0100 Subject: [PATCH 13/14] test-tool repository: check return value of `lookup_commit()` On the off-chance that it's NULL... Signed-off-by: Johannes Schindelin --- t/helper/test-repository.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/t/helper/test-repository.c b/t/helper/test-repository.c index 63c37de33d22f1..05417d8f43dcab 100644 --- a/t/helper/test-repository.c +++ b/t/helper/test-repository.c @@ -27,6 +27,8 @@ static void test_parse_commit_in_graph(const char *gitdir, const char *worktree, repo_set_hash_algo(the_repository, hash_algo_by_ptr(r.hash_algo)); c = lookup_commit(&r, commit_oid); + if (!c) + die("Could not look up %s", oid_to_hex(commit_oid)); if (!parse_commit_in_graph(&r, c)) die("Couldn't parse commit"); From 17e9e9ae0e426728cb22927a26bbaf7191bc37cb Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 16 Dec 2022 23:25:01 +0100 Subject: [PATCH 14/14] shallow: handle missing shallow commits gracefully As pointed out by CodeQL, `lookup_commit()` can return NULL. Signed-off-by: Johannes Schindelin --- shallow.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/shallow.c b/shallow.c index 4bd9342c9a745a..011f262cc7d4d4 100644 --- a/shallow.c +++ b/shallow.c @@ -702,7 +702,8 @@ void assign_shallow_commits_to_refs(struct shallow_info *info, for (i = 0; i < nr_shallow; i++) { struct commit *c = lookup_commit(the_repository, &oid[shallow[i]]); - c->object.flags |= BOTTOM; + if (c) + c->object.flags |= BOTTOM; } for (i = 0; i < ref->nr; i++)