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

From: Jeff King <peff@peff.net>
Date: Nov 24, 2021, 15:51 UTC
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/