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

From: Emily Klassen <forivall@gmail.com>
Date: Feb 10, 2025, 20:56 UTC
Message-ID: <CADY4h_o_wfUpjSBhWa9TPU_G-G8qpENpUeOKGQDY8dq6Zb2+qg@mail.gmail.com>
In-Reply-To: <xmqqtt91dbzt.fsf@gitster.g>

On Mon, Feb 10, 2025 at 8:02 AM Junio C Hamano <gitster@pobox.com> wrote:

Show 13 quoted lines

>
> Emily M Klassen <forivall@gmail.com> writes:
>
> > Subject: Re: [PATCH] revision: fix missing null for freed memory
> >
> > "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()
>
> Rereading the title, I cannot make sense out of "fix missing null"
> and guess what it wants to say.  Is "null" here used as a verb to
> mean "to assign a NULL to a variable that points at ..."?

Yeah, this was meant to say something like "fix missing null assignment after freeing graph data", and I didn't really have the energy to think of a better summary at the time.

Show 14 quoted lines

>
>     revision: clear graph callback upon "--no-graph"
>
>     "git log --graph --no-graph" first populates the .output_prefix
>     member of diffopt, which is a callback function, to compute
>     "--graph" header, and then discards the data the callback needs
>     to compute the graph header but forgets to clear .output_prefix
>     pointer in response to "--no-graph".  At runtime, we end up
>     calling the function that we should not.
>
>     Clear the member to stop making callback, and for a better
>     hyginene, also clear the pointer pointing at a freed memory.
>
> or something?

Yup, this works well. A small bit of rephrasing for readability:

    revision: clear graph prefix callback upon "--no-graph"

    "git log --graph --no-graph" misses some cleanup: handling
    "--graph", it assigns the .output_prefix member of diffopt, which
    is a callback function to compute the graph prefix when displaying
    a diff. Then, when handling "--no-graph" it discards the data the
    callback needs to compute the graph header but forgets to clear
    .output_prefix pointer.  At runtime, we  call the function when we
    should not. It also passes a stale pointer to the data, which leads
    to a segfault when the callback is used for "--patch",
    "--name-status" or "--name-only".

    Clear the member to stop the callback from being called, and for
    hygiene, also clear the pointer pointing at a freed memory.

>
> Other than that, as I said earlier, the patch looks good.
>
> Thanks.

Awesome. I'll also add a test before re-submitting, as mentioned in your other message.

Thanks for the feedback!

Show 22 quoted lines

>
> > 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.
> >
> >  revision.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/revision.c b/revision.c
> > index 474fa1e767..84cb028e11 100644
> > --- a/revision.c
> > +++ b/revision.c
> > @@ -2615,6 +2615,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
> >               graph_clear(revs->graph);
> >               revs->graph = graph_init(revs);
> >       } else if (!strcmp(arg, "--no-graph")) {
> > +             revs->diffopt.output_prefix = NULL;
> > +             revs->diffopt.output_prefix_data = NULL;
> >               graph_clear(revs->graph);
> >               revs->graph = NULL;
> >       } else if (!strcmp(arg, "--encode-email-headers")) {