Re: [BUGREPORT] git diff-tree --cc SEGFAUTs
From: Wink Saville <hidden>
Date: 2025-01-03 23:35:10
Subsystem:
the rest · Maintainer:
Linus Torvalds
On Fri, Jan 3, 2025 at 12:46 PM Jeff King [off-list ref] wrote:
quoted hunk ↗ jump to hunk
On Fri, Jan 03, 2025 at 11:28:47AM -0800, Wink Saville wrote:quoted
`git diff-tree --cc` SEGFAUTs after adding trace_printf to diff_tree_combined.Hmm, is it really a bug in Git if you had to add new code which contains the bug? :)quoted
@@ -1595,8 +1597,16 @@ void diff_tree_combined(const struct object_id *oid, } /* find out number of surviving paths */ - for (num_paths = 0, p = paths; p; p = p->next) + trace_printf("Wink diff_tree_combined: find number of surviving paths num_parent=%d\n", num_parent); + for (num_paths = 0, p = paths; p; p = p->next) { + trace_printf("Wink diff_tree_combined: num_paths=%d &p=%p mode=%0x, oid=%s path=%s\n", num_paths, p, p->mode, oid_to_hex(&p->oid), p->path); + for (i = 0; i < num_parent; i++) { + trace_printf("Wink diff_tree_combined: &p->parent[%d]=%p status=%c mode=%x oid=%s path.buf=%p contents path.buf=%s\n", + i, &p->parent[i], p->parent[i].status, p->parent[i].mode, oid_to_hex(&p->parent[i].oid), p->parent[i].path.buf, p->parent[i].path.buf); + }The parent "path" strbufs are only initialized in intersect_paths() if combined_all_paths is set, and if there was an actual path change (a copy or rename). So you'd probably need something like this:diff --git a/combine-diff.c b/combine-diff.c index 455bc19087..1e58809c4e 100644 --- a/combine-diff.c +++ b/combine-diff.c@@ -1601,8 +1601,11 @@ void diff_tree_combined(const struct object_id *oid, for (num_paths = 0, p = paths; p; p = p->next) { trace_printf("Wink diff_tree_combined: num_paths=%d &p=%p mode=%0x, oid=%s path=%s\n", num_paths, p, p->mode, oid_to_hex(&p->oid), p->path); for (i = 0; i < num_parent; i++) { + const char *path = rev->combine_all_paths && + filename_changed(p->parent[i].status) ? + p->parent[i].path.buf : NULL; trace_printf("Wink diff_tree_combined: &p->parent[%d]=%p status=%c mode=%x oid=%s path.buf=%p contents path.buf=%s\n", - i, &p->parent[i], p->parent[i].status, p->parent[i].mode, oid_to_hex(&p->parent[i].oid), p->parent[i].path.buf, p->parent[i].path.buf); + i, &p->parent[i], p->parent[i].status, p->parent[i].mode, oid_to_hex(&p->parent[i].oid), path, path); } num_paths++; } -Peff
TYVM! That worked but changed the name and fixed a typo in `combined_all_paths`:
wink@3900x 25-01-03T23:06:08.344Z:~/data/prgs/forks/git
(wink-segfault-with-minimal-changes)
$ git diff
diff --git a/combine-diff.c b/combine-diff.c
index 455bc19087..70394c3350 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -1601,8 +1601,9 @@ void diff_tree_combined(const struct object_id *oid,
for (num_paths = 0, p = paths; p; p = p->next) {
trace_printf("Wink diff_tree_combined: num_paths=%d
&p=%p mode=%0x, oid=%s path=%s\n", num_paths, p, p->mode,
oid_to_hex(&p->oid), p->path);
for (i = 0; i < num_parent; i++) {
+ const char *parent_path =
rev->combined_all_paths && filename_changed(p->parent[i].status) ?
p->parent[i].path.buf : NULL;
trace_printf("Wink diff_tree_combined:
&p->parent[%d]=%p status=%c mode=%x oid=%s path.buf=%p contents
path.buf=%s\n",
- i, &p->parent[i],
p->parent[i].status, p->parent[i].mode, oid_to_hex(&p->parent[i].oid),
p->parent[i].path.buf, p->parent[i].path.buf);
+ i, &p->parent[i],
p->parent[i].status, p->parent[i].mode, oid_to_hex(&p->parent[i].oid),
parent_path, parent_path);
}
num_paths++;
}
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. 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? -- wink