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.

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