[PATCH 0/2] Make run-command.c honour SHELL_PATH

[PATCH 0/2] Make run-command.c honour SHELL_PATH

[Ben Walton]

Hi Jeff and Junio,

[Others touched this file too, but it appears Jeff wrote the affected
functionality.]

I hit a glitch with t7006-pager while testing the 1.7.10 rc1/rc2
builds for OpenCSW/Solaris that turned out to be a problem with the
way run-command.c:prepare_shell_cmd was setting up external
utilities.  It was hard coded to fork 'sh -c' instead of honouring the
SHELL_PATH as set at build time.

In this case, the failing test was t7006-pager:command-specific
pager.  That test (and some subsequent ones) were setting the pager
command used by git log to "sed s/^/foo:/ >actual" which is fine in a
POSIX-compliant sh, but not in Solaris' sh.  If the user PATH at
runtime happened to allow the broken system sh used instead of a sane
sh, the ^ is interpreted the same as[1] | and this caused sed to fail
with incomplete s/ command and a "command not found: /foo:" from the
other forked process.

To mitigate this, the following patches introduce the macro SHELL_PATH
for use in run-command.c, defaulting to "sh" to preserve the current
behaviour and then cause the build system to provide the SHELL_PATH as
set by the builder.  This means that all processed forked by
run-command will use the same interpreter as the shell scripts in the
git suite.

I considered implementing a dynamically generated .h file for this,
similar to common-cmds.h, but thought that was overkill at the current
time.  If you think that (or something else) is a better fit for the
change, let me know and I'll make the required adjustments.

Thanks
-Ben

[1] http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/cmd/sh/cmd.c#184


Ben Walton (2):
  run-command.c: Define SHELL_PATH macro for use in prepare_shell_cmd
  Makefile: Set EXTRA_CPPFLAGS during the compilation of run-command

 Makefile      |    2 ++
 run-command.c |    6 +++++-
 2 files changed, 7 insertions(+), 1 deletions(-)

-- 
1.7.5.4

[PATCH 1/2] run-command.c: Define SHELL_PATH macro for use in prepare_shell_cmd

[Ben Walton]

The shell spawned in run-command.c:prepare_shell_cmd was hard coded to
'sh'.  Instead, make this a macro named SHELL_PATH so that it can be
overridden by the build system.  Use 'sh' as the default to preserve
original behaviour and ensure that a value is always set.

This avoids a situation where some commands were spawned using a
different shell than the one configured at build time.  Previously, it
was possible for things to be executed by a non-POSIX shell depending
on the user's PATH.

Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
---
 run-command.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/run-command.c b/run-command.c
index 1db8abf..f005a31 100644
--- a/run-command.c
+++ b/run-command.c
@@ -4,6 +4,10 @@
 #include "sigchain.h"
 #include "argv-array.h"
 
+#ifndef SHELL_PATH
+# define SHELL_PATH "sh"
+#endif
+
 struct child_to_clean {
 	pid_t pid;
 	struct child_to_clean *next;
@@ -90,7 +94,7 @@ static const char **prepare_shell_cmd(const char **argv)
 		die("BUG: shell command is empty");
 
 	if (strcspn(argv[0], "|&;<>()$`\\\"' \t\n*?[#~=%") != strlen(argv[0])) {
-		nargv[nargc++] = "sh";
+		nargv[nargc++] = SHELL_PATH;
 		nargv[nargc++] = "-c";
 
 		if (argc < 2)
-- 
1.7.5.4

[PATCH 2/2] Makefile: Set EXTRA_CPPFLAGS during the compilation of run-command

[Ben Walton]

When building run-command.o, we want to pass in a define for
SHELL_PATH to ensure that the shell configured at build time is used
at run time when spawning external commands.

Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
---
 Makefile |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/Makefile b/Makefile
index be1957a..344e2e0 100644
--- a/Makefile
+++ b/Makefile
@@ -1913,6 +1913,8 @@ builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
 	'-DGIT_MAN_PATH="$(mandir_SQ)"' \
 	'-DGIT_INFO_PATH="$(infodir_SQ)"'
 
+run-command.o: EXTRA_CPPFLAGS = -DSHELL_PATH='"$(SHELL_PATH)"'
+
 $(BUILT_INS): git$X
 	$(QUIET_BUILT_IN)$(RM) $@ && \
 	ln git$X $@ 2>/dev/null || \
-- 
1.7.5.4

Re: [PATCH 0/2] Make run-command.c honour SHELL_PATH

[Jonathan Nieder]

Hi Ben,

Good catch.  A few comments:

Ben Walton wrote:

> In this case, the failing test was t7006-pager:command-specific
> pager.  That test (and some subsequent ones) were setting the pager
> command used by git log to "sed s/^/foo:/ >actual" which is fine in a
> POSIX-compliant sh, but not in Solaris' sh.  If the user PATH at
> runtime happened to allow the broken system sh used instead of a sane
> sh, the ^ is interpreted the same as[1] | and this caused sed to fail
> with incomplete s/ command and a "command not found: /foo:" from the
> other forked process.

When I first read the corresponding patch without reading this cover
letter I was mystified.  Until I saw the above paragraph, I did not
even see what problem was being solved.  The above paragraph should
probably be part of the commit message.

Ok, on to the proposed solution. ;-)

My first reaction was to suspect the series is solving the problem in
the wrong place.  The core of the bug might be t7006 itself, which
assumes that the shell used to interpret the GIT_PAGER setting is a
POSIX-style shell rather than an ancient Bourne shell or cmd.exe.
In the far long term, we should probably skip this test on some
platforms using an appropriate test prerequisite.

To put it another way, the RUN_USING_SHELL magic is just supposed to
be a more featureful way to do what system() normally does.  What
shell does system() use on Solaris?

A second reaction was to wonder why the usual fixup from
v1.6.4-rc0~66^2~1 (Makefile: insert SANE_TOOL_PATH to PATH before /bin
or /usr/bin, 2009-07-08) didn't apply.  Should the git wrapper prepend
the same magic to $PATH that git-sh-setup.sh does to make the behavior
of scripted and unscripted commands a little more consistent?

A third reaction was that git_pager in the sh-setup library uses the
eval builtin, so we are already assuming that GIT_PAGER is appropriate
input for a POSIX-style shell.  So maybe the approach you've adopted
is appropriate after all, at least in the short term while we require
a POSIX-style shell elsewhere in git.

A few added words in the commit message could save the next reader
from going through so long a thought process before seeing why what
the patch does is the right thing to do.

A more minor comment: patch 1/2 was even more mysterious.  Combining
the two patches would be enough to avoid confusion there.  I haven't
checked the makefile changes and interaction with GIT-CFLAGS carefully
yet and hope to do so in the next round.

Thanks for working on this.

Sincerely,
Jonathan

Re: [PATCH 0/2] Make run-command.c honour SHELL_PATH

[Ben Walton]

Excerpts from Jonathan Nieder's message of Sun Mar 25 21:11:48 -0400 2012:

Hi Jonathan,

> > In this case, the failing test was t7006-pager:command-specific
> > pager.  That test (and some subsequent ones) were setting the pager
> > command used by git log to "sed s/^/foo:/ >actual" which is fine in a
> > POSIX-compliant sh, but not in Solaris' sh.  If the user PATH at
> > runtime happened to allow the broken system sh used instead of a sane
> > sh, the ^ is interpreted the same as[1] | and this caused sed to fail
> > with incomplete s/ command and a "command not found: /foo:" from the
> > other forked process.
> 
> When I first read the corresponding patch without reading this cover
> letter I was mystified.  Until I saw the above paragraph, I did not
> even see what problem was being solved.  The above paragraph should
> probably be part of the commit message.

Ok, that's fair.  I thought the commit message was self-contained
enough to justify the change, but point taken. :)  I'll redo the
commit log and squash the two patches as per your later points.

> My first reaction was to suspect the series is solving the problem
> in the wrong place.  The core of the bug might be t7006 itself,
> which assumes that the shell used to interpret the GIT_PAGER setting
> is a POSIX-style shell rather than an ancient Bourne shell or
> cmd.exe.  In the far long term, we should probably skip this test on
> some platforms using an appropriate test prerequisite.

I started looking through t7006 first as well, thinking that some
non-POSIX syntax had slipped in... This raises a good point though.
I'd come at this thinking that things _should_ be able to rely on a
POSIX shell (or a modern standard, anyway) for things like this.  Is
there any sort of defined expectation for this?

>From my point of view, I'd expect the command to be spawned under
something sane on the current platform.  I haven't looked for this
specifically, but this code path would still try to fork 'sh' on
Windows as far as my understanding of it goes.  I don't use git there,
so I don't know if part of the installation sets up a usable sh
too.

> To put it another way, the RUN_USING_SHELL magic is just supposed to
> be a more featureful way to do what system() normally does.  What
> shell does system() use on Solaris?

This depends on some macros.  The default is /usr/bin/sh, but if
various XOPEN-type macros are set, it will use /usr/xpg4/bin/sh.

> A second reaction was to wonder why the usual fixup from
> v1.6.4-rc0~66^2~1 (Makefile: insert SANE_TOOL_PATH to PATH before
> /bin or /usr/bin, 2009-07-08) didn't apply.  Should the git wrapper
> prepend the same magic to $PATH that git-sh-setup.sh does to make
> the behavior of scripted and unscripted commands a little more
> consistent?

I did some poking at this before creating my patch as that is what I'd
expected too.  It would likely be a good idea, but in my case, even
that wouldn't help.

I add /opt/csw/gnu:/opt/csw/bin:/usr/xpg4/bin to the SANE_TOOL_PATH
but set the SHELL_PATH to /opt/csw/bin/bash.  Without my patch, but
with SANE_TOOL_PATH honoured, I'd still see /opt/csw/bin/sh forked and
that sh is crippled too.

So that leads me to think that if we're going to fork a shell, it
should be one that we know to be good...if the builder has provided
that value.  I think you agree with this based on your next comment.

> A third reaction was that git_pager in the sh-setup library uses the
> eval builtin, so we are already assuming that GIT_PAGER is
> appropriate input for a POSIX-style shell.  So maybe the approach
> you've adopted is appropriate after all, at least in the short term
> while we require a POSIX-style shell elsewhere in git.

I'm unclear what you're meaning by this.  Are you implying that the
requirement for a POSIX-style shell should be relaxes to the point
where things don't rely on that base set of functionality?

> A few added words in the commit message could save the next reader
> from going through so long a thought process before seeing why what
> the patch does is the right thing to do.

Ok, I can re-do the commit message to incorporate notes about the
broken text and some of your rationale above.

> A more minor comment: patch 1/2 was even more mysterious.  Combining
> the two patches would be enough to avoid confusion there.  I haven't
> checked the makefile changes and interaction with GIT-CFLAGS
> carefully yet and hope to do so in the next round.

To me they were two logically separate changes, so I split them.  I
don't mind squashing it together though.

I'll resubmit later today.

Thanks
-Ben
--
Ben Walton
Systems Programmer - CHASS
University of Toronto
C:416.407.5610 | W:416.978.4302

Re: [PATCH 0/2] Make run-command.c honour SHELL_PATH

[Jeff King]

On Sun, Mar 25, 2012 at 08:31:34AM -0400, Ben Walton wrote:

> [Others touched this file too, but it appears Jeff wrote the affected
> functionality.]

Sort of. I was just refactoring what was happening elsewhere in the
code, where individual callsites were sitting "sh -c" manually into the
argv list. So I didn't actually give any thought to whether SHELL_PATH
should be used. :)

> I hit a glitch with t7006-pager while testing the 1.7.10 rc1/rc2
> builds for OpenCSW/Solaris that turned out to be a problem with the
> way run-command.c:prepare_shell_cmd was setting up external
> utilities.  It was hard coded to fork 'sh -c' instead of honouring the
> SHELL_PATH as set at build time.
> 
> In this case, the failing test was t7006-pager:command-specific
> pager.  That test (and some subsequent ones) were setting the pager
> command used by git log to "sed s/^/foo:/ >actual" which is fine in a
> POSIX-compliant sh, but not in Solaris' sh.  If the user PATH at
> runtime happened to allow the broken system sh used instead of a sane
> sh, the ^ is interpreted the same as[1] | and this caused sed to fail
> with incomplete s/ command and a "command not found: /foo:" from the
> other forked process.

The original intent of SHELL_PATH is to give the full path, so it could
be used in places where PATH lookup is not an option (i.e., on
#!-lines). Whereas the run-command use_shell option looks up "sh" in the
PATH, so the "right" thing is to have your PATH set up to put a sane
"sh" near the front.

So in that sense, your patch is unnecessary. On the other hand, it is
very unlikely to harm anyone[1], and it reduces the number of things the
user has to make sure are set up properly, so I think overall it's an
improvement, even if there is already another way to fix the problem.

-Peff

[1] I can't imagine anybody will be upset that we would use SHELL_PATH
    instead of "sh". It could produce a difference of behavior if you
    had a sane "sh" in your PATH, but SHELL_PATH points to the absolute
    path of some other shell. But if your SHELL_PATH and the "sh" in
    your PATH are so wildly divergent that you notice the difference, I
    think you may have bigger problems.

Re: [PATCH 0/2] Make run-command.c honour SHELL_PATH

[Jeff King]

On Sun, Mar 25, 2012 at 08:11:48PM -0500, Jonathan Nieder wrote:

> My first reaction was to suspect the series is solving the problem in
> the wrong place.  The core of the bug might be t7006 itself, which
> assumes that the shell used to interpret the GIT_PAGER setting is a
> POSIX-style shell rather than an ancient Bourne shell or cmd.exe.
> In the far long term, we should probably skip this test on some
> platforms using an appropriate test prerequisite.

I don't think that's an unreasonable assumption. Solaris /bin/sh is
really a total piece of junk, and we have already gone to lengths to let
users avoid it. We have SHELL_PATH, but we also have SANE_TOOL_PATH.
This is a case that should have been caught by SANE_TOOL_PATH, but
wasn't. So I think the problem is worth fixing, and just lets Solaris
people enjoy the same reasonable assumption about having a working shell
that other systems do.

> A second reaction was to wonder why the usual fixup from
> v1.6.4-rc0~66^2~1 (Makefile: insert SANE_TOOL_PATH to PATH before /bin
> or /usr/bin, 2009-07-08) didn't apply.  Should the git wrapper prepend
> the same magic to $PATH that git-sh-setup.sh does to make the behavior
> of scripted and unscripted commands a little more consistent?

I think that would be an OK solution, too. Though I wonder if using
SHELL_PATH isn't simply easier for the user to get right (I seem to
recall people finding SANE_TOOL_PATH confusing to set up in the past,
but I have not personally used it myself).

> A more minor comment: patch 1/2 was even more mysterious.  Combining
> the two patches would be enough to avoid confusion there.  I haven't
> checked the makefile changes and interaction with GIT-CFLAGS carefully
> yet and hope to do so in the next round.

I think they would be easier to understand squashed, too. If there were
many users of the new macro to be converted individually, I would say it
might make sense to introduce it in one commit, then convert each class
of callsites separately. But here there is really only one user, so
seeing the application can help understand the rationale for the
definition.

-Peff

Re: [PATCH 0/2] Make run-command.c honour SHELL_PATH

[Jeff King]

On Mon, Mar 26, 2012 at 09:38:45AM -0400, Ben Walton wrote:

> > A second reaction was to wonder why the usual fixup from
> > v1.6.4-rc0~66^2~1 (Makefile: insert SANE_TOOL_PATH to PATH before
> > /bin or /usr/bin, 2009-07-08) didn't apply.  Should the git wrapper
> > prepend the same magic to $PATH that git-sh-setup.sh does to make
> > the behavior of scripted and unscripted commands a little more
> > consistent?
> 
> I did some poking at this before creating my patch as that is what I'd
> expected too.  It would likely be a good idea, but in my case, even
> that wouldn't help.
> 
> I add /opt/csw/gnu:/opt/csw/bin:/usr/xpg4/bin to the SANE_TOOL_PATH
> but set the SHELL_PATH to /opt/csw/bin/bash.  Without my patch, but
> with SANE_TOOL_PATH honoured, I'd still see /opt/csw/bin/sh forked and
> that sh is crippled too.
> 
> So that leads me to think that if we're going to fork a shell, it
> should be one that we know to be good...if the builder has provided
> that value.  I think you agree with this based on your next comment.

What is /opt/csw/bin/sh? Is it a symlink to bash (which would mean bash
running in POSIX mode)? Or is it a lightweight shell like ash? Or
something else?

The point of SHELL_PATH is to provide a POSIX shell.  Generally, bash
behavior is a superset of POSIX, so you will not run into any
incompatibilities by running things with bash. But do be aware that you
are slightly incompatible with the rest of the world (so things that
work for you, for example, might not work for people using git with the
stock "shell is /bin/sh" configuration).

-Peff

Re: [PATCH 0/2] Make run-command.c honour SHELL_PATH

[Jonathan Nieder]

Ben Walton wrote:
> Excerpts from Jonathan Nieder's message of Sun Mar 25 21:11:48 -0400 2012:

[...]
>>                                          at least in the short term
>> while we require a POSIX-style shell elsewhere in git.
>
> I'm unclear what you're meaning by this.  Are you implying that the
> requirement for a POSIX-style shell should be relaxes to the point
> where things don't rely on that base set of functionality?

Currently git uses bash on Windows by necessity.  I suspect we could
make a lot of people happy by dropping that dependency some day and
using cmd.exe or something like powershell in places the user provides
a script.

Context: [1]

Of course that's far in the future and not certain at all. ;-)

Ciao,
Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/164656/focus=164716

Re: [PATCH 0/2] Make run-command.c honour SHELL_PATH

[Ben Walton]

Excerpts from Jeff King's message of Mon Mar 26 14:12:38 -0400 2012:

> What is /opt/csw/bin/sh? Is it a symlink to bash (which would mean
> bash running in POSIX mode)? Or is it a lightweight shell like ash?
> Or something else?

This is some version of the traditional solaris /bin/sh packaged as
part of schilyutils (Joerg Schilling's utilities).  I'm really not
sure why he packages it or what the differences from /bin/sh
are...Asking him is on my todo list though.  I only just learned it
existed when I hit this glitch.

> The point of SHELL_PATH is to provide a POSIX shell.  Generally,
> bash behavior is a superset of POSIX, so you will not run into any
> incompatibilities by running things with bash. But do be aware that
> you are slightly incompatible with the rest of the world (so things
> that work for you, for example, might not work for people using git
> with the stock "shell is /bin/sh" configuration).

This was a conscious choice I made early in the packaging of git for
OpenCSW.  The idea was that we had a shell that was also under our
packaging control and 'sane' at the same time.  I realize it's a
superset of the POSIX functionality.  It's something I should maybe
change, but I'd need to stage it slowly so as to avoid breakages.

Thanks
-Ben
--
Ben Walton
Systems Programmer - CHASS
University of Toronto
C:416.407.5610 | W:416.978.4302

Re: [PATCH 0/2] Make run-command.c honour SHELL_PATH

[Jeff King]

On Mon, Mar 26, 2012 at 02:19:54PM -0400, Ben Walton wrote:

> Excerpts from Jeff King's message of Mon Mar 26 14:12:38 -0400 2012:
> 
> > What is /opt/csw/bin/sh? Is it a symlink to bash (which would mean
> > bash running in POSIX mode)? Or is it a lightweight shell like ash?
> > Or something else?
> 
> This is some version of the traditional solaris /bin/sh packaged as
> part of schilyutils (Joerg Schilling's utilities).  I'm really not
> sure why he packages it or what the differences from /bin/sh
> are...Asking him is on my todo list though.  I only just learned it
> existed when I hit this glitch.

OK, that's gross. I would argue that your SANE_TOOL_PATH is not great,
then, because the "sh" is not sane. But that being said, this is exactly
the sort of thing I was talking about in my first message, which is that
SANE_TOOL_PATH is hard to get right. If we can make things Just Work by
using SHELL_PATH, I don't see that as a bad thing.

> > The point of SHELL_PATH is to provide a POSIX shell.  Generally,
> > bash behavior is a superset of POSIX, so you will not run into any
> > incompatibilities by running things with bash. But do be aware that
> > you are slightly incompatible with the rest of the world (so things
> > that work for you, for example, might not work for people using git
> > with the stock "shell is /bin/sh" configuration).
> 
> This was a conscious choice I made early in the packaging of git for
> OpenCSW.  The idea was that we had a shell that was also under our
> packaging control and 'sane' at the same time.  I realize it's a
> superset of the POSIX functionality.  It's something I should maybe
> change, but I'd need to stage it slowly so as to avoid breakages.

It sounds like using bash is probably the least bad thing to do, and
doing anything else would not be worth the complexity. You could use
"bash -posix" (or an sh-symlink to bash), but in practice I doubt it is
really hurting anybody.

-Peff

[PATCH] Use SHELL_PATH to fork commands in run_command.c:prepare_shell_cmd

[Ben Walton]

The shell spawned in run-command.c:prepare_shell_cmd was hard coded to
"sh."  This was breaking "t7006-pager:command-specific pager" when the
"sh" found in the PATH happened to be a broken one such as Solaris'
/usr/bin/sh.  (The breakage in this case was due to ^ being
interpreted the same as | which was seeing two processes forked
instead of a single sed process.)

To allow the build system to supply a value that is deemed sane, thus
removing variation injected by the local user environment, define a
macro named SHELL_PATH (to match the variable of the same name in the
build system).  Use this macro instead of the literal "sh" when
preparing a set of arguments to be forked.  Set "sh" as the default
value of this macro if none is set through other means to preserve the
original behaviour.

The Makefile now sets EXTRA_CPPFLAGS to a value that includes a
definition of SHELL_PATH when building run_command.c.  This sees the
sane shell passed to run_command.c and used when forking external
commands.

Leveraging SANE_TOOL_PATH was considered here but had downsides that
may have made it an incomplete solution.  For example, some builds may
use bash as the shell and not want sh used at all.

Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
---
 Makefile      |    2 ++
 run-command.c |    6 +++++-
 2 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index be1957a..344e2e0 100644
--- a/Makefile
+++ b/Makefile
@@ -1913,6 +1913,8 @@ builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
 	'-DGIT_MAN_PATH="$(mandir_SQ)"' \
 	'-DGIT_INFO_PATH="$(infodir_SQ)"'
 
+run-command.o: EXTRA_CPPFLAGS = -DSHELL_PATH='"$(SHELL_PATH)"'
+
 $(BUILT_INS): git$X
 	$(QUIET_BUILT_IN)$(RM) $@ && \
 	ln git$X $@ 2>/dev/null || \
diff --git a/run-command.c b/run-command.c
index 1db8abf..f005a31 100644
--- a/run-command.c
+++ b/run-command.c
@@ -4,6 +4,10 @@
 #include "sigchain.h"
 #include "argv-array.h"
 
+#ifndef SHELL_PATH
+# define SHELL_PATH "sh"
+#endif
+
 struct child_to_clean {
 	pid_t pid;
 	struct child_to_clean *next;
@@ -90,7 +94,7 @@ static const char **prepare_shell_cmd(const char **argv)
 		die("BUG: shell command is empty");
 
 	if (strcspn(argv[0], "|&;<>()$`\\\"' \t\n*?[#~=%") != strlen(argv[0])) {
-		nargv[nargc++] = "sh";
+		nargv[nargc++] = SHELL_PATH;
 		nargv[nargc++] = "-c";
 
 		if (argc < 2)
-- 
1.7.5.4

Re: [PATCH 0/2] Make run-command.c honour SHELL_PATH

[Ben Walton]

Excerpts from Jeff King's message of Mon Mar 26 14:24:27 -0400 2012:

Hi Jeff,

> OK, that's gross. I would argue that your SANE_TOOL_PATH is not
> great, then, because the "sh" is not sane. But that being said, this
> is exactly the sort of thing I was talking about in my first
> message, which is that SANE_TOOL_PATH is hard to get right. If we
> can make things Just Work by using SHELL_PATH, I don't see that as a
> bad thing.

Ok, thanks for the great feedback.  I just sent an updated version of
the patch with the squash done and what I hope is a better commit
message.

> It sounds like using bash is probably the least bad thing to do, and
> doing anything else would not be worth the complexity. You could use
> "bash -posix" (or an sh-symlink to bash), but in practice I doubt it
> is really hurting anybody.

I'd need to jump through some hoops to do an sh-symlink that was
within our packaging standards, although it's not impossible.  Using
"bash -posix" is likely a better path for me to travel.  I'll still
need to plan out the change with lots of notice for users in case
there have been bashism's used in local scripts, but as you say it's
not likely hurting anyone in practice.

Thanks
-Ben
--
Ben Walton
Systems Programmer - CHASS
University of Toronto
C:416.407.5610 | W:416.978.4302

Re: [PATCH] Use SHELL_PATH to fork commands in run_command.c:prepare_shell_cmd

[Jeff King]

On Mon, Mar 26, 2012 at 10:41:18PM -0400, Ben Walton wrote:

> diff --git a/Makefile b/Makefile
> index be1957a..344e2e0 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1913,6 +1913,8 @@ builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
>  	'-DGIT_MAN_PATH="$(mandir_SQ)"' \
>  	'-DGIT_INFO_PATH="$(infodir_SQ)"'
>  
> +run-command.o: EXTRA_CPPFLAGS = -DSHELL_PATH='"$(SHELL_PATH)"'
> +

This should be $(SHELL_PATH_SQ), no?

> diff --git a/run-command.c b/run-command.c
> index 1db8abf..f005a31 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -4,6 +4,10 @@
>  #include "sigchain.h"
>  #include "argv-array.h"
>  
> +#ifndef SHELL_PATH
> +# define SHELL_PATH "sh"
> +#endif

Does this default ever kick in? The Makefile defaults SHELL_PATH to
/bin/sh, so we will always end up with at least that.

Doing so at least makes us consistent across builds, but I wonder if we
should leave it as "sh" on systems that do not set SHELL_PATH manually.
Executing "sh" via the PATH is the normal system() thing to do. I doubt
many people have an "sh" in their PATH ahead of /bin/sh, but if they do,
we are changing the behavior for them for no good reason.

The whole SHELL_PATH and SANE_TOOL_PATH mess is about helping people on
less-abled systems, and I do not mind bending the usual conventions to
make things more convenient on those systems (e.g., by not doing the
PATH lookup of the shell name). But it would be nice if that bending did
not affect people on more mainstream systems.

-Peff

Re: [PATCH] Use SHELL_PATH to fork commands in run_command.c:prepare_shell_cmd

[Jeff King]

On Mon, Mar 26, 2012 at 11:29:17PM -0400, Jeff King wrote:

> > +#ifndef SHELL_PATH
> > +# define SHELL_PATH "sh"
> > +#endif
> 
> Does this default ever kick in? The Makefile defaults SHELL_PATH to
> /bin/sh, so we will always end up with at least that.
> 
> Doing so at least makes us consistent across builds, but I wonder if we
> should leave it as "sh" on systems that do not set SHELL_PATH manually.
> Executing "sh" via the PATH is the normal system() thing to do. I doubt
> many people have an "sh" in their PATH ahead of /bin/sh, but if they do,
> we are changing the behavior for them for no good reason.
> 
> The whole SHELL_PATH and SANE_TOOL_PATH mess is about helping people on
> less-abled systems, and I do not mind bending the usual conventions to
> make things more convenient on those systems (e.g., by not doing the
> PATH lookup of the shell name). But it would be nice if that bending did
> not affect people on more mainstream systems.

I think you could do that pretty easily with this in the Makefile:

diff --git a/Makefile b/Makefile
index be1957a..fcd6896 100644
--- a/Makefile
+++ b/Makefile
@@ -528,6 +528,9 @@ BINDIR_PROGRAMS_NO_X += git-cvsserver
 # Set paths to tools early so that they can be used for version tests.
 ifndef SHELL_PATH
 	SHELL_PATH = /bin/sh
+	SHELL_NAME = sh
+else
+	SHELL_NAME = $(SHELL_PATH)
 endif
 ifndef PERL_PATH
 	PERL_PATH = /usr/bin/perl

and then just pass and use $SHELL_NAME in run-command.c (I guess you'd
have to make a $SHELL_NAME_SQ, too).

One other option not discussed: what if you always just used the
basename of $SHELL_PATH? That would fix your problem (it would do "bash
-c" instead of "sh -c"), while keeping the same PATH-lookup semantics
that exist now. The only setup it wouldn't help is somebody who uses
/some/other/sh, then still has /bin in their PATH before /some/other.

-Peff

Re: [PATCH] Use SHELL_PATH to fork commands in run_command.c:prepare_shell_cmd

[Jonathan Nieder]

Hi again,

Ben Walton wrote:

> The shell spawned in run-command.c:prepare_shell_cmd was hard coded to
> "sh."  This was breaking "t7006-pager:command-specific pager" when the
> "sh" found in the PATH happened to be a broken one such as Solaris'
> /usr/bin/sh.  (The breakage in this case was due to ^ being
> interpreted the same as | which was seeing two processes forked
> instead of a single sed process.)

Better.  But the above is still focusing on what you changed rather
than the intended impact and potential fallout.  Remember, the commit
log is where most of git's design documents are stored.

Isn't it something more like this?

	Ever since <date or version>, the "command-specific pager" test
	from t7006-pager has not been passing on Solaris with the
	default value of PATH.

	The cause: that test (and some subsequent ones) sets the pager
	command used by git log to "sed s/^/foo:/ >actual" which is fine
	in a POSIX-compliant sh, but not in Solaris' sh.  The
	run_command machinery uses plain "sh" as a shell and if the user
	PATH at runtime happens to put the ancient system sh before any
	modern POSIX-style sh, the ^ is interpreted the same as | and
	chaos results.

	Or to put it another way, t7006 expects that the shell used to
	interpret GIT_PAGER is a POSIX-style shell and run_command's
	shell feature fails to deliver.

	To fix this:

	a. instead of launching a shell on its own, run_command could
	   call system() or popen().  This launches a POSIX shell on
	   Solaris as long as _POSIX_SOURCE is defined.

	b. the git wrapper could prepend SANE_TOOL_PATH to $PATH for
	   builtin commands for consistency with scripted commands that
	   do that using git-sh-setup.  If we're lucky, the first "sh"
	   command found in the new PATH would be a suitable shell,
	   though git's current scripts do not rely on that.

	c. the run_command machinery could use the same SHELL_PATH
	   shell that is used by the git filter-branch script and in
	   all script's #! lines.

	However:

	- (a) means losing the ability to open a bidirectional pipe to a
	  filter script.  It would also break git for Windows, where
	  system() uses cmd.exe instead of a POSIX-style shell (see
	  v1.7.5-rc0~144^2, "alias: use run_command api to execute
	  aliases, 2011-01-07).

	- On Solaris systems with schilyutils in /opt/csw/bin at the
	  beginning of SANE_TOOL_PATH, (b) finds /opt/csw/bin/sh which
	  is another ancient Bourne-style shell with the same problem.

	So let's go with (c).

	After this patch, $GIT_PAGER is interpreted by the same shell
	in scripted commands which use the git_pager function that uses
	"eval" and builtins which use the run_command machinery to
	launch the interpreter specified as SHELL_PATH when git was built.

Just my two cents,
Jonathan

Re: [PATCH] Use SHELL_PATH to fork commands in run_command.c:prepare_shell_cmd

[Jonathan Nieder]

Jonathan Nieder wrote:

[...]
> 	The cause: that test (and some subsequent ones) sets the pager

	The cause: that test and some subsequent ones set the pager ...

(The text has subject/verb agreement problems if the reader chooses to
follow the parentheses.  Removing parens, s/sets/set to spare the reader
the choice.)

> 	command used by git log to "sed s/^/foo:/ >actual" which is fine
> 	in a POSIX-compliant sh, but not in Solaris' sh.  The

	                   ... to "sed s/^/foo:/ >actual", which is fine
	in a POSIX-compliant sh but not in Solaris's sh.

(missing comma before "which"; extra comma before "but").

[...]
> 	a. instead of launching a shell on its own, run_command could
> 	   call system() or popen().  This launches a POSIX shell on
> 	   Solaris as long as _POSIX_SOURCE is defined.

Git uses _XOPEN_SOURCE so it's clearer to say that instead.

[...]
> 	c. the run_command machinery could use the same SHELL_PATH
> 	   shell that is used by the git filter-branch script and in
> 	   all script's #! lines.

Punctuation: script's should be scripts'.

Sorry for the noise.

Re: [PATCH] Use SHELL_PATH to fork commands in run_command.c:prepare_shell_cmd

[Jonathan Nieder]

Jeff King wrote:

> Does this default ever kick in? The Makefile defaults SHELL_PATH to
> /bin/sh, so we will always end up with at least that.
>
> Doing so at least makes us consistent across builds, but I wonder if we
> should leave it as "sh" on systems that do not set SHELL_PATH manually.
> Executing "sh" via the PATH is the normal system() thing to do.

It's more common for system() to default to /bin/sh.

I noticed the Makefile already doesn't do this sort of thing for
mandir and htmldir, but do you think changes to SHELL_PATH should be
tracked to force a rebuild when it changes?

	GIT-SHELL-PATH: FORCE
		@FLAGS='$(SHELL_PATH_SQ)'; \
		    if test x"$$FLAGS" != x"`cat GIT-SHELL-PATH 2>/dev/null`" ; then \
			echo 1>&2 "    * new shell path"; \
			echo "$$FLAGS" >GIT-SHELL-PATH; \
		    fi

	$(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh GIT-SHELL-PATH
		...

	$(SCRIPT_LIB) : % : %.sh GIT-SHELL-PATH
		...

	run_command.sp run_command.o: GIT-SHELL-PATH
	exec_cmd.sp exec_cmd.s exec_cmd.o: EXTRA_CPPFLAGS = \
		'-DSHELL_PATH="$(SHELL_PATH_SQ)"'

Re: [PATCH] Use SHELL_PATH to fork commands in run_command.c:prepare_shell_cmd

[Jeff King]

On Tue, Mar 27, 2012 at 12:01:10AM -0500, Jonathan Nieder wrote:

> Jeff King wrote:
> 
> > Does this default ever kick in? The Makefile defaults SHELL_PATH to
> > /bin/sh, so we will always end up with at least that.
> >
> > Doing so at least makes us consistent across builds, but I wonder if we
> > should leave it as "sh" on systems that do not set SHELL_PATH manually.
> > Executing "sh" via the PATH is the normal system() thing to do.
> 
> It's more common for system() to default to /bin/sh.

Hmm, you're right. I think I was fooled by the fact that it puts "sh"
into argv[0]. It doesn't seem to actually do a PATH lookup, though (at
least not on my system).

In that case, Ben's patch is actually making us _more_ normal and
like system(), which is probably a slight benefit (although it's still a
behavior change from what we've been doing).

> I noticed the Makefile already doesn't do this sort of thing for
> mandir and htmldir, but do you think changes to SHELL_PATH should be
> tracked to force a rebuild when it changes?

Yeah, that would be nice.

In general, I wish adding these sorts of dependencies wasn't so manual
and painful. I'm not sure of a good solution short of totally retooling
our build system, though.

-Peff

Re: [PATCH] Use SHELL_PATH to fork commands in run_command.c:prepare_shell_cmd

[Jonathan Nieder]

Jeff King wrote:
> On Tue, Mar 27, 2012 at 12:01:10AM -0500, Jonathan Nieder wrote:

>> I noticed the Makefile already doesn't do this sort of thing for
>> mandir and htmldir, but do you think changes to SHELL_PATH should be
>> tracked to force a rebuild when it changes?
>
> Yeah, that would be nice.

Thanks.  On second thought, without the corresponding tracking for
PERL_PATH, USE_GETTEXT_SCHEME, localedir, etc, it doesn't seem worth
it.

> In general, I wish adding these sorts of dependencies wasn't so manual
> and painful. I'm not sure of a good solution short of totally retooling
> our build system, though.

The Linux kernel build system has an interesting solution to this.  It
keeps track of the command line used to build each file and forces a
rebuild when that command changes.

Ciao,
Jonathan

Re: [PATCH] Use SHELL_PATH to fork commands in run_command.c:prepare_shell_cmd

[Johannes Sixt]

Am 3/27/2012 5:29, schrieb Jeff King:
> On Mon, Mar 26, 2012 at 10:41:18PM -0400, Ben Walton wrote:
>> diff --git a/run-command.c b/run-command.c
>> index 1db8abf..f005a31 100644
>> --- a/run-command.c
>> +++ b/run-command.c
>> @@ -4,6 +4,10 @@
>>  #include "sigchain.h"
>>  #include "argv-array.h"
>>  
>> +#ifndef SHELL_PATH
>> +# define SHELL_PATH "sh"
>> +#endif
> 
> Does this default ever kick in? The Makefile defaults SHELL_PATH to
> /bin/sh, so we will always end up with at least that.

People do use this code outside git. Providing a fallback here would help
them.

-- Hannes

Re: [PATCH] Use SHELL_PATH to fork commands in run_command.c:prepare_shell_cmd

[Ben Walton]

Excerpts from Jeff King's message of Mon Mar 26 23:29:17 -0400 2012:

Hi Jeff,

> > +run-command.o: EXTRA_CPPFLAGS = -DSHELL_PATH='"$(SHELL_PATH)"'
> > +
> 
> This should be $(SHELL_PATH_SQ), no?

Yes, you're right, it should be.

> > +#ifndef SHELL_PATH
> > +# define SHELL_PATH "sh"
> > +#endif
> 
> Does this default ever kick in? The Makefile defaults SHELL_PATH to
> /bin/sh, so we will always end up with at least that.

Not when using the build system, but as Hannes mentioned, there is
potential for this to be used outside of the default build system, so
I think having the fallback is a good defensive option.  Should it
maybe be set to /bin/sh though to be more consistent with system()?

> The whole SHELL_PATH and SANE_TOOL_PATH mess is about helping people
> on less-abled systems, and I do not mind bending the usual
> conventions to make things more convenient on those systems (e.g.,
> by not doing the PATH lookup of the shell name). But it would be
> nice if that bending did not affect people on more mainstream
> systems.

Given the rest of the discussion that happened, I think I understand
that my patch is actually ok with the following caveats:

1. My commit message still needs work.
2. Possibly change the default setting of the SHELL_PATH macro from
   "sh" to "/bin/sh"
3. Use the _SQ variant of SHELL_PATH.

(The tracking of changes for SHELL_PATH is considered too heavy for
now when the other _PATH items aren't tracked the same way.  This
might make a nice separate patch series though, using the idea from
the kernel where individual commands are tracked.)

Did I miss anything else?

Thanks
-Ben
--
Ben Walton
Systems Programmer - CHASS
University of Toronto
C:416.407.5610 | W:416.978.4302

Re: [PATCH] Use SHELL_PATH to fork commands in run_command.c:prepare_shell_cmd

[Jeff King]

On Tue, Mar 27, 2012 at 10:46:15PM -0400, Ben Walton wrote:

> > > +#ifndef SHELL_PATH
> > > +# define SHELL_PATH "sh"
> > > +#endif
> > 
> > Does this default ever kick in? The Makefile defaults SHELL_PATH to
> > /bin/sh, so we will always end up with at least that.
> 
> Not when using the build system, but as Hannes mentioned, there is
> potential for this to be used outside of the default build system, so
> I think having the fallback is a good defensive option.  Should it
> maybe be set to /bin/sh though to be more consistent with system()?

Yeah, I think making it "/bin/sh" is better. It's more obvious to people
reading the code as part of git (since even though it is a repetition of
the default from the Makefile, at least it is the _same_ default), and
if it does get reused, it's probably a better default.

> Given the rest of the discussion that happened, I think I understand
> that my patch is actually ok with the following caveats:
> 
> 1. My commit message still needs work.
> 2. Possibly change the default setting of the SHELL_PATH macro from
>    "sh" to "/bin/sh"
> 3. Use the _SQ variant of SHELL_PATH.

Yeah, I think so (not that I am the final word, but that at least
matches my perception of the discussion so far, too).

> (The tracking of changes for SHELL_PATH is considered too heavy for
> now when the other _PATH items aren't tracked the same way.  This
> might make a nice separate patch series though, using the idea from
> the kernel where individual commands are tracked.)

Agreed.

-Peff

[PATCH] Use SHELL_PATH from build system in run_command.c:prepare_shell_cmd

[Ben Walton]

During the testing of the 1.7.10 rc series on Solaris for OpenCSW, it
was discovered that t7006-pager was failing due to finding a bad "sh"
in PATH after a call to execvp("sh", ...).  This call was setup by
run_command.c:prepare_shell_cmd.

The SANE_TOOL_PATH in use at the time was lead by /opt/csw/gnu and
/opt/csw/bin as used by OpenCSW packages so that OpenCSW packaged GNU
tools are found instead of system versions.  A package named
schilyutils (Joerg Schilling's utilities) was installed on the build
system and it provided a version of the traditional Solaris
/usr/bin/sh, as /opt/csw/bin/sh.  This version of "sh" contains many
of the same problems as the traditional Solaris /usr/bin/sh.

The command-specific pager test failed due to the broken "sh" handling
^ as a pipe character.  It tried to fork two processes when it
encountered "sed s/^/foo:/" as the pager command.  This problem was
entirely dependent on the PATH of the user at runtime.

Possible fixes for this issue are:

1. Use the standard system() or popen() which both launch a POSIX
   shell on Solaris as long as _POSIX_SOURCE is defined.

2. The git wrapper could prepend SANE_TOOL_PATH to PATH for
   consistency with builtin commands.

3. The run_command.c:prepare_shell_command() could use the same
   SHELL_PATH that is in the #! line of all all scripts.

Option 1 would preclude opening a bidirectional pipe to a filter
script and would also break git for Windows as cmd.exe is spawned from
system() (cf. v1.7.5-rc0~144^2, "alias: use run_command api to execute
aliases, 2011-01-07).

Option 2 is voided by the same example that turned up this issue.
SANE_TOOL_PATH might also include 'insane' tools.

Option 3 is the best choice at this time.

After this patch, $GIT_PAGER is interpreted by the same shell in
scripted commands which use the git_pager function that uses "eval"
and builtins which use the run_command machinery.

The default shell used by commands will be /bin/sh if not overridden
by the build system.  (This allows for use of this code without the
build system, which was noted during the discussion as a good
quality.[1]) The build always system will pass the value of
SHELL_PATH, which default to /bin/sh as well.

[1] http://thread.gmane.org/gmane.comp.version-control.git/193866/focus=194018

Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
---
 Makefile      |    2 ++
 run-command.c |    6 +++++-
 2 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index be1957a..dea1f15 100644
--- a/Makefile
+++ b/Makefile
@@ -1913,6 +1913,8 @@ builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
 	'-DGIT_MAN_PATH="$(mandir_SQ)"' \
 	'-DGIT_INFO_PATH="$(infodir_SQ)"'
 
+run-command.o: EXTRA_CPPFLAGS = -DSHELL_PATH='"$(SHELL_PATH_SQ)"'
+
 $(BUILT_INS): git$X
 	$(QUIET_BUILT_IN)$(RM) $@ && \
 	ln git$X $@ 2>/dev/null || \
diff --git a/run-command.c b/run-command.c
index 1db8abf..2af3e0f 100644
--- a/run-command.c
+++ b/run-command.c
@@ -4,6 +4,10 @@
 #include "sigchain.h"
 #include "argv-array.h"
 
+#ifndef SHELL_PATH
+# define SHELL_PATH "/bin/sh"
+#endif
+
 struct child_to_clean {
 	pid_t pid;
 	struct child_to_clean *next;
@@ -90,7 +94,7 @@ static const char **prepare_shell_cmd(const char **argv)
 		die("BUG: shell command is empty");
 
 	if (strcspn(argv[0], "|&;<>()$`\\\"' \t\n*?[#~=%") != strlen(argv[0])) {
-		nargv[nargc++] = "sh";
+		nargv[nargc++] = SHELL_PATH;
 		nargv[nargc++] = "-c";
 
 		if (argc < 2)
-- 
1.7.5.4

Re: [PATCH] Use SHELL_PATH to fork commands in run_command.c:prepare_shell_cmd

[Ben Walton]

Excerpts from Jeff King's message of Wed Mar 28 00:22:15 -0400 2012:

> Yeah, I think so (not that I am the final word, but that at least
> matches my perception of the discussion so far, too).

Ok, I just sent the updated patch.  Thanks to everyone that has
provided feedback on this so far.

Thanks
-Ben
--
Ben Walton
Systems Programmer - CHASS
University of Toronto
C:416.407.5610 | W:416.978.4302

Re: [PATCH] Use SHELL_PATH from build system in run_command.c:prepare_shell_cmd

[Junio C Hamano]

Ben Walton <bwalton@artsci.utoronto.ca> writes:

> Possible fixes for this issue are:
> ...
> 2. The git wrapper could prepend SANE_TOOL_PATH to PATH for
>    consistency with builtin commands.
>
> 3. The run_command.c:prepare_shell_command() could use the same
>    SHELL_PATH that is in the #! line of all all scripts.
> ...
> Option 2 is voided by the same example that turned up this issue.
> SANE_TOOL_PATH might also include 'insane' tools.
>
> Option 3 is the best choice at this time.

This line of reasoning makes me feel uneasy.

The approach may allow a directory that has "insane" tools in it to be on
the SANE_TOOL_PATH for *this* particular codepath, but at the same time,
it encourages people to build Git with such a broken SANE_TOOL_PATH.

Aren't you exposing your users to potential breakage in other parts of the
system, caused by broken tools on SANE_TOOL_PATH that is not shell, by
doing so?  Worse yet, there is no escape hatch similar to SHELL_PATH for
such tools.

Will queue on 'pu' for now, but I am not convinced.

Re: [PATCH] Use SHELL_PATH from build system in run_command.c:prepare_shell_cmd

[Jonathan Nieder]

Junio C Hamano wrote:
> Ben Walton <bwalton@artsci.utoronto.ca> writes:

>> Possible fixes for this issue are:
>> ...
>> 2. The git wrapper could prepend SANE_TOOL_PATH to PATH for
>>    consistency with builtin commands.
>>
>> 3. The run_command.c:prepare_shell_command() could use the same
>>    SHELL_PATH that is in the #! line of all all scripts.
>> ...
>> Option 2 is voided by the same example that turned up this issue.
>> SANE_TOOL_PATH might also include 'insane' tools.
[...]
> This line of reasoning makes me feel uneasy.

I think the missing detail in the above explanation is that "sh" is a
special case.  Even though POSIX and good sense would require "sh" to
name a reasonable shell just like "grep" needs to name a reasonable
grep, on Solaris we have never been able to count on an "sh" for git's
needs and common practice seems to be to give up and just use bash or
ksh.

See v1.5.5-rc0~5^2~3 (filter-branch: use $SHELL_PATH instead of 'sh',
2008-03-12), for example.

Re: [PATCH] Use SHELL_PATH from build system in run_command.c:prepare_shell_cmd

[Jonathan Nieder]

Ben Walton wrote:

[...]
> 2. The git wrapper could prepend SANE_TOOL_PATH to PATH for
>    consistency with builtin commands.

The phrase "builtin commands" left me confused for a moment ---
busybox-style builtins and standalone commands in C both don't get the
PATH fixup.

I think you meant scripted commands (i.e., commands that get the PATH
fixup by sourcing git-sh-setup).

> 3. The run_command.c:prepare_shell_command() could use the same
>    SHELL_PATH that is in the #! line of all all scripts.
>
> Option 1 would preclude opening a bidirectional pipe to a filter
> script and would also break git for Windows as cmd.exe is spawned from
> system() (cf. v1.7.5-rc0~144^2, "alias: use run_command api to execute
> aliases, 2011-01-07).
>
> Option 2 is voided by the same example that turned up this issue.
> SANE_TOOL_PATH might also include 'insane' tools.

As Junio mentioned, the only insane tool that SANE_TOOL_PATH is
allowed to point to is sh.  I guess the note on insane tools was meant
as a wistful observation, but it didn't come through clearly.

Maybe a reference to v1.5.5-rc0~5^2~3 (filter-branch: use $SHELL_PATH
instead of 'sh', 2008-03-12) would make the motivation behind the
"don't trust sh" principle clearer.

> Option 3 is the best choice

Makes sense to me and the patch looks good.  Thanks for your
perseverance.

Ciao,
Jonathan

Re: [PATCH] Use SHELL_PATH from build system in run_command.c:prepare_shell_cmd

[Jonathan Nieder]

Ben Walton wrote:

> The issue really comes down to the fact the current code allows a
> user's environment at runtime to negatively affect the ability to run
> commands that would otherwise be fine.
[...]
> The way that SANE_TOOL_PATH is injected into PATH for shell scripts
> only sees it given priority over /bin and /usr/bin (and things
> following them).  If this were mimicked for consistency in the
> non-script parts of git, users would still be able to have paths with
> 'insane' tools given precedence.

Oh, that's what you meant.

Git could unconditionally override or prefix the PATH with some value
determined at build time, but we deliberately gave users more control
than that.  git-rebase.sh et al make use of various tools from the
usual Unix toolset, expecting to find them on the PATH.  If the user
sets the PATH to point to the Plan 9 tools, say, then these scripts
will break.  Is that a problem?

More importantly: is it the same problem your patch fixes?

If "yes", then that is problematic.  If the SANE_TOOL_PATH facility is
broken, we should be fixing or eliminating it, not piling workarounds
on top.  If we want to say 'sh' is special, we should mean it.

[My personal answers are "no, give them rope" and "no, they are
different problems", so I'm not too worried. ;-)]

Thanks for some food for thought.
Jonathan

[PATCH] Use SHELL_PATH from build system in run_command.c:prepare_shell_cmd

[Ben Walton]

During the testing of the 1.7.10 rc series on Solaris for OpenCSW, it
was discovered that t7006-pager was failing due to finding a bad "sh"
in PATH after a call to execvp("sh", ...).  This call was setup by
run_command.c:prepare_shell_cmd.

The PATH in use at the time saw /opt/csw/bin given precedence to
traditional Solaris paths such as /usr/bin and /usr/xpg4/bin.  A
package named schilyutils (Joerg Schilling's utilities) was installed
on the build system and it delivered a modified version of the
traditional Solaris /usr/bin/sh as /opt/csw/bin/sh.  This version of
sh suffers from many of the same problems as /usr/bin/sh.

The command-specific pager test failed due to the broken "sh" handling
^ as a pipe character.  It tried to fork two processes when it
encountered "sed s/^/foo:/" as the pager command.  This problem was
entirely dependent on the PATH of the user at runtime.

Possible fixes for this issue are:

1. Use the standard system() or popen() which both launch a POSIX
   shell on Solaris as long as _POSIX_SOURCE is defined.

2. The git wrapper could prepend SANE_TOOL_PATH to PATH thus forcing
   all unqualified commands run to use the known good tools on the
   system.

3. The run_command.c:prepare_shell_command() could use the same
   SHELL_PATH that is in the #! line of all all scripts and not rely
   on PATH to find the sh to run.

Option 1 would preclude opening a bidirectional pipe to a filter
script and would also break git for Windows as cmd.exe is spawned from
system() (cf. v1.7.5-rc0~144^2, "alias: use run_command api to execute
aliases, 2011-01-07).

Option 2 is not friendly to users as it would negate their ability to
use tools of their choice in many cases.  Alternately, injecting
SANE_TOOL_PATH such that it takes precedence over /bin and /usr/bin
(and anything with lower precedence than those paths) as
git-sh-setup.sh does would not solve the problem either as the user
environment could still allow a bad sh to be found.  (Many OpenCSW
users will have /opt/csw/bin leading their PATH and some subset would
have schilyutils installed.)

Option 3 allows us to use a known good shell while still honouring the
users' PATH for the utilities being run.  Thus, it solves the problem
while not negatively impacting either users or git's ability to run
external commands in convenient ways.  Essentially, the shell is a
special case of tool that should not rely on SANE_TOOL_PATH and must
be called explicitly.

With this patch applied, any code path leading to
run_command.c:prepare_shell_cmd can count on using the same sane shell
that all shell scripts in the git suite use.  Both the build system
and run_command.c will default this shell to /bin/sh unless
overridden.

Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
---
 Makefile      |    2 ++
 run-command.c |    6 +++++-
 2 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index be1957a..dea1f15 100644
--- a/Makefile
+++ b/Makefile
@@ -1913,6 +1913,8 @@ builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
 	'-DGIT_MAN_PATH="$(mandir_SQ)"' \
 	'-DGIT_INFO_PATH="$(infodir_SQ)"'
 
+run-command.o: EXTRA_CPPFLAGS = -DSHELL_PATH='"$(SHELL_PATH_SQ)"'
+
 $(BUILT_INS): git$X
 	$(QUIET_BUILT_IN)$(RM) $@ && \
 	ln git$X $@ 2>/dev/null || \
diff --git a/run-command.c b/run-command.c
index 1db8abf..2af3e0f 100644
--- a/run-command.c
+++ b/run-command.c
@@ -4,6 +4,10 @@
 #include "sigchain.h"
 #include "argv-array.h"
 
+#ifndef SHELL_PATH
+# define SHELL_PATH "/bin/sh"
+#endif
+
 struct child_to_clean {
 	pid_t pid;
 	struct child_to_clean *next;
@@ -90,7 +94,7 @@ static const char **prepare_shell_cmd(const char **argv)
 		die("BUG: shell command is empty");
 
 	if (strcspn(argv[0], "|&;<>()$`\\\"' \t\n*?[#~=%") != strlen(argv[0])) {
-		nargv[nargc++] = "sh";
+		nargv[nargc++] = SHELL_PATH;
 		nargv[nargc++] = "-c";
 
 		if (argc < 2)
-- 
1.7.5.4

Re: [PATCH] Use SHELL_PATH from build system in run_command.c:prepare_shell_cmd

[Jonathan Nieder]

Ben Walton wrote:

> During the testing of the 1.7.10 rc series on Solaris for OpenCSW, it
> was discovered that t7006-pager was failing
[...]
> --- a/run-command.c
> +++ b/run-command.c
> @@ -4,6 +4,10 @@
>  #include "sigchain.h"
>  #include "argv-array.h"
>  
> +#ifndef SHELL_PATH
> +# define SHELL_PATH "/bin/sh"
> +#endif
> +
>  struct child_to_clean {
>  	pid_t pid;
>  	struct child_to_clean *next;
> @@ -90,7 +94,7 @@ static const char **prepare_shell_cmd(const char **argv)
>  		die("BUG: shell command is empty");
>  
>  	if (strcspn(argv[0], "|&;<>()$`\\\"' \t\n*?[#~=%") != strlen(argv[0])) {
> -		nargv[nargc++] = "sh";
> +		nargv[nargc++] = SHELL_PATH;

The underlying problem is an old one.  Thanks for fixing it.

For what it's worth,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

[...]
> Essentially, the shell is a
> special case of tool that should not rely on SANE_TOOL_PATH and must
> be called explicitly.

It is probably annoying to hear me say this, but:

The above doesn't tell me _why_ it is a special case and that on
Solaris users have been burned by "sh" being the original Bourne shell
or a temperamental version of ksh so SHELL_PATH usually points to bash
or ksh93 instead.

I trust the reader enough to fill in the blank, though, so I think the
patch is ok as is.

Re: [PATCH] Use SHELL_PATH from build system in run_command.c:prepare_shell_cmd

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Ben Walton wrote:
>
> The underlying problem is an old one.  Thanks for fixing it.
>
> For what it's worth,
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks, all of you who participated in the review process.

Re: [PATCH] Use SHELL_PATH from build system in run_command.c:prepare_shell_cmd

[Jonathan Nieder]

Ben Walton wrote:

> --- a/Makefile
> +++ b/Makefile
> @@ -1913,6 +1913,8 @@ builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
>  	'-DGIT_MAN_PATH="$(mandir_SQ)"' \
>  	'-DGIT_INFO_PATH="$(infodir_SQ)"'
>  
> +run-command.o: EXTRA_CPPFLAGS = -DSHELL_PATH='"$(SHELL_PATH_SQ)"'

Sorry I forgot to send this before.  Quick tweaks:

 - let assembler listings (from "make run-command.s") and sparse checks
   reflect the SHELL_PATH setting, too

 - the entire -DFOO="bar" argument is surrounded with single quotes
   in other EXTRA_CPPFLAGS settings, so let this one follow that
   pattern to avoid standing out

diff --git i/Makefile w/Makefile
index dea1f157..a3791139 100644
--- i/Makefile
+++ w/Makefile
@@ -1913,7 +1913,8 @@ builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
 	'-DGIT_MAN_PATH="$(mandir_SQ)"' \
 	'-DGIT_INFO_PATH="$(infodir_SQ)"'
 
-run-command.o: EXTRA_CPPFLAGS = -DSHELL_PATH='"$(SHELL_PATH_SQ)"'
+run-command.sp run-command.s run-command.o: EXTRA_CPPFLAGS = \
+	'-DSHELL_PATH="$(SHELL_PATH_SQ)"'
 
 $(BUILT_INS): git$X
 	$(QUIET_BUILT_IN)$(RM) $@ && \

Re: [PATCH] Use SHELL_PATH from build system in run_command.c:prepare_shell_cmd

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Sorry I forgot to send this before.  Quick tweaks:
>
>  - let assembler listings (from "make run-command.s") and sparse checks
>    reflect the SHELL_PATH setting, too
>
>  - the entire -DFOO="bar" argument is surrounded with single quotes
>    in other EXTRA_CPPFLAGS settings, so let this one follow that
>    pattern to avoid standing out
>
> diff --git i/Makefile w/Makefile
> index dea1f157..a3791139 100644
> --- i/Makefile
> +++ w/Makefile
> @@ -1913,7 +1913,8 @@ builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
>  	'-DGIT_MAN_PATH="$(mandir_SQ)"' \
>  	'-DGIT_INFO_PATH="$(infodir_SQ)"'
>  
> -run-command.o: EXTRA_CPPFLAGS = -DSHELL_PATH='"$(SHELL_PATH_SQ)"'
> +run-command.sp run-command.s run-command.o: EXTRA_CPPFLAGS = \
> +	'-DSHELL_PATH="$(SHELL_PATH_SQ)"'
>  
>  $(BUILT_INS): git$X
>  	$(QUIET_BUILT_IN)$(RM) $@ && \

Actually, I do not think this is sufficient, and it happens that you and I
are the best position to realize it ;-).

Look at what is done in the Makefile for DEFAULT_EDITOR and DEFAULT_PAGER,
and compare with what the above is doing, and think why the EDITOR/PAGER
needs to have another level of quoting.

Re: [PATCH] Use SHELL_PATH from build system in run_command.c:prepare_shell_cmd

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jonathan Nieder <jrnieder@gmail.com> writes:
> ...
>> -run-command.o: EXTRA_CPPFLAGS = -DSHELL_PATH='"$(SHELL_PATH_SQ)"'
>> +run-command.sp run-command.s run-command.o: EXTRA_CPPFLAGS = \
>> +	'-DSHELL_PATH="$(SHELL_PATH_SQ)"'
>>  
>>  $(BUILT_INS): git$X
>>  	$(QUIET_BUILT_IN)$(RM) $@ && \
>
> Actually, I do not think this is sufficient, and it happens that you and I
> are in the best position to realize it ;-).
>
> Look at what is done in the Makefile for DEFAULT_EDITOR and DEFAULT_PAGER,
> and compare with what the above is doing, and think why the EDITOR/PAGER
> needs to have another level of quoting.

In other words, something like this squashed into Ben's patch...

diff --git i/Makefile w/Makefile
index dea1f15..abee43e 100644
--- i/Makefile
+++ w/Makefile
@@ -1849,6 +1849,13 @@ DEFAULT_PAGER_CQ_SQ = $(subst ','\'',$(DEFAULT_PAGER_CQ))
 BASIC_CFLAGS += -DDEFAULT_PAGER='$(DEFAULT_PAGER_CQ_SQ)'
 endif
 
+ifdef SHELL_PATH
+SHELL_PATH_CQ = "$(subst ",\",$(subst \,\\,$(SHELL_PATH)))"
+SHELL_PATH_CQ_SQ = $(subst ','\'',$(SHELL_PATH_CQ))
+
+BASIC_CFLAGS += -DSHELL_PATH='$(SHELL_PATH_CQ_SQ)'
+endif
+
 ALL_CFLAGS += $(BASIC_CFLAGS)
 ALL_LDFLAGS += $(BASIC_LDFLAGS)
 
@@ -1913,8 +1920,6 @@ builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
 	'-DGIT_MAN_PATH="$(mandir_SQ)"' \
 	'-DGIT_INFO_PATH="$(infodir_SQ)"'
 
-run-command.o: EXTRA_CPPFLAGS = -DSHELL_PATH='"$(SHELL_PATH_SQ)"'
-
 $(BUILT_INS): git$X
 	$(QUIET_BUILT_IN)$(RM) $@ && \
 	ln git$X $@ 2>/dev/null || \