[PATCH 00/14] Some defensive programming

[PATCH 00/14] Some defensive programming

[Johannes Schindelin via GitGitGadget]

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
-- 
gitgitgadget

[PATCH 01/14] revision: defensive programming

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 revision.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/revision.c b/revision.c
index c4390f0938cb..59eae4eb8ba8 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) {
-- 
gitgitgadget

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

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 object-name.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/object-name.c b/object-name.c
index 76749fbfe652..ca54dad2f4c8 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);
-- 
gitgitgadget

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

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 fetch-pack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index 1ed5e11dd568..4cbcb0c14c48 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;
-- 
gitgitgadget

[PATCH 04/14] unparse_commit(): defensive programming

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 commit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/commit.c b/commit.c
index 6efdb03997d9..1cbc798e32ba 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;
-- 
gitgitgadget

[PATCH 05/14] verify_commit_graph(): defensive programming

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 commit-graph.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/commit-graph.c b/commit-graph.c
index 1021ccb983d4..b3696736d248 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"),
-- 
gitgitgadget

[PATCH 06/14] stash: defensive programming

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/stash.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index dbaa999cf171..23c4bbd3e21e 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);
-- 
gitgitgadget

[PATCH 07/14] stash: defensive programming

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/stash.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/builtin/stash.c b/builtin/stash.c
index 23c4bbd3e21e..8efcd31d6c61 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)) {
-- 
gitgitgadget

[PATCH 08/14] push: defensive programming

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/push.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/push.c b/builtin/push.c
index 92d530e5c4df..db698c103424 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;
-- 
gitgitgadget

[PATCH 09/14] fetch: defensive programming

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/fetch.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 95fd0018b981..e7523c531407 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) {
-- 
gitgitgadget

[PATCH 10/14] describe: defensive programming

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/describe.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/builtin/describe.c b/builtin/describe.c
index e2e73f3d757c..455ca193ebd3 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)) {
-- 
gitgitgadget

[PATCH 11/14] inherit_tracking(): defensive programming

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 branch.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/branch.c b/branch.c
index 91297d55ac9f..a10b6119b214 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);
-- 
gitgitgadget

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

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/submodule--helper.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index c1a8029714bf..55826b82407c 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) {
-- 
gitgitgadget

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

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 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 63c37de33d22..05417d8f43dc 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");
-- 
gitgitgadget

[PATCH 14/14] shallow: handle missing shallow commits gracefully

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 shallow.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/shallow.c b/shallow.c
index 4bd9342c9a74..011f262cc7d4 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++)
-- 
gitgitgadget

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

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 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?

> Pointed out by CodeQL.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  revision.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/revision.c b/revision.c
> index c4390f0938cb..59eae4eb8ba8 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) {

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

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> CodeQL points out that `lookup_commit_reference()` can return NULL
> values.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  object-name.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/object-name.c b/object-name.c
> index 76749fbfe652..ca54dad2f4c8 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;

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.


>  	if (!idx) {
>  		oidcpy(result, &commit->object.oid);

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

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> CodeQL points out that `parse_object()` can return NULL values.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  fetch-pack.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 1ed5e11dd568..4cbcb0c14c48 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;

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.


>  			if (mark_tags_complete_and_check_obj_db)
>  				tag->object.flags |= COMPLETE;

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

[Jeff King]

On Thu, May 15, 2025 at 12:45:27PM +0000, Johannes Schindelin via GitGitGadget wrote:

> CodeQL points out that `lookup_commit_reference()` can return NULL
> values.
> [...]
>  	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