Re: Potential Null Pointer Dereference detected by static analysis tool

From: Jeff King <peff@peff.net>
Date: Aug 18, 2025, 05:05 UTC
Message-ID: <20250818050545.GA62231@coredump.intra.peff.net>
In-Reply-To: <20250818044807.GA51803@coredump.intra.peff.net>

On Mon, Aug 18, 2025 at 12:48:07AM -0400, Jeff King wrote:

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

So you almost nerd-sniped me into making a series. But the more I dug into the rabbit-hole, the more I turned away in disgust. :)

The set of problems I found are:

  1. What should happen when traversing from HEAD does not find the blob
     in question? Right now we print a blank line, which is...weird.
     Probably we should either print nothing, or return an error. If we
     return an error, should we respect --always? Are we stuck with the
     current dumb behavior because it's a plumbing command?

  2. When we are on an unborn branch, we print a confusing message:

       $ git init
       $ git commit --allow-empty -m foo
       $ git tag foo
       $ git symbolic-ref HEAD refs/heads/unborn
       $ git describe $(echo blob | git hash-object -w --stdin)
       fatal: ambiguous argument 'HEAD': unknown revision or path not in the working tree.
       Use '--' to separate paths from revisions, like this:
       'git <command> [<revision>...] -- [<file>...]'

     We should probably resolve HEAD ourselves and either bail with an
     empty output or an error (depending on what we do for (1) above).

  3. When we do traverse, if process_object() sees that we didn't find a
     commit, we should detect that and either return an empty result or
     an error (again, depending on the behavior of (2) above). This is
     done by checking is_null_oid(&pcd->current_commit) there.

  4. Then we can teach describe_commit() to take a commit rather than an
     oid (and the is_null_oid() check becomes a NULL check).

So it all depends on what to do with (1), and for a feature that IMHO should not even exist in the first place, I had trouble summoning the will-power to make this 4-patch series.

-Peff