Re: [BUGREPORT] git diff-tree --cc SEGFAUTs

From: Wink Saville <wink@saville.com>
Date: Jan 3, 2025, 23:34 UTC
Message-ID: <CAKk8isrz1NQ=3=2aZ3tANymo0eSsCy=r6W5yKgn6gxmOom54CA@mail.gmail.com>
In-Reply-To: <20250103204624.GE3212696@coredump.intra.peff.net>

On Fri, Jan 3, 2025 at 12:46 PM Jeff King <peff@peff.net> wrote:

Show 46 quoted lines

>
> On Fri, Jan 03, 2025 at 11:28:47AM -0800, Wink Saville wrote:
>
> > `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? :)
>
> > @@ -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