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

From: Jeff King <peff@peff.net>
Date: Nov 22, 2021, 17:41 UTC
Message-ID: <YZvWO6MaZwTA86gc@coredump.intra.peff.net>
In-Reply-To: <211122.861r38yuun.gmgdl@evledraar.gmail.com>

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

Show 7 quoted lines

> 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