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

Enzo Matsumiya [off-list ref] writes:

I'm preparing v3 with the above suggestions in mind.

Thanks for an update, and thanks for working on this one.

On 11/22, Junio C Hamano wrote:

Enzo Matsumiya [off-list ref] writes:

quoted

I'm preparing v3 with the above suggestions in mind.

Thanks for an update, and thanks for working on this one.

Btw I'm on hold until Ævar's patchset is sorted out, which seems to kind
of overlap/invalidate my fix.

Sorry I couldn't follow much of yesterday's discussion.


Cheers,

Enzo

On Tue, Nov 23 2021, Enzo Matsumiya wrote:

On 11/22, Junio C Hamano wrote:

quoted

Enzo Matsumiya [off-list ref] writes:

quoted

I'm preparing v3 with the above suggestions in mind.

Thanks for an update, and thanks for working on this one.

Btw I'm on hold until Ævar's patchset is sorted out, which seems to kind
of overlap/invalidate my fix.

Sorry I couldn't follow much of yesterday's discussion.

I think per https://lore.kernel.org/git/xmqq7dd0giwp.fsf@gitster.g/ that
Junio's in turn waiting on you, and in my v2 re-roll of my topic[1] I
ejected the test derived from your report, on the assumption that an
earlier fix from you would land first.

I.e. I understood that Junio wanted to queue up your more narrow fix
which would fix the segfault, and my larger topic to remove "argv" and
"env" might come some time later.

I don't mind either way as long as the root cause of "argv" and "env"
gets fixed eventually.

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.

But I honestly didn't look too deeply into your approach & what could be
done to safely work around "argv" and/or "env" on the current "master",
since I had the alternate patches to remove them entirely :)

1. https://lore.kernel.org/git/cover-v2-0.9-00000000000-20211123T115551Z-avarab@gmail.com/
2. https://lore.kernel.org/git/20211122153119.h2t2ti3lkiycd7pb@cyberdelia/
3. https://lore.kernel.org/git/YZvFkwivicJ%2ftFAo@coredump.intra.peff.net/

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/