Re: [PATCH 3/3] commit-graph.c: handle corrupt/missing trees
- From
Jeff King <peff@peff.net>
- Date
- Sep 6, 2019, 17:34 UTC
- Message-ID
- <20190906173436.GE23181@sigill.intra.peff.net>
- In-Reply-To
- <20190906154214.GA3657@syl.local>
On Fri, Sep 06, 2019 at 11:42:14AM -0400, Taylor Blau wrote:
Show 11 quoted lines
> > > struct object_id *get_commit_tree_oid(const struct commit *commit)
> > > {
> > > - return &get_commit_tree(commit)->object.oid;
> > > + struct tree *tree = get_commit_tree(commit);
> > > + return tree ? &tree->object.oid : NULL;
> > > }
>
> You mentioned in the version of this series that is rebased on GitHub's
> fork that it may be worth putting this hunk in a separate commit
> entirely. I don't disagree, so if there are other comments that merit a
> reroll of this, I'm happy to pull this change out as 3/4.Yeah, I could go either way on that, I think. I was thinking it might be fixing other callsites, but it seems that nobody else bothers to check for NULL anyway. But being in its own commit, we could explain that.
Show 22 quoted lines
> > This one in theory benefits lots of other callsites, too, since it means
> > we'll actually return NULL instead of nonsense like "8". But grepping
> > around for calls to this function, I found literally zero of them
> > actually bother checking for a NULL result. So there are probably dozens
> > of similar segfaults waiting to happen in other code paths.
> > Discouraging.
>
> Discouraging indeed. I think that you suggest it below, but perhaps the
> right thing to do here is implement 'get_commit_tree_oid()' as follows:
>
> struct object_id *get_commit_tree_oid(const struct commit *commit)
> {
> struct tree *tree = get_commit_tree(commit);
> if (!tree)
> die(_("unable to get tree from commit %s"),
> oid_to_hex(&commit->object.oid));
> return &tree->object.oid;
> }
>
> Which then puts the onus on the *caller* to check their commit pointer
> to make sure that it has a legit tree in it, unless they're OK with
> dying.Yeah, I agree that would prevent segfaults (and is similar to what René proposed for tags with a similar situation). It does feel like a step backwards in terms of lib-ification. But maybe it's a belt-and-suspenders on top of trying to catch this case at the parsing stage, too.
> All of that said, I don't know if I think it's worth holding this series > up on the above in the meantime. I do think that it (or something like > it) is generally worth doing, but I'm not sure that now is the time to > do it.
I'd agree with that, and I think it's sensible to take your patches with the extra tree check. We can rip it out later if it becomes redundant.
-Peff