Re: [BUGREPORT] git diff-tree --cc SEGFAUTs
- From
Jeff King <peff@peff.net>
- Date
- Jan 4, 2025, 00:31 UTC
- Message-ID
- <20250104003154.GB3244554@coredump.intra.peff.net>
- In-Reply-To
- <CAKk8isrz1NQ=3=2aZ3tANymo0eSsCy=r6W5yKgn6gxmOom54CA@mail.gmail.com>
On Fri, Jan 03, 2025 at 03:34:58PM -0800, Wink Saville wrote:
Show 19 quoted lines
> But having to protect yourself is unobvious and especially if it isn't necessary > when using the `fetch_paths_generic`. > > In addition, from strbuf.h `buf` is never NULL: > > " > * strbufs have some invariants that are very important to keep in mind: > * > * - The `buf` member is never NULL, so it can be used in any usual C > * string operations safely. strbufs _have_ to be initialized either by > * `strbuf_init()` or by `= STRBUF_INIT` before the invariants, though. > * > " > > So I'd say this could be considered a bug in git at least in how > combine_diff_path > is being managed. I assume you agree that neither find_paths_generic or > find_paths_multitree are adhering to at least that strbuf invariant and I wonder > if the other strbuf invariants are being upheld.
The strbuf invariant can only be held on strbufs which have been initialized, and this one has not. I don't think it's wrong to have variables which not (yet) been initialized. It can make for a fragile interface, though, if uninitialized struct members are exposed widely.
I'm not sure how wide this case is. It's mostly an internal combine-diff data structure, though it looks like it gets exposed to other code in a few spots (though nobody outside of combine-diff.c currently looks at the parent paths at all).
So I wouldn't call it a bug, as the internals of Git are not part of the public interface and there is no user-visible behavior problem without patching. But I doubt anybody would object to a patch making the API less fragile if it can be done cheaply and easily. And strbufs are designed to be cheap to initialize. So something like (completely untested):
diff --git a/combine-diff.c b/combine-diff.c index 641bc92dbd..452b5f5beb 100644 --- a/combine-diff.c +++ b/combine-diff.c @@ -67,9 +67,9 @@ static struct combine_diff_path *intersect_paths( p->parent[n].mode = q->queue[i]->one->mode; p->parent[n].status = q->queue[i]->status; + strbuf_init(&p->parent[n].path, 0); if (combined_all_paths && filename_changed(p->parent[n].status)) { - strbuf_init(&p->parent[n].path, 0); strbuf_addstr(&p->parent[n].path, q->queue[i]->one->path); } @@ -92,9 +92,7 @@ static struct combine_diff_path *intersect_paths( /* p->path not in q->queue[]; drop it */ *tail = p->next; for (j = 0; j < num_parent; j++) - if (combined_all_paths && - filename_changed(p->parent[j].status)) - strbuf_release(&p->parent[j].path); + strbuf_release(&p->parent[j].path); free(p); continue; } @@ -1645,9 +1643,7 @@ void diff_tree_combined(const struct object_id *oid, struct combine_diff_path *tmp = paths; paths = paths->next; for (i = 0; i < num_parent; i++) - if (rev->combined_all_paths && - filename_changed(tmp->parent[i].status)) - strbuf_release(&tmp->parent[i].path); + strbuf_release(&tmp->parent[i].path); free(tmp); } might help the uninitialized-pointer issue. OTOH it is not really solving the more fundamental problem, which is that p->parent[i].path is only sometimes useful (we do not fill it in if it would just be the same as p->path, so the patch only changes it from uninitialized memory into an empty strbuf). And that is probably not something we want to change, as allocating duplicates of each path may be expensive. Probably we'd be better to encapsulate it in a function which falls back to p->path automatically. But then, AFAICT there are only two sites (both inside combine-diff.c) which look at it, so it would mostly be hypothetical future-proofing. I dunno. > So, should this bug be "closed" and a new one "created"? > > Actually, using the mailing list to identify bugs and initially discuss > them, seems fine. But is there a place where there is a list of current bugs and > their state? No, there's no bug tracker for the project[1]. Discussion may lead to a patch or not, which may be applied or not, but there is no formal classification of "open" or "fixed" or "won't fix". -Peff [1] Some folks seem to be using: https://git.issues.gerritcodereview.com/issues?q=status:open but I don't know how active it is. I never use it and had to dig out the link to https://crbug.com, which now redirects there, from a message from 2017. There's some recent activity, but I wouldn't count on opening something there to get wide attention.