From: Jeff King <peff@peff.net>
Date: Wed, 24 Nov 2021 15:51:00 GMT
Subject: Re: [PATCH v2] pager: fix crash when pager program doesn't exist
Message-ID: <YZ5fZGyMj/lAuy7i@coredump.intra.peff.net>
In-Reply-To: <211124.865ysie2br.gmgdl@evledraar.gmail.com>

On Wed, Nov 24, 2021 at 02:55:20AM +0100, Ævar Arnfjörð Bjarmason wrote:

> I do wonder re [2] and [3] if a simpler and self-contained/isolated
> patch in this area might not be a mirage of sorts. I.e. to know whether
> the approach in [2] and [3] is safe we basically have to reason about
> all the callers of this API anyway, which is what my larger series does.

After thinking on the various solutions, the original snippet I posted
to just re-initialize the struct in each run seems like the best fit[1].
It's true that this "args/argv" thing is the source of the actual
segfault, so any cleanup changes there would address that. But the root
of the confusion in setup_pager() is that it inits the child only once,
but then uses it multiple times. There could be similar confusion over
other fields in the struct (though I don't think there is currently). So
this seems like the most direct fix, and applies regardless of any
args/argv cleanup.

It would become moot if we start to die() on pager setup, which I'd be
in favor of. But I think we should fix the segfault bug first, which
allows us to worry about the larger behavior change separately.

-Peff

[1] https://lore.kernel.org/git/YZhVA8DOjHu90gzs@coredump.intra.peff.net/