Re: [PATCH] pager: fix crash when pager program doesn't exist

From: Jeff King <peff@peff.net>
Date: Nov 20, 2021, 03:42 UTC
Message-ID: <YZhurgbXq7vhDTXC@coredump.intra.peff.net>
In-Reply-To: <20211120023246.7ynehp5v3iypfr6w@cyberdelia>

On Fri, Nov 19, 2021 at 11:32:46PM -0300, Enzo Matsumiya wrote:

Show 9 quoted lines

> > GIT_PAGER=no-such-command git -p log
> > 
> > I had to run it with ASan to trigger a failure, as use-after-free bugs
> > aren't always deterministic.
> 
> Please use my reproducer as it's 100% reliable and consistent (same
> memory regions are affected).
> 
> I couldn't reproduce the issue with yours.

Our reproducers are triggering the same behavior. But it won't be 100% reliable in the sense that the behavior is undefined. Depending upon random details of the allocator, we may get a segfault, or see random trash on the heap, or even see the old data. That's why I suggested using ASan; it poisons the freed memory to reliably detect problems.

But at any rate, yes, it's clear that there is a bug here.

Show 14 quoted lines

> > diff --git a/run-command.c b/run-command.c
> > index f40df01c77..92e00d9455 100644
> > --- a/run-command.c
> > +++ b/run-command.c
> > @@ -21,6 +21,7 @@ void child_process_clear(struct child_process *child)
> > {
> > 	strvec_clear(&child->args);
> > 	strvec_clear(&child->env_array);
> > +	child_process_init(child);
> > }
> > 
> > struct child_to_clean {
> 
> Of course this one works as well. And is more elegant IMHO.

Yeah, I think so, too.

> Should I submit a v2 or will you?

Why don't you put together a v2, and I can review it.

-Peff