Skip to contentSkip to conversation

Some defensive programming

PATCH In review Johannes Schindelin via GitGitGadget 3 participants 19 messages May 15, 2025

lore
Share & tools
lore
#0 lore raw

cover Some defensive programming

These patches implement some defensive programming to address complaints some static analyzers might have.

Johannes Schindelin (14):
revision: defensive programming
get_parent(): defensive programming
fetch-pack: defensive programming
unparse_commit(): defensive programming verify_commit_graph(): defensive programming
stash: defensive programming
stash: defensive programming
push: defensive programming
fetch: defensive programming
describe: defensive programming
inherit_tracking(): defensive programming
submodule: check return value of submodule_from_path() test-tool repository: check return value of lookup_commit()
shallow: handle missing shallow commits gracefully

 branch.c                    | 2 ++
 builtin/describe.c          | 2 ++
 builtin/fetch.c             | 3 ++-
 builtin/push.c              | 2 +-
 builtin/stash.c             | 7 ++++++-
 builtin/submodule--helper.c | 3 +++
 commit-graph.c              | 5 +++++
 commit.c                    | 2 +-
 fetch-pack.c                | 2 +-
 object-name.c               | 2 +-
 revision.c                  | 3 +++
 shallow.c                   | 3 ++-
 t/helper/test-repository.c  | 2 ++
 13 files changed, 31 insertions(+), 7 deletions(-)

base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1890%2Fdscho%2Fdefensive-programming-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1890/dscho/defensive-programming-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1890

Signature · 1 line
gitgitgadget
#1 lore raw

PATCH 1/14 revision: defensive programming

From: Johannes Schindelin <[email protected]>

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.

---
 revision.c | 3 +++
 1 file changed, 3 insertions(+)
revision.c +3 -0
index c4390f0938cb..59eae4eb8ba8 100644
@@ -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) {
Signature · 1 line
gitgitgadget
#3 lore raw

PATCH 2/14 get_parent(): defensive programming

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

CodeQL points out that lookup_commit_reference() can return NULL values.

---
 object-name.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
object-name.c +1 -1
index 76749fbfe652..ca54dad2f4c8 100644
@@ -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);
Signature · 1 line
gitgitgadget
#6 lore raw

PATCH 3/14 fetch-pack: defensive programming

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

CodeQL points out that parse_object() can return NULL values.

---
 fetch-pack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fetch-pack.c +1 -1
index 1ed5e11dd568..4cbcb0c14c48 100644
@@ -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;
Signature · 1 line
gitgitgadget
#8 lore raw

PATCH 4/14 unparse_commit(): defensive programming

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

CodeQL points out that lookup_commit() can return NULL values.

---
 commit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
commit.c +1 -1
index 6efdb03997d9..1cbc798e32ba 100644
@@ -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;
Signature · 1 line
gitgitgadget
#9 lore raw

PATCH 5/14 verify_commit_graph(): defensive programming

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

CodeQL points out that lookup_commit() can return NULL values.

---
 commit-graph.c | 5 +++++
 1 file changed, 5 insertions(+)
commit-graph.c +5 -0
index 1021ccb983d4..b3696736d248 100644
@@ -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"),
Signature · 1 line
gitgitgadget
#10 lore raw

PATCH 6/14 stash: defensive programming

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

CodeQL points out that parse_tree_indirect() can return NULL values.

---
 builtin/stash.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/stash.c +1 -1
index dbaa999cf171..23c4bbd3e21e 100644
@@ -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);
Signature · 1 line
gitgitgadget
#11 lore raw

PATCH 7/14 stash: defensive programming

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

CodeQL points out that lookup_commit() can return NULL values.

---
 builtin/stash.c | 5 +++++
 1 file changed, 5 insertions(+)
builtin/stash.c +5 -0
index 23c4bbd3e21e..8efcd31d6c61 100644
@@ -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)) {
Signature · 1 line
gitgitgadget
#12 lore raw

PATCH 8/14 push: defensive programming

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

CodeQL points out that branch_get() can return NULL values.

---
 builtin/push.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/push.c +1 -1
index 92d530e5c4df..db698c103424 100644
@@ -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;
Signature · 1 line
gitgitgadget
#13 lore raw

PATCH 9/14 fetch: defensive programming

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

CodeQL points out that branch_get() can return NULL values.

---
 builtin/fetch.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
builtin/fetch.c +2 -1
index 95fd0018b981..e7523c531407 100644
@@ -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) {
Signature · 1 line
gitgitgadget
#14 lore raw

PATCH 10/14 describe: defensive programming

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

CodeQL points out that lookup_commit_reference() can return NULL values.

---
 builtin/describe.c | 2 ++
 1 file changed, 2 insertions(+)
builtin/describe.c +2 -0
index e2e73f3d757c..455ca193ebd3 100644
@@ -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)) {
Signature · 1 line
gitgitgadget
#15 lore raw

PATCH 11/14 inherit_tracking(): defensive programming

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

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.

---
 branch.c | 2 ++
 1 file changed, 2 insertions(+)
branch.c +2 -0
index 91297d55ac9f..a10b6119b214 100644
@@ -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);
Signature · 1 line
gitgitgadget
#16 lore raw

PATCH 12/14 submodule: check return value of `submodule_from_path()`

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

As pointed out by CodeQL, it could be NULL and we usually check for that.

---
 builtin/submodule--helper.c | 3 +++
 1 file changed, 3 insertions(+)
builtin/submodule--helper.c +3 -0
index c1a8029714bf..55826b82407c 100644
@@ -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) {
Signature · 1 line
gitgitgadget
#17 lore raw

PATCH 13/14 test-tool repository: check return value of `lookup_commit()`

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

On the off-chance that it's NULL...

---
 t/helper/test-repository.c | 2 ++
 1 file changed, 2 insertions(+)
t/helper/test-repository.c +2 -0
index 63c37de33d22..05417d8f43dc 100644
@@ -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");
Signature · 1 line
gitgitgadget
#18 lore raw

PATCH 14/14 shallow: handle missing shallow commits gracefully

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

As pointed out by CodeQL, lookup_commit() can return NULL.

---
 shallow.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
shallow.c +2 -1
index 4bd9342c9a74..011f262cc7d4 100644
@@ -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++)
Signature · 1 line
gitgitgadget
#2 lore raw

Re: [PATCH 01/14] revision: defensive programming

In reply to Johannes Schindelin via GitGitGadget

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 4 lines · writes:

From: Johannes Schindelin <[email protected]>

On the off chance that lookup_decoration() cannot find anything, let leave_one_treesame_to_parent() return gracefully instead of crashing.

But wouldn't it be a BUG("") worthy event for the treesame decoration not to exist for the commit object in question at this point of the code? Is it really defensive to silently pretend that nothing bad happened and to move forward?

Quoted 21 lines

Pointed out by CodeQL.

---
 revision.c | 3 +++
 1 file changed, 3 insertions(+)
revision.c +3 -0
index c4390f0938cb..59eae4eb8ba8 100644
@@ -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) {
#4 lore raw

Re: [PATCH 02/14] get_parent(): defensive programming

In reply to Johannes Schindelin via GitGitGadget

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 15 lines · writes:

From: Johannes Schindelin <[email protected]>

CodeQL points out that lookup_commit_reference() can return NULL values.

---
 object-name.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
object-name.c +0 -0
index 76749fbfe652..ca54dad2f4c8 100644

object-name.c static enum get_oid_result get_parent(struct repository *r, · line 1110

Quoted 6 lines
object-name.c +1 -1
@@ -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;

Most of the time, the check for "ret" we see in the pre-context, which is a result of get_oid_1(), would prevent an oid that is not a valid name for a committish to even reach this code, I would think, but with possible repository corruption, we may fail to "lookup" the commit, so this is a good correction, I would think.

Thanks.

Quoted 2 lines
object-name.c +0 -0
 	if (!idx) {
 		oidcpy(result, &commit->object.oid);
#7 lore raw

Re: [PATCH 03/14] fetch-pack: defensive programming

In reply to Johannes Schindelin via GitGitGadget

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 14 lines · writes:

From: Johannes Schindelin <[email protected]>

CodeQL points out that parse_object() can return NULL values.

---
 fetch-pack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fetch-pack.c +0 -0
index 1ed5e11dd568..4cbcb0c14c48 100644

fetch-pack.c static struct commit *deref_without_lazy_fetch(const struct object_id *oid, · line 159

Quoted 6 lines
fetch-pack.c +1 -1
@@ -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;

The "oid" can come from corruptible sources like commit graph file, so I agree with your analysis that it may name a missing object in a corrupt repository, leading "tag" being NULL. Looks correct.

Quoted 2 lines
fetch-pack.c +0 -0
 			if (mark_tags_complete_and_check_obj_db)
 				tag->object.flags |= COMPLETE;
#5 lore raw

Re: [PATCH 02/14] get_parent(): defensive programming

In reply to Johannes Schindelin via GitGitGadget

object-name.c static enum get_oid_result get_parent(struct repository *r, · line 1110

Quoted 7 lines · Johannes Schindelin via GitGitGadget

CodeQL points out that lookup_commit_reference() can return NULL values.
[...]

object-name.c +1 -1
 	commit = lookup_commit_reference(r, &oid);
-	if (repo_parse_commit(r, commit))
+	if (!commit || repo_parse_commit(r, commit))
 		return MISSING_OBJECT;

Sure, but repo_parse_commit() can also handle NULL values. It returns "-1" in that case. I think CodeQL is not smart enough to know that.

-Peff