coverity: avoid dereferencing NULL

This is a continuation of the effort I started in the patch series that became js/coverity-fixes. This next batch adds guards to avoid dereferencing NULL pointers and accessing NULL file descriptors.

Johannes Schindelin (11):
diffcore-break: guard against NULLed queue entries in merge loop
diff: handle NULL return from repo_get_commit_tree()
remote: guard remote_tracking() against NULL remote reftable/stack: guard against NULL list_file in stack_destroy
mailsplit: move NULL check before first use of file handle
bisect: handle NULL commit in bisect_successful()
replay: die when --onto does not peel to a commit
revision: avoid dereferencing NULL in add_parents_only()
pack-bitmap: handle missing bitmap for base MIDX
bisect: ensure non-NULL head before using it
shallow: fix NULL dereference

 builtin/bisect.c    |  9 ++++++++-
 builtin/diff.c      | 10 +++++++---
 builtin/mailsplit.c |  6 +++---
 diffcore-break.c    |  2 ++
 pack-bitmap.c       |  4 ++++
 reftable/stack.c    |  3 ++-
 remote.c            |  2 ++
 replay.c            |  8 ++++++--
 revision.c          |  9 +++++++--
 shallow.c           |  2 +-
 10 files changed, 42 insertions(+), 13 deletions(-)

base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2174%2Fdscho%2Fcoverity-fixes-null-safety-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2174/dscho/coverity-fixes-null-safety-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2174

gitgitgadget

From: Johannes Schindelin <[email protected]>

The outer loop in diffcore_merge_broken() sets q->queue[j] to NULL when it merges a broken pair back together, and has a NULL check to skip such entries on subsequent iterations. The inner loop, however, lacks this guard: when it scans forward looking for a matching peer, it can encounter a slot that was NULLed by a previous outer-loop iteration and dereference it unconditionally.

In practice this requires at least two broken pairs whose peers both survive rename/copy detection and appear later in the queue, which is rare but not impossible.

Add the same if (!pp) continue guard to the inner loop.

Pointed out by Coverity.

---
 diffcore-break.c | 2 ++
 1 file changed, 2 insertions(+)

index 17b5ad1fed..b5bcc956cc 100644
@@ -289,6 +289,8 @@ void diffcore_merge_broken(void)
 			 */
 			for (j = i + 1; j < q->nr; j++) {
 				struct diff_filepair *pp = q->queue[j];
+				if (!pp)
+					continue;
 				if (pp->broken_pair &&
 				    !strcmp(pp->one->path, pp->two->path) &&
 				    !strcmp(p->one->path, pp->two->path)) {

gitgitgadget

From: Johannes Schindelin <[email protected]>

The repo_get_commit_tree() function can return NULL when a commit's tree object is not available (e.g., the commit was parsed but its maybe_tree field is unset and the commit is not in the commit-graph). In cmd_diff(), the return value is immediately dereferenced via ->object without a NULL check, which would crash if the tree cannot be loaded.

Add an explicit NULL check and die with a descriptive message.

Pointed out by Coverity.

---
 builtin/diff.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

index 4b46e394ce..18b1083e98 100644
@@ -579,9 +579,13 @@ int cmd_diff(int argc,
 		obj = deref_tag(the_repository, obj, NULL, 0);
 		if (!obj)
 			die(_("invalid object '%s' given."), name);
-		if (obj->type == OBJ_COMMIT)
-			obj = &repo_get_commit_tree(the_repository,
-						    ((struct commit *)obj))->object;
+		if (obj->type == OBJ_COMMIT) {
+			struct tree *tree = repo_get_commit_tree(
+				the_repository, (struct commit *)obj);
+			if (!tree)
+				die(_("unable to read tree object for commit '%s'"), name);
+			obj = &tree->object;
+		}
 
 		if (obj->type == OBJ_TREE) {
 			if (sdiff.skip && bitmap_get(sdiff.skip, i))

gitgitgadget

From: Johannes Schindelin <[email protected]>

The remote_tracking() function unconditionally dereferences remote->fetch without checking whether remote is NULL.

In practice, this never happens because the only caller (apply_cas()) guards the calls to this function by checking the use_tracking and use_tracking_for_rest attributes.

However, it requires quite involved reasoning to reach that conclusion, and is therefore fragile. Just return -1 ("no tracking ref") when there is no remote to work with.

Pointed out by Coverity.

---
 remote.c | 2 ++
 1 file changed, 2 insertions(+)

index 00723b385e..34d0367f11 100644
@@ -2681,6 +2681,8 @@ static int remote_tracking(struct remote *remote, const char *refname,
 {
 	char *dst;
 
+	if (!remote)
+		return -1; /* no remote to look up tracking ref */
 	dst = apply_refspecs(&remote->fetch, refname);
 	if (!dst)
 		return -1; /* no tracking ref for refname at remote */

gitgitgadget

From: Johannes Schindelin <[email protected]>

When reftable_new_stack() fails partway through initialization (e.g., reftable_buf_addstr returns an OOM error before reftable_buf_detach assigns p->list_file), it jumps to the error path which calls reftable_stack_destroy(p). At that point, p->list_file is still NULL because the detach never happened.

reftable_stack_destroy() passes st->list_file unconditionally to read_lines(), which calls open(filename, O_RDONLY). Passing NULL to open() is undefined behavior and will typically crash.

Guard the read_lines() call with a NULL check on st->list_file. When list_file is NULL, there are no table files to clean up anyway, so skipping read_lines is the correct behavior.

Pointed out by Coverity.

---
 reftable/stack.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index 1fba96ddb3..3fc3c0b2d1 100644
@@ -171,7 +171,8 @@ void reftable_stack_destroy(struct reftable_stack *st)
 		st->merged = NULL;
 	}
 
-	err = read_lines(st->list_file, &names);
+	if (st->list_file)
+		err = read_lines(st->list_file, &names);
 	if (err < 0) {
 		REFTABLE_FREE_AND_NULL(names);
 	}

gitgitgadget

From: Johannes Schindelin <[email protected]>

The split_mbox() function calls fileno(f) to check whether the input is a terminal, but the NULL check for f (from fopen()) does not happen until later. When the file cannot be opened, f is NULL, and fileno(NULL) is undefined behavior, typically crashing with a segmentation fault.

Move the NULL check above the isatty()/fileno() call so the error path is taken before any use of the potentially-NULL handle.

Pointed out by Coverity.

---
 builtin/mailsplit.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index 264df6259a..0993418e63 100644
@@ -225,14 +225,14 @@ static int split_mbox(const char *file, const char *dir, int allow_bare,
 	FILE *f = !strcmp(file, "-") ? stdin : fopen(file, "r");
 	int file_done = 0;
 
-	if (isatty(fileno(f)))
-		warning(_("reading patches from stdin/tty..."));
-
 	if (!f) {
 		error_errno("cannot open mbox %s", file);
 		goto out;
 	}
 
+	if (isatty(fileno(f)))
+		warning(_("reading patches from stdin/tty..."));
+
 	do {
 		peek = fgetc(f);
 		if (peek == EOF) {

gitgitgadget

From: Johannes Schindelin <[email protected]>

When lookup_commit_reference_by_name() is called to find the first bad commit, the result is passed to repo_format_commit_message() immediately, which dereferences commit without checking for NULL.

However, the commit could be NULL, even though in practice this is unlikely because bisect_successful() is only called after a successful bisect run has identified the bad commit, but the ref could still become dangling due to a concurrent gc or repository corruption.

Pointed out by Coverity.

---
 builtin/bisect.c | 5 +++++
 1 file changed, 5 insertions(+)

index e7c2d2f3bb..6ff600c856 100644
@@ -663,6 +663,11 @@ static int bisect_successful(struct bisect_terms *terms)
 
 	refs_read_ref(get_main_ref_store(the_repository), bad_ref, &oid);
 	commit = lookup_commit_reference_by_name(bad_ref);
+	if (!commit) {
+		res = error(_("could not find commit for '%s'"), bad_ref);
+		free(bad_ref);
+		return res;
+	}
 	repo_format_commit_message(the_repository, commit, "%s", &commit_name,
 				   &pp);
 

gitgitgadget

From: Johannes Schindelin <[email protected]>

The peel_committish() function calls repo_peel_to_type() to convert the given object to a commit, but does not check the return value. When the object exists but cannot be peeled to a commit (e.g., a tree or blob OID is passed as --onto), the return value is NULL. Add an explicit NULL check and die with a descriptive message in that case.

Pointed out by Coverity.

---
 replay.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

index da531d5bc6..b38cd5efe4 100644
@@ -36,12 +36,16 @@ static struct commit *peel_committish(struct repository *repo,
 {
 	struct object *obj;
 	struct object_id oid;
+	struct commit *commit;
 
 	if (repo_get_oid(repo, name, &oid))
 		die(_("'%s' is not a valid commit-ish for %s"), name, mode);
 	obj = parse_object_or_die(repo, &oid, name);
-	return (struct commit *)repo_peel_to_type(repo, name, 0, obj,
-						  OBJ_COMMIT);
+	commit = (struct commit *)repo_peel_to_type(repo, name, 0, obj,
+						    OBJ_COMMIT);
+	if (!commit)
+		die(_("'%s' does not point to a commit for %s"), name, mode);
+	return commit;
 }
 
 static char *get_author(const char *message)

gitgitgadget

From: Johannes Schindelin <[email protected]>

This function resolves revision suffixes like commit^@ (all parents), commit^! (commit minus parents), and commit^-N (exclude Nth parent). It calls get_reference() in a loop to peel through tag objects until it reaches a commit.

The existing NULL check after get_reference() only handles the ignore_missing case, but get_reference() can return NULL through three distinct paths:

1. revs->ignore_missing: the caller asked to silently skip missing objects.

2. revs->exclude_promisor_objects: the object is a lazy promisor object that should be excluded from the walk.

3. revs->do_not_die_on_missing_objects: the caller wants to record missing OIDs for later reporting (used by git rev-list --missing=print) rather than dying.

In the latter two instances, the code falls through to dereference the NULL pointer.

Handle all three cases explicitly:

- ignore_missing: return 0, matching the existing behavior and the pattern in handle_revision_arg().

- do_not_die_on_missing_objects: return 0. The missing OID has already been recorded in revs->missing_commits by get_reference(). Returning 0 is consistent with handle_revision_arg() and process_parents(), both of which continue without error when this flag is set. The broader codebase pattern for this flag is "record and continue": list-objects.c, builtin/rev-list.c, and process_parents all skip the die/error and keep walking.

- everything else (only the exclude_promisor_objects case in practice): return -1, consistent with handle_revision_arg() where the condition only matches ignore_missing or do_not_die_on_missing_objects, falling through to ret = -1 for the promisor case.

Note: the callers of add_parents_only() in handle_revision_pseudo_opt() treat any nonzero return as "handled" (if (add_parents_only(...)) { ret = 0; }), so the -1 for the promisor case is indistinguishable from success there. This means a promisor-excluded tag target referenced via commit^@ would be silently skipped rather than producing an error. This is a pre-existing limitation of the caller's return value handling and not made worse by this change; the alternative (a NULL dereference crash) _would be_ strictly worse.

Pointed out by Coverity.

---
 revision.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

index e91d7e1f11..7f3999b551 100644
@@ -1903,8 +1903,13 @@ static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,
 		return 0;
 	while (1) {
 		it = get_reference(revs, arg, &oid, 0);
-		if (!it && revs->ignore_missing)
-			return 0;
+		if (!it) {
+			if (revs->ignore_missing)
+				return 0;
+			if (revs->do_not_die_on_missing_objects)
+				return 0;
+			return -1;
+		}
 		if (it->type != OBJ_TAG)
 			break;
 		if (!((struct tag*)it)->tagged)

gitgitgadget

From: Johannes Schindelin <[email protected]>

When prepare_midx_bitmap_git() is called to load the bitmap for a chained MIDX's base layer, if the base MIDX does not have an associated bitmap file (e.g., it was not generated, or was deleted by gc), the return value is NULL. It is then stored in bitmap_git->base and immediately dereferenced on the next line.

This can happen in practice with incremental MIDX chains: the base MIDX may have been written without --write-bitmap-index, or the bitmap may have been pruned while the incremental layer's bitmap still references it.

Check the return value and go to the cleanup label (which unmaps the current bitmap and returns -1) so the caller falls back to non-bitmap object enumeration, matching the handling of other bitmap loading failures in the same function.

Pointed out by Coverity.

---
 pack-bitmap.c | 4 ++++
 1 file changed, 4 insertions(+)

index e8a82945cc..ca7998c10b 100644
@@ -523,6 +523,10 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
 
 	if (midx->base_midx) {
 		bitmap_git->base = prepare_midx_bitmap_git(midx->base_midx);
+		if (!bitmap_git->base) {
+			warning(_("could not open bitmap for base MIDX"));
+			goto cleanup;
+		}
 		bitmap_git->base_nr = bitmap_git->base->base_nr + 1;
 	} else {
 		bitmap_git->base_nr = 0;

gitgitgadget

From: Johannes Schindelin <[email protected]>

When refs_resolve_ref_unsafe() is called to resolve HEAD, and returns NULL (e.g., HEAD does not exist as a proper ref), the code falls back to repo_get_oid("HEAD") to try to resolve the OID directly. If that succeeds, execution continues with head still set to NULL.

Later, that variable is passed to repo_get_oid() and starts_with(), both of which would dereference the NULL pointer.

The scenario "refs_resolve_ref_unsafe() returns NULL but repo_get_oid() succeeds" can happen when HEAD is a detached bare OID that the ref backend cannot resolve symbolically (a potential edge case with the reftable backend) but the OID itself is valid. In this case, the bisect-start file does not yet exist (this is a fresh "git bisect start"), so the else branch is taken with the NULL head.

Simply assign "HEAD" to head as a fallback to address this.

Pointed out by Coverity.

---
 builtin/bisect.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

index 6ff600c856..a69771c6d3 100644
@@ -811,9 +811,11 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, int argc,
 	 */
 	head = refs_resolve_ref_unsafe(get_main_ref_store(the_repository),
 				       "HEAD", 0, &head_oid, &flags);
-	if (!head)
+	if (!head) {
 		if (repo_get_oid(the_repository, "HEAD", &head_oid))
 			return error(_("bad HEAD - I need a HEAD"));
+		head = "HEAD";
+	}
 
 	/*
 	 * Check if we are bisecting

gitgitgadget

From: Johannes Schindelin <[email protected]>

After write_one_shallow() calls lookup_commit() to find the commit object for a shallow graft entry, it then checks if (!c || ...). Inside that block, when the VERBOSE flag is set, it prints the OID being removed, via c->object.oid. But c can be NULL (the first condition in the || check).

This happens when a shallow graft entry references a commit object that is not in the object store (e.g., after a partial fetch or in a corrupted repository). In that case, lookup_commit() returns NULL because the object cannot be found, the SEEN_ONLY check correctly decides to remove this entry from .git/shallow, but the verbose message crashes before the removal can complete.

Use graft->oid instead of c->object.oid for the message. The graft entry's OID is the same value (it was used as the lookup key) and is always available regardless of whether the commit object exists.

Pointed out by Coverity.

---
 shallow.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 07cae44ae5..3d2230351e 100644
@@ -371,7 +371,7 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)
 		if (!c || !(c->object.flags & SEEN)) {
 			if (data->flags & VERBOSE)
 				printf("Removing %s from .git/shallow\n",
-				       oid_to_hex(&c->object.oid));
+				       oid_to_hex(&graft->oid));
 			return 0;
 		}
 	}

gitgitgadget

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

                struct write_shallow_data *data = cb_data;
        -	const char *hex = oid_to_hex(&graft->oid);
        +	char hex[GIT_MAX_HEXSZ + 1];
        +
        +       oid_to_hex_r(hex, &graft->oid);
                if (graft->nr_parent != -1)
                        return 0;

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

Interesting find. This is an ancient part of the codebase that nobody has touched in the past 21 years since eeaa460314 ([PATCH]
diff: Update -B heuristics., 2005-06-03) introduced it ;-).

Well spotted.

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

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

In a case like this, where the function is designed not to be called with NULL remote, I would prefer to have an explicit BUG() rather than sweeping the problem under the rug. That would make sure your investigation and involved reasoning done here remain relevant if the BUG() triggers due to careless changes to the caller in the future.

Thanks.

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

Nice spotting and recovery. Well done.

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

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

	error(_("..."));
	free(bad_ref);
	return BISECT_FAILED;

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

Nicely spotted. It sounds like something a test can ensure does not to regress in the future, unless I am misreading this explanation. Could you include such a test?

Thanks.

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

Nicely reasoned. It would have been nicer to CC those who are more familiar with the area, though.

Cc'ed Taylor for incremental MIDX expertise just in case.

Thanks.

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

I agree that setting head to the string "HEAD" is a good solution to ensure that !starts_with(), !repo_get_oid(), and skip_prefix() are not called with NULL.

However, I am not sure I understand your "can happen" scenario.

I naively thought that the only case where HEAD does not resolve to an object correctly is when HEAD is a symbolic ref pointing to an unborn branch.

Is the bug in your "can happen" scenario something we can demonstrate? If so, could you add a test to prevent regressions in the future?

Thanks.

This is a continuation of the effort I started in the patch series that became js/coverity-fixes. This next batch adds guards to avoid dereferencing NULL pointers and accessing NULL file descriptors.

Changes since v1:

* Calling remote_tracking() no longer returns -1 when remote is NULL, but instead BUG()s out.
* bisect_successful() returns with BISECT_FAILED instead of the -1 that only worked by happenstance.
* The commit "revision: avoid dereferencing NULL in add_parents_only()" now comes with a regression test.
* The commit "bisect: ensure non-NULL head before using it" no longer claims that the fixed bug can be triggered with the current code base.
* The missing shallow commit's OID is no longer computed twice.
* A follow-up commit was folded into this patch series that lets write_one_shallow() avoid the rolling buffers of oid_to_hex(), as suggested by Junio. It technically does not fit the goal of this patch series (fixing issues pointed out by Coverity), but was asked for explicitly.

Johannes Schindelin (12):
diffcore-break: guard against NULLed queue entries in merge loop
diff: handle NULL return from repo_get_commit_tree()
remote: guard remote_tracking() against NULL remote reftable/stack: guard against NULL list_file in stack_destroy
mailsplit: move NULL check before first use of file handle
bisect: handle NULL commit in bisect_successful()
replay: die when --onto does not peel to a commit
revision: avoid dereferencing NULL in add_parents_only()
pack-bitmap: handle missing bitmap for base MIDX
bisect: ensure non-NULL head before using it
shallow: fix NULL dereference
shallow: give write_one_shallow() its own hex buffer

 builtin/bisect.c         |  9 ++++++++-
 builtin/diff.c           | 10 +++++++---
 builtin/mailsplit.c      |  6 +++---
 diffcore-break.c         |  2 ++
 pack-bitmap.c            |  4 ++++
 reftable/stack.c         |  3 ++-
 remote.c                 |  2 ++
 replay.c                 |  8 ++++++--
 revision.c               |  9 +++++++--
 shallow.c                |  7 ++++---
 t/t0410-partial-clone.sh | 18 ++++++++++++++++++
 11 files changed, 63 insertions(+), 15 deletions(-)

base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2174%2Fdscho%2Fcoverity-fixes-null-safety-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2174/dscho/coverity-fixes-null-safety-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/2174

Range-diff vs v1:

  1:  df00334f8b =  1:  df00334f8b diffcore-break: guard against NULLed queue entries in merge loop
  2:  4fdba0542b =  2:  4fdba0542b diff: handle NULL return from repo_get_commit_tree()
  3:  dcaefc5987 !  3:  1398a2f120 remote: guard `remote_tracking()` against NULL remote
     @@ remote.c: static int remote_tracking(struct remote *remote, const char *refname,
       	char *dst;
       
      +	if (!remote)
     -+		return -1; /* no remote to look up tracking ref */
     ++		BUG("remote_tracking() called with NULL remote");
       	dst = apply_refspecs(&remote->fetch, refname);
       	if (!dst)
       		return -1; /* no tracking ref for refname at remote */
  4:  d7bc7fce35 =  4:  285f019fb3 reftable/stack: guard against NULL list_file in stack_destroy
  5:  41eef047ae =  5:  12c2c8450e mailsplit: move NULL check before first use of file handle
  6:  7041375108 !  6:  ca818ee405 bisect: handle NULL commit in `bisect_successful()`
     @@ builtin/bisect.c: static int bisect_successful(struct bisect_terms *terms)
       	refs_read_ref(get_main_ref_store(the_repository), bad_ref, &oid);
       	commit = lookup_commit_reference_by_name(bad_ref);
      +	if (!commit) {
     -+		res = error(_("could not find commit for '%s'"), bad_ref);
     ++		error(_("could not find commit for '%s'"), bad_ref);
      +		free(bad_ref);
     -+		return res;
     ++		return BISECT_FAILED;
      +	}
       	repo_format_commit_message(the_repository, commit, "%s", &commit_name,
       				   &pp);
  7:  a7245cdffa =  7:  8216769be9 replay: die when --onto does not peel to a commit
  8:  0675767797 !  8:  41285dd8e1 revision: avoid dereferencing NULL in `add_parents_only()`
     @@ revision.c: static int add_parents_only(struct rev_info *revs, const char *arg_,
       		if (it->type != OBJ_TAG)
       			break;
       		if (!((struct tag*)it)->tagged)
     +
     + ## t/t0410-partial-clone.sh ##
     +@@ t/t0410-partial-clone.sh: test_expect_success 'rev-list dies for missing objects on cmd line' '
     + 	done
     + '
     + 
     ++test_expect_success '--exclude-promisor-objects with ^@ on missing object' '
     ++	rm -rf repo &&
     ++	test_create_repo repo &&
     ++	test_commit -C repo foo &&
     ++	test_commit -C repo bar &&
     ++
     ++	COMMIT=$(git -C repo rev-parse foo) &&
     ++	promise_and_delete "$COMMIT" &&
     ++
     ++	git -C repo config core.repositoryformatversion 1 &&
     ++	git -C repo config extensions.partialclone "arbitrary string" &&
     ++
     ++	# Ensure that "$COMMIT^@" is handled gracefully even though the
     ++	# actual commits are missing.
     ++	git -C repo rev-list --exclude-promisor-objects "$COMMIT^@" >out &&
     ++	test_must_be_empty out
     ++'
     ++
     + test_expect_success 'single promisor remote can be re-initialized gracefully' '
     + 	# ensure one promisor is in the promisors list
     + 	rm -rf repo &&
  9:  0b27860478 =  9:  cccd36137f pack-bitmap: handle missing bitmap for base MIDX
 10:  428a3a006b ! 10:  376a6581cb bisect: ensure non-NULL `head` before using it

@@ Commit message
Later, that variable is passed to repo_get_oid() and starts_with(), both of which would dereference the NULL pointer.

- The scenario "refs_resolve_ref_unsafe() returns NULL but
- repo_get_oid() succeeds" can happen when HEAD is a detached bare OID
- that the ref backend cannot resolve symbolically (a potential edge case
- with the reftable backend) but the OID itself is valid. In this case,
- the bisect-start file does not yet exist (this is a fresh "git bisect
- start"), so the else branch is taken with the NULL head.
-
- Simply assign "HEAD" to head as a fallback to address this.
-
- Pointed out by Coverity.
-
- Assisted-by: Claude Opus 4.6
+ A concrete trigger for refs_resolve_ref_unsafe() returning NULL while
+ repo_get_oid() succeeds could not be constructed against the ref
+ backends currently in the tree; the naive case (a symbolic HEAD pointing
+ at a nonexistent branch, in either the files or the reftable backend)
+ fails in both calls consistently and returns via the existing
+ error(_("bad HEAD - I need a HEAD")) path. Coverity, however, flags
+ the leftover use of head after the outer if (!head) on a formal
+ reading: head is still NULL at that point, and both `starts_with(head,
+ ...) and the second repo_get_oid(..., head, ...)` in the else-branch
+ would dereference it if that state were ever reached.
+
+ Removing the outer check would risk regressing to a crash if a future
+ ref backend ever manages to hit the "returns NULL for HEAD but has a
+ valid OID for HEAD" state. Assigning the literal string "HEAD" as a
+ safe fallback documents the intent and satisfies the analyzer without
+ changing behavior in any code path we can currently reach.
+
+ Assisted-by: Claude Opus 4.7

## builtin/bisect.c ##
11: 9f3a239484 ! 11: e581bc91ee shallow: fix NULL dereference

     @@ Commit message
      
       ## shallow.c ##
      @@ shallow.c: static int write_one_shallow(const struct commit_graft *graft, void *cb_data)
     + 		struct commit *c = lookup_commit(the_repository, &graft->oid);
       		if (!c || !(c->object.flags & SEEN)) {
       			if (data->flags & VERBOSE)
     - 				printf("Removing %s from .git/shallow\n",
     +-				printf("Removing %s from .git/shallow\n",
      -				       oid_to_hex(&c->object.oid));
     -+				       oid_to_hex(&graft->oid));
     ++				printf("Removing %s from .git/shallow\n", hex);
       			return 0;
       		}
       	}

-: ---------- > 12: 2ef74b52fa shallow: give write_one_shallow() its own hex buffer

gitgitgadget

From: Johannes Schindelin <[email protected]>

The repo_get_commit_tree() function can return NULL when a commit's tree object is not available (e.g., the commit was parsed but its maybe_tree field is unset and the commit is not in the commit-graph). In cmd_diff(), the return value is immediately dereferenced via ->object without a NULL check, which would crash if the tree cannot be loaded.

Add an explicit NULL check and die with a descriptive message.

Pointed out by Coverity.

---
 builtin/diff.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

index 4b46e394ce..18b1083e98 100644
@@ -579,9 +579,13 @@ int cmd_diff(int argc,
 		obj = deref_tag(the_repository, obj, NULL, 0);
 		if (!obj)
 			die(_("invalid object '%s' given."), name);
-		if (obj->type == OBJ_COMMIT)
-			obj = &repo_get_commit_tree(the_repository,
-						    ((struct commit *)obj))->object;
+		if (obj->type == OBJ_COMMIT) {
+			struct tree *tree = repo_get_commit_tree(
+				the_repository, (struct commit *)obj);
+			if (!tree)
+				die(_("unable to read tree object for commit '%s'"), name);
+			obj = &tree->object;
+		}
 
 		if (obj->type == OBJ_TREE) {
 			if (sdiff.skip && bitmap_get(sdiff.skip, i))

gitgitgadget

From: Johannes Schindelin <[email protected]>

The outer loop in diffcore_merge_broken() sets q->queue[j] to NULL when it merges a broken pair back together, and has a NULL check to skip such entries on subsequent iterations. The inner loop, however, lacks this guard: when it scans forward looking for a matching peer, it can encounter a slot that was NULLed by a previous outer-loop iteration and dereference it unconditionally.

In practice this requires at least two broken pairs whose peers both survive rename/copy detection and appear later in the queue, which is rare but not impossible.

Add the same if (!pp) continue guard to the inner loop.

Pointed out by Coverity.

---
 diffcore-break.c | 2 ++
 1 file changed, 2 insertions(+)

index 17b5ad1fed..b5bcc956cc 100644
@@ -289,6 +289,8 @@ void diffcore_merge_broken(void)
 			 */
 			for (j = i + 1; j < q->nr; j++) {
 				struct diff_filepair *pp = q->queue[j];
+				if (!pp)
+					continue;
 				if (pp->broken_pair &&
 				    !strcmp(pp->one->path, pp->two->path) &&
 				    !strcmp(p->one->path, pp->two->path)) {

gitgitgadget

From: Johannes Schindelin <[email protected]>

The remote_tracking() function unconditionally dereferences remote->fetch without checking whether remote is NULL.

In practice, this never happens because the only caller (apply_cas()) guards the calls to this function by checking the use_tracking and use_tracking_for_rest attributes.

However, it requires quite involved reasoning to reach that conclusion, and is therefore fragile. Just return -1 ("no tracking ref") when there is no remote to work with.

Pointed out by Coverity.

---
 remote.c | 2 ++
 1 file changed, 2 insertions(+)

index 00723b385e..887e388f9c 100644
@@ -2681,6 +2681,8 @@ static int remote_tracking(struct remote *remote, const char *refname,
 {
 	char *dst;
 
+	if (!remote)
+		BUG("remote_tracking() called with NULL remote");
 	dst = apply_refspecs(&remote->fetch, refname);
 	if (!dst)
 		return -1; /* no tracking ref for refname at remote */

gitgitgadget

From: Johannes Schindelin <[email protected]>

When reftable_new_stack() fails partway through initialization (e.g., reftable_buf_addstr returns an OOM error before reftable_buf_detach assigns p->list_file), it jumps to the error path which calls reftable_stack_destroy(p). At that point, p->list_file is still NULL because the detach never happened.

reftable_stack_destroy() passes st->list_file unconditionally to read_lines(), which calls open(filename, O_RDONLY). Passing NULL to open() is undefined behavior and will typically crash.

Guard the read_lines() call with a NULL check on st->list_file. When list_file is NULL, there are no table files to clean up anyway, so skipping read_lines is the correct behavior.

Pointed out by Coverity.

---
 reftable/stack.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index 1fba96ddb3..3fc3c0b2d1 100644
@@ -171,7 +171,8 @@ void reftable_stack_destroy(struct reftable_stack *st)
 		st->merged = NULL;
 	}
 
-	err = read_lines(st->list_file, &names);
+	if (st->list_file)
+		err = read_lines(st->list_file, &names);
 	if (err < 0) {
 		REFTABLE_FREE_AND_NULL(names);
 	}

gitgitgadget

From: Johannes Schindelin <[email protected]>

The split_mbox() function calls fileno(f) to check whether the input is a terminal, but the NULL check for f (from fopen()) does not happen until later. When the file cannot be opened, f is NULL, and fileno(NULL) is undefined behavior, typically crashing with a segmentation fault.

Move the NULL check above the isatty()/fileno() call so the error path is taken before any use of the potentially-NULL handle.

Pointed out by Coverity.

---
 builtin/mailsplit.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index 264df6259a..0993418e63 100644
@@ -225,14 +225,14 @@ static int split_mbox(const char *file, const char *dir, int allow_bare,
 	FILE *f = !strcmp(file, "-") ? stdin : fopen(file, "r");
 	int file_done = 0;
 
-	if (isatty(fileno(f)))
-		warning(_("reading patches from stdin/tty..."));
-
 	if (!f) {
 		error_errno("cannot open mbox %s", file);
 		goto out;
 	}
 
+	if (isatty(fileno(f)))
+		warning(_("reading patches from stdin/tty..."));
+
 	do {
 		peek = fgetc(f);
 		if (peek == EOF) {

gitgitgadget

From: Johannes Schindelin <[email protected]>

When lookup_commit_reference_by_name() is called to find the first bad commit, the result is passed to repo_format_commit_message() immediately, which dereferences commit without checking for NULL.

However, the commit could be NULL, even though in practice this is unlikely because bisect_successful() is only called after a successful bisect run has identified the bad commit, but the ref could still become dangling due to a concurrent gc or repository corruption.

Pointed out by Coverity.

---
 builtin/bisect.c | 5 +++++
 1 file changed, 5 insertions(+)

index e7c2d2f3bb..408e0f414e 100644
@@ -663,6 +663,11 @@ static int bisect_successful(struct bisect_terms *terms)
 
 	refs_read_ref(get_main_ref_store(the_repository), bad_ref, &oid);
 	commit = lookup_commit_reference_by_name(bad_ref);
+	if (!commit) {
+		error(_("could not find commit for '%s'"), bad_ref);
+		free(bad_ref);
+		return BISECT_FAILED;
+	}
 	repo_format_commit_message(the_repository, commit, "%s", &commit_name,
 				   &pp);
 

gitgitgadget

From: Johannes Schindelin <[email protected]>

The peel_committish() function calls repo_peel_to_type() to convert the given object to a commit, but does not check the return value. When the object exists but cannot be peeled to a commit (e.g., a tree or blob OID is passed as --onto), the return value is NULL. Add an explicit NULL check and die with a descriptive message in that case.

Pointed out by Coverity.

---
 replay.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

index da531d5bc6..b38cd5efe4 100644
@@ -36,12 +36,16 @@ static struct commit *peel_committish(struct repository *repo,
 {
 	struct object *obj;
 	struct object_id oid;
+	struct commit *commit;
 
 	if (repo_get_oid(repo, name, &oid))
 		die(_("'%s' is not a valid commit-ish for %s"), name, mode);
 	obj = parse_object_or_die(repo, &oid, name);
-	return (struct commit *)repo_peel_to_type(repo, name, 0, obj,
-						  OBJ_COMMIT);
+	commit = (struct commit *)repo_peel_to_type(repo, name, 0, obj,
+						    OBJ_COMMIT);
+	if (!commit)
+		die(_("'%s' does not point to a commit for %s"), name, mode);
+	return commit;
 }
 
 static char *get_author(const char *message)

gitgitgadget

From: Johannes Schindelin <[email protected]>

This function resolves revision suffixes like commit^@ (all parents), commit^! (commit minus parents), and commit^-N (exclude Nth parent). It calls get_reference() in a loop to peel through tag objects until it reaches a commit.

The existing NULL check after get_reference() only handles the ignore_missing case, but get_reference() can return NULL through three distinct paths:

1. revs->ignore_missing: the caller asked to silently skip missing objects.

2. revs->exclude_promisor_objects: the object is a lazy promisor object that should be excluded from the walk.

3. revs->do_not_die_on_missing_objects: the caller wants to record missing OIDs for later reporting (used by git rev-list --missing=print) rather than dying.

In the latter two instances, the code falls through to dereference the NULL pointer.

Handle all three cases explicitly:

- ignore_missing: return 0, matching the existing behavior and the pattern in handle_revision_arg().

- do_not_die_on_missing_objects: return 0. The missing OID has already been recorded in revs->missing_commits by get_reference(). Returning 0 is consistent with handle_revision_arg() and process_parents(), both of which continue without error when this flag is set. The broader codebase pattern for this flag is "record and continue": list-objects.c, builtin/rev-list.c, and process_parents all skip the die/error and keep walking.

- everything else (only the exclude_promisor_objects case in practice): return -1, consistent with handle_revision_arg() where the condition only matches ignore_missing or do_not_die_on_missing_objects, falling through to ret = -1 for the promisor case.

Note: the callers of add_parents_only() in handle_revision_pseudo_opt() treat any nonzero return as "handled" (if (add_parents_only(...)) { ret = 0; }), so the -1 for the promisor case is indistinguishable from success there. This means a promisor-excluded tag target referenced via commit^@ would be silently skipped rather than producing an error. This is a pre-existing limitation of the caller's return value handling and not made worse by this change; the alternative (a NULL dereference crash) _would be_ strictly worse.

Pointed out by Coverity.

---
 revision.c               |  9 +++++++--
 t/t0410-partial-clone.sh | 18 ++++++++++++++++++
 2 files changed, 25 insertions(+), 2 deletions(-)

index e91d7e1f11..7f3999b551 100644
@@ -1903,8 +1903,13 @@ static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,
 		return 0;
 	while (1) {
 		it = get_reference(revs, arg, &oid, 0);
-		if (!it && revs->ignore_missing)
-			return 0;
+		if (!it) {
+			if (revs->ignore_missing)
+				return 0;
+			if (revs->do_not_die_on_missing_objects)
+				return 0;
+			return -1;
+		}
 		if (it->type != OBJ_TAG)
 			break;
 		if (!((struct tag*)it)->tagged)

index dff442da20..cc070019be 100755
@@ -489,6 +489,24 @@ test_expect_success 'rev-list dies for missing objects on cmd line' '
 	done
 '
 
+test_expect_success '--exclude-promisor-objects with ^@ on missing object' '
+	rm -rf repo &&
+	test_create_repo repo &&
+	test_commit -C repo foo &&
+	test_commit -C repo bar &&
+
+	COMMIT=$(git -C repo rev-parse foo) &&
+	promise_and_delete "$COMMIT" &&
+
+	git -C repo config core.repositoryformatversion 1 &&
+	git -C repo config extensions.partialclone "arbitrary string" &&
+
+	# Ensure that "$COMMIT^@" is handled gracefully even though the
+	# actual commits are missing.
+	git -C repo rev-list --exclude-promisor-objects "$COMMIT^@" >out &&
+	test_must_be_empty out
+'
+
 test_expect_success 'single promisor remote can be re-initialized gracefully' '
 	# ensure one promisor is in the promisors list
 	rm -rf repo &&

gitgitgadget

From: Johannes Schindelin <[email protected]>

When prepare_midx_bitmap_git() is called to load the bitmap for a chained MIDX's base layer, if the base MIDX does not have an associated bitmap file (e.g., it was not generated, or was deleted by gc), the return value is NULL. It is then stored in bitmap_git->base and immediately dereferenced on the next line.

This can happen in practice with incremental MIDX chains: the base MIDX may have been written without --write-bitmap-index, or the bitmap may have been pruned while the incremental layer's bitmap still references it.

Check the return value and go to the cleanup label (which unmaps the current bitmap and returns -1) so the caller falls back to non-bitmap object enumeration, matching the handling of other bitmap loading failures in the same function.

Pointed out by Coverity.

---
 pack-bitmap.c | 4 ++++
 1 file changed, 4 insertions(+)

index e8a82945cc..ca7998c10b 100644
@@ -523,6 +523,10 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
 
 	if (midx->base_midx) {
 		bitmap_git->base = prepare_midx_bitmap_git(midx->base_midx);
+		if (!bitmap_git->base) {
+			warning(_("could not open bitmap for base MIDX"));
+			goto cleanup;
+		}
 		bitmap_git->base_nr = bitmap_git->base->base_nr + 1;
 	} else {
 		bitmap_git->base_nr = 0;

gitgitgadget

From: Johannes Schindelin <[email protected]>

When refs_resolve_ref_unsafe() is called to resolve HEAD, and returns NULL (e.g., HEAD does not exist as a proper ref), the code falls back to repo_get_oid("HEAD") to try to resolve the OID directly. If that succeeds, execution continues with head still set to NULL.

Later, that variable is passed to repo_get_oid() and starts_with(), both of which would dereference the NULL pointer.

A concrete trigger for refs_resolve_ref_unsafe() returning NULL while repo_get_oid() succeeds could not be constructed against the ref backends currently in the tree; the naive case (a symbolic HEAD pointing at a nonexistent branch, in either the files or the reftable backend) fails in both calls consistently and returns via the existing error(_("bad HEAD - I need a HEAD")) path. Coverity, however, flags the leftover use of head after the outer if (!head) on a formal
reading: head is still NULL at that point, and both starts_with(head, ...) and the second repo_get_oid(..., head, ...) in the else-branch would dereference it if that state were ever reached.

Removing the outer check would risk regressing to a crash if a future ref backend ever manages to hit the "returns NULL for HEAD but has a valid OID for HEAD" state. Assigning the literal string "HEAD" as a safe fallback documents the intent and satisfies the analyzer without changing behavior in any code path we can currently reach.

---
 builtin/bisect.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

index 408e0f414e..dccf0be6bb 100644
@@ -811,9 +811,11 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, int argc,
 	 */
 	head = refs_resolve_ref_unsafe(get_main_ref_store(the_repository),
 				       "HEAD", 0, &head_oid, &flags);
-	if (!head)
+	if (!head) {
 		if (repo_get_oid(the_repository, "HEAD", &head_oid))
 			return error(_("bad HEAD - I need a HEAD"));
+		head = "HEAD";
+	}
 
 	/*
 	 * Check if we are bisecting

gitgitgadget

From: Johannes Schindelin <[email protected]>

After write_one_shallow() calls lookup_commit() to find the commit object for a shallow graft entry, it then checks if (!c || ...). Inside that block, when the VERBOSE flag is set, it prints the OID being removed, via c->object.oid. But c can be NULL (the first condition in the || check).

This happens when a shallow graft entry references a commit object that is not in the object store (e.g., after a partial fetch or in a corrupted repository). In that case, lookup_commit() returns NULL because the object cannot be found, the SEEN_ONLY check correctly decides to remove this entry from .git/shallow, but the verbose message crashes before the removal can complete.

Use graft->oid instead of c->object.oid for the message. The graft entry's OID is the same value (it was used as the lookup key) and is always available regardless of whether the commit object exists.

Pointed out by Coverity.

---
 shallow.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index 07cae44ae5..2f96db5170 100644
@@ -370,8 +370,7 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)
 		struct commit *c = lookup_commit(the_repository, &graft->oid);
 		if (!c || !(c->object.flags & SEEN)) {
 			if (data->flags & VERBOSE)
-				printf("Removing %s from .git/shallow\n",
-				       oid_to_hex(&c->object.oid));
+				printf("Removing %s from .git/shallow\n", hex);
 			return 0;
 		}
 	}

gitgitgadget

From: Johannes Schindelin <[email protected]>

The previous fix reuses the local hex variable that is already computed at the top of write_one_shallow(). That works today, but oid_to_hex() returns a pointer into a small rotating buffer, so it is not stable across an unrelated call to oid_to_hex() from the same thread. A future edit that adds such a call between the assignment and the last user of hex would silently corrupt the output.

Move write_one_shallow() off the rotating buffer entirely by using a local buffer instead. The current users of that hex variable are unchanged.

---
 shallow.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

index 2f96db5170..c567cc3c69 100644
@@ -359,7 +359,9 @@ struct write_shallow_data {
 static int write_one_shallow(const struct commit_graft *graft, void *cb_data)
 {
 	struct write_shallow_data *data = cb_data;
-	const char *hex = oid_to_hex(&graft->oid);
+	char hex[GIT_MAX_HEXSZ + 1];
+
+	oid_to_hex_r(hex, &graft->oid);
 	if (graft->nr_parent != -1)
 		return 0;
 	if (data->flags & QUICK) {

gitgitgadget

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

I found everything including the new patch good. Unless others find more issues in this round in a few days, let's mark the topic for 'next'.

Thanks.