Re: Potential Null Pointer Dereference detected by static analysis tool

From: Jeff King <peff@peff.net>
Date: Aug 18, 2025, 04:48 UTC
Message-ID: <20250818044807.GA51803@coredump.intra.peff.net>
In-Reply-To: <7f289d4f-0a9d-480d-a5a9-7f4d4d24626b@web.de>

On Sun, Aug 17, 2025 at 11:27:12AM +0200, René Scharfe wrote:

Show 15 quoted lines

> > @@ -546,7 +544,7 @@ static void process_object(struct object *obj, const char *path, void *data)
> >  
> >  	if (oideq(&pcd->looking_for, &obj->oid) && !pcd->dst->len) {
> >  		reset_revision_walk();
> > -		describe_commit(&pcd->current_commit, pcd->dst);
> > +		describe_commit(pcd->current_commit, pcd->dst);
> 
> pcd->current_commit is initialized to NULL below, but
> traverse_commit_list() without a filter must have set it via our
> process_commit() callback before we get to the describe_commit() call.
> 
> Or are there weird repositories (e.g., just a blob, just a tag) that can
> cause traverse_commit_list() to call its show_object() callback without
> ever calling its show_commit() callback?  I don't see how, but may be
> missing some way.

If there are, then I think the current code would segfault, too. It initializes &pcd->current_commit to the null oid, and then describe_commit() resolves that via lookup_commit_reference(). That would return NULL, and the next line dereferencing the result would segfault.

And it would be a counter-example to the claim that the call to lookup_commit_reference() in describe_commit() never fails. ;)

I think your intuition is right that we could get the traversal code to call show_object() without show_commit() in general. E.g. just:

  git init
  git tag foo $(echo bar | git hash-object -w --stdin)
  git rev-list --all --objects

would do so. But in this code our traversal always starts from HEAD, which must be a commit (and this is enforced by the refs code). So you'd have to corrupt your repository like:

  # do this in two steps since redirecting to .git/HEAD breaks the
  # repository for a moment!
  tag=$(git rev-parse foo)
  echo $tag >.git/HEAD

And indeed, the current code does then segfault on "git describe foo" at the spot I mentioned. Even though the repository state here is unexpected and corrupt, I do think we should probably be more defensive and avoid the segfault.

I also find it a bit funny that describing a blob only walks from HEAD (and not say, all refs or ones matching --match/--exclude, etc). It is documented that way, though. Actually, I find the whole feature a bit pointless now that we have the diff "--find-object" option. Reading 15af58c1ad (diffcore: add a pickaxe option to find a specific blob, 2018-01-04), I think the git-describe behavior is mostly considered a mistake (which we have to keep around for historical reasons). I guess another possible candidate for removing in Git 3.0. :)

-Peff