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

From: Jeff King <peff@peff.net>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: Enzo Matsumiya <ematsumiya@suse.de>,
	Junio C Hamano <gitster@pobox.com>,
	git@vger.kernel.org
Subject: Re: [PATCH v2] pager: fix crash when pager program doesn't exist
Date: Mon, 22 Nov 2021 12:41:15 -0500	[thread overview]
Message-ID: <YZvWO6MaZwTA86gc@coredump.intra.peff.net> (raw)
In-Reply-To: <211122.861r38yuun.gmgdl@evledraar.gmail.com>

On Mon, Nov 22, 2021 at 06:10:47PM +0100, Ævar Arnfjörð Bjarmason wrote:

> I think a better approach is to just die early and not fallback if the
> user's pager setting is broken. I.e. let's not second-guess their
> explicit configuration, trust it, and if it doesn't work die() or
> error().
> 
> We do fallback on emitting to stdout, so perhaps there's a reason to do
> more exhaustive fallbacks here, I'm not really sure about the above.

I suspect this fallback-to-stdout does not kick in very much in
practice. It only kicks in when start_command() fails, which means
either:

  - some OS-level error with fork, pipe(), etc

  - execve() failed to find the program (we detect this even over fork()
    with a magic protocol between the parent and child)

The second case is the more interesting one for a fallback, but it only
works if we aren't running the pager through a shell. And we do set
use_shell, so it kicks in at all only because of our "skip the shell"
optimization when the command looks simple.

So this falls back:

  $ GIT_PAGER=does-not-exist git log -1 --pretty=format:foo
  error: cannot run does-not-exist: No such file or directory
  foo

but this does not:

  $ GIT_PAGER='does-not-exist --arg' git log -1 --pretty=format:foo
  does-not-exist --arg: 1: does-not-exist: not found

In the non-fallback case we just send all output (including stderr!) to
a dead pipe (and probably die with SIGPIPE, though it's racy).

So we may be better to just die() immediately when pager setup fails.
That naturally fixes this bug, too. ;)

Of course if we are going to do more exhaustive fallback, then it is
going in the opposite direction. But I tend to agree that we should
stick to what the user told us to do, and fail if it's not possible.

-Peff