On Mon, Feb 10, 2025 at 8:02 AM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Emily M Klassen <forivall@xxxxxxxxx> 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.
>
> 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!
>
> > Signed-off-by: Emily M Klassen <forivall@xxxxxxxxx>
> > ---
> > 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")) {
