Re: [PATCH] revision: fix missing null for freed memory

["D. Ben Knoble" <ben.knoble@gmail.com>]

On Tue, Feb 11, 2025 at 2:31 PM D. Ben Knoble <ben.knoble@gmail.com> wrote:
>
> On Tue, Feb 11, 2025 at 2:56 AM Patrick Steinhardt <ps@pks.im> wrote:
> >
> > On Fri, Feb 07, 2025 at 10:17:02PM -0800, Emily M Klassen wrote:
> > > "git log --graph --no-graph" missed cleaning up the output_prefix and
> > > output_prefix_data pointers. This resulted in a segfault when using "--patch",
> > > "--name-status" or "--name-only", as the output_prefix_data continued to be in
> > > use after free()
> > >
> > > Signed-off-by: Emily M Klassen <forivall@gmail.com>
> > > ---
> > > I previously reported this a few hours ago, and ended up digging in and figuring
> > > it out. I'll make sure to bottom reply in the follow ups to this patch.
> >
> > Do we know when this bug was introduced? Is it a recent regression or a
> > long-standing issue? Might be nice to point out in the commit message if
> > we do know.
> >
> > Patrick
>
> Out of morbid curiosity, I've started bisecting this, but it's proven
> slightly more subtle than I anticipated. If nobody beats me to it,
> I'll post my results when I have them.
>
> --
> D. Ben Knoble

2.{30,35}.0 fails to recognize --no-graph, so I checked "git log --grep no-graph
origin/master" with "git describe --contains" and decided that 2.36.0 was first
release recognizing --no-graph, but it didn't build for me (possibly an issue on
my end). I got 2.37.0 built, and it was "good," so that's where I started.

Here's my "bisect run" script.

    #! /bin/sh -x
    make || exit 125
    # segfault has exit >128
    ./bin-wrappers/git --no-pager log -2 --graph --no-graph --patch
--cc || exit 1

The --cc is important, since this repro logs from where the bisect is! Without
it, if the head commits are both merges (likely), the repro will accidentally
mark the commit as good when looking further for a commit with a patch will
fail. Omitting -2 might work, too, but that makes "git log" take longer.

With --first-parent on bisect, we find 3eb4cc451e (Merge branch
'jk/output-prefix-cleanup', 2024-10-10), which looks like a reasonable
candidate. Restarting between that commit and it's first parent, we get
19752d9c91 (diff: return line_prefix directly when possible, 2024-10-03). That
commit actually looks relatively innocuous, and I haven't tracked down how it
really impacts the problem or fix.

Perhaps the topic merge is more helpful to folks in assessing where the bug came
from. Otherwise, it may be that --cc is not enough and we should
bisect with --diff-merges=1 or something else guaranteed to generate a
diff and trip the bug.

To my shame, I didn't save the log from the --first-parent bisect from 2.37.0 to
388218fac7 (The ninth batch, 2025-02-10), but I did save the smaller one.

    # bad: [3eb4cc451ed97123ff76e183a5be8a7dc164d1f6] Merge branch
'jk/output-prefix-cleanup'
    # good: [31bc4454de66c22bc8570fd3af52a99843ac69b0] Merge branch
'ps/leakfixes-part-8'
    git bisect start '@' '@^'
    # good: [436728fe9d75d05fa2439f867ca2039012b86e69] diff: return
const char from output_prefix callback
    git bisect good 436728fe9d75d05fa2439f867ca2039012b86e69
    # bad: [1164e270b5af80516625b628945ec7365d992055] diff: store
graph prefix buf in git_graph struct
    git bisect bad 1164e270b5af80516625b628945ec7365d992055
    # bad: [19752d9c912478b9eef0bd83c2cf6da98974f536] diff: return
line_prefix directly when possible
    git bisect bad 19752d9c912478b9eef0bd83c2cf6da98974f536
    # first bad commit: [19752d9c912478b9eef0bd83c2cf6da98974f536]
diff: return line_prefix directly when possible

-- 
D. Ben Knoble