Re: [PATCH] rebase-i-exec: Allow space in SHELL_PATH

From: Jeff King <peff@peff.net>
Date: Nov 13, 2015, 22:27 UTC
Message-ID: <20151113222748.GA14830@sigill.intra.peff.net>
In-Reply-To: <CABA5-zk+RVBxfmuLyK8CcCFUpMXEzbHRKeWWV2SKsJqjnG-nfA@mail.gmail.com>

On Fri, Nov 13, 2015 at 04:25:18PM +0100, Fredrik Medley wrote:

Show 21 quoted lines

> >> -             ${SHELL:-@SHELL_PATH@} -c "$rest" # Actual execution
> >> +             "${SHELL:-@SHELL_PATH@}" -c "$rest" # Actual execution
> >
> > I think this is the right thing to do (at least I could not think of a
> > case that would be harmed by it, and it certainly fixes your case). It
> > looks like filter-branch would need a similar fix?
> >
> > I think this still isn't resilient to weird meta-characters in the
> > @SHELL_PATH@, but as this is a build-time option, I think it's OK to let
> > people who do
> >
> >   make SHELL_PATH='}"; rm -rf /'
> >
> > hang themselves.
> >
> > -Peff
> 
> Okay, that's what @SHELL_PATH@ stands for. I just read the result
> in the Windows installation that is something like ${SHELL:-/bin/sh}.
> The shell script processor then replaces /bin/sh with
> C:\Program Files\...\bin\sh.

Hmm. Now I'm a bit puzzled. It sounds like the installed file does have @SHELL_PATH@ set to /bin/sh, which is normal. And presumably the setting containing space is coming from the $SHELL environment variable.

My original "I could not think of a case harmed by it" was because @SHELL_PATH@ also gets used on the shebang line at the beginning of the script. And that does not really handle command names with arguments well (you get one argument, and it better not have spaces in it). Of course, it _also_ does not handle command names with spaces in them, either (and there's no room for quoting there).

So anything exotic pretty much has to be coming in from $SHELL, and my mention of filter-branch is not interesting; it just uses @SHELL_PATH@.

So what are people putting in $SHELL? Obviously a shell path with a space in it wants the quotes. Do people do more exotic things like:

  SHELL="my-shell --options"

(I think a plausible option might be "--posix" for some shells). That would break with your patch.

I dunno. We usually try to err on the side of the status quo, so as to avoid breaking existing users. But I find the idea of $SHELL with options reasonably unlikely, so I think your patch is the right direction. I wish I understood better who was setting $SHELL and why, though.

-Peff