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

[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 || \

[PATCH] Do not use SHELL_PATH from build system in prepare_shell_cmd on Windows

[Johannes Sixt]

From: Johannes Sixt <j6t@kdbg.org>

The recent change to use SHELL_PATH instead of "sh" to spawn shell commands
is not suited for Windows:

- The default setting, "/bin/sh", does not work when git has to run the
  shell because it is a POSIX style path, but not a proper Windows style
  path.

- If it worked, it would hard-code a position in the files system where
  the shell is expected, making git (more precisely, the POSIX toolset that
  is needed alongside git) non-relocatable. But we cannot sacrifice
  relocatability on Windows.

- Apart from that, even though the Makefile leaves SHELL_PATH set to
  "/bin/sh" for the Windows builds, the build system passes a mangled path
  to the compiler, and something like "D:/Src/msysgit/bin/sh" is used,
  which is doubly bad because it points to where /bin/sh resolves to on
  the system where git was built.

- Finally, the system's CreateProcess() function that is used under
  mingw.c's hood does not work with forward slashes and cannot find the
  shell.

Undo the earlier change on Windows.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
Am 3/31/2012 3:33, schrieb Ben Walton:
> +#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)

This looked so obviously correct, that I carelessly did not test the
change. But given that the first two points addressed in the commit
message above are actually dealbreakers, I should have noticed this issue
much earlier.

-- Hannes

 run-command.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/run-command.c b/run-command.c
index 2af3e0f..e4edede 100644
--- a/run-command.c
+++ b/run-command.c
@@ -94,7 +94,11 @@ static const char **prepare_shell_cmd(const char **argv)
 		die("BUG: shell command is empty");
 
 	if (strcspn(argv[0], "|&;<>()$`\\\"' \t\n*?[#~=%") != strlen(argv[0])) {
+#ifndef WIN32
 		nargv[nargc++] = SHELL_PATH;
+#else
+		nargv[nargc++] = "sh";
+#endif
 		nargv[nargc++] = "-c";
 
 		if (argc < 2)
-- 
1.7.10.1370.ga6f62.dirty

Re: [PATCH] Do not use SHELL_PATH from build system in prepare_shell_cmd on Windows

[Ben Walton]

Excerpts from Johannes Sixt's message of Tue Apr 17 03:03:21 -0400 2012:

Hi Hannes,

> The recent change to use SHELL_PATH instead of "sh" to spawn shell
> commands is not suited for Windows:

Ah yes, I can see how that would be undesirable on Windows.


> +#ifndef WIN32
>          nargv[nargc++] = SHELL_PATH;
> +#else
> +        nargv[nargc++] = "sh";
> +#endif
>          nargv[nargc++] = "-c";

A minor style question: Would this conditional code not be better if
it were up where the fallback SHELL_PATH is set?  That would leave
this code block #ifdef free while still avoiding the breakage on
Windows.

Admittedly, it's a very minor point and possibly entirely a personal
preference.

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

Re: [PATCH] Do not use SHELL_PATH from build system in prepare_shell_cmd on Windows

[Johannes Sixt]

Am 4/17/2012 15:45, schrieb Ben Walton:
> Excerpts from Johannes Sixt's message of Tue Apr 17 03:03:21 -0400 2012:
>> The recent change to use SHELL_PATH instead of "sh" to spawn shell
>> commands is not suited for Windows:
> 
> Ah yes, I can see how that would be undesirable on Windows.
> 
>> +#ifndef WIN32
>>          nargv[nargc++] = SHELL_PATH;
>> +#else
>> +        nargv[nargc++] = "sh";
>> +#endif
>>          nargv[nargc++] = "-c";
> 
> A minor style question: Would this conditional code not be better if
> it were up where the fallback SHELL_PATH is set?  That would leave
> this code block #ifdef free while still avoiding the breakage on
> Windows.

It would require either to #define yet another new name to SHELL_PATH or
"sh", or to #undef SHELL_PATH and re#define it for WIN32. Neither is very
appealing. I personally prefer to have the conditional (if it is as short
as this one) at the point where it matters.

-- Hannes

Re: [PATCH] Do not use SHELL_PATH from build system in prepare_shell_cmd on Windows

[Ben Walton]

Excerpts from Johannes Sixt's message of Tue Apr 17 10:00:25 -0400 2012:

> It would require either to #define yet another new name to
> SHELL_PATH or "sh", or to #undef SHELL_PATH and re#define it for
> WIN32. Neither is very appealing. I personally prefer to have the
> conditional (if it is as short as this one) at the point where it
> matters.

Ok.  I'll defer to your more experienced judgement. :)  Thanks for
clarifying though.

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

Re: [PATCH] Do not use SHELL_PATH from build system in prepare_shell_cmd on Windows

[Jeff King]

On Tue, Apr 17, 2012 at 09:03:21AM +0200, Johannes Sixt wrote:

> diff --git a/run-command.c b/run-command.c
> index 2af3e0f..e4edede 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -94,7 +94,11 @@ static const char **prepare_shell_cmd(const char **argv)
>  		die("BUG: shell command is empty");
>  
>  	if (strcspn(argv[0], "|&;<>()$`\\\"' \t\n*?[#~=%") != strlen(argv[0])) {
> +#ifndef WIN32
>  		nargv[nargc++] = SHELL_PATH;
> +#else
> +		nargv[nargc++] = "sh";
> +#endif
>  		nargv[nargc++] = "-c";
>  
>  		if (argc < 2)

It sounds like the real problem is not the use of a configurable shell,
but rather the use of an absolute path. Should you maybe try to pass the
basename of SHELL_PATH? Or maybe that is not even worth worrying about,
as somebody on Windows is not going to ever set SHELL_PATH, since it is
not like they are working around a non-POSIX "sh" included with the
operating system (which is why people on Solaris typically set
SHELL_PATH).

-Peff

Re: [PATCH] Do not use SHELL_PATH from build system in prepare_shell_cmd on Windows

[Johannes Sixt]

Am 4/18/2012 0:14, schrieb Jeff King:
> On Tue, Apr 17, 2012 at 09:03:21AM +0200, Johannes Sixt wrote:
> 
>> diff --git a/run-command.c b/run-command.c
>> index 2af3e0f..e4edede 100644
>> --- a/run-command.c
>> +++ b/run-command.c
>> @@ -94,7 +94,11 @@ static const char **prepare_shell_cmd(const char **argv)
>>  		die("BUG: shell command is empty");
>>  
>>  	if (strcspn(argv[0], "|&;<>()$`\\\"' \t\n*?[#~=%") != strlen(argv[0])) {
>> +#ifndef WIN32
>>  		nargv[nargc++] = SHELL_PATH;
>> +#else
>> +		nargv[nargc++] = "sh";
>> +#endif
>>  		nargv[nargc++] = "-c";
>>  
>>  		if (argc < 2)
> 
> It sounds like the real problem is not the use of a configurable shell,
> but rather the use of an absolute path. Should you maybe try to pass the
> basename of SHELL_PATH? Or maybe that is not even worth worrying about,
> as somebody on Windows is not going to ever set SHELL_PATH, since it is
> not like they are working around a non-POSIX "sh" included with the
> operating system (which is why people on Solaris typically set
> SHELL_PATH).

I thought about offering a customization point, but decided that it is not
worth the hassle: Most people download an installer, then the installer
can set up the PATH so that "sh" is not broken or something entirely
unrelated. And those who build git themselves know sufficiently well what
they are doing.

-- Hannes

Re: [PATCH] Do not use SHELL_PATH from build system in prepare_shell_cmd on Windows

[Jeff King]

On Wed, Apr 18, 2012 at 07:39:43AM +0200, Johannes Sixt wrote:

> > It sounds like the real problem is not the use of a configurable shell,
> > but rather the use of an absolute path. Should you maybe try to pass the
> > basename of SHELL_PATH? Or maybe that is not even worth worrying about,
> > as somebody on Windows is not going to ever set SHELL_PATH, since it is
> > not like they are working around a non-POSIX "sh" included with the
> > operating system (which is why people on Solaris typically set
> > SHELL_PATH).
> 
> I thought about offering a customization point, but decided that it is not
> worth the hassle: Most people download an installer, then the installer
> can set up the PATH so that "sh" is not broken or something entirely
> unrelated. And those who build git themselves know sufficiently well what
> they are doing.

OK. I'll defer your judgement on what Windows users prefer. I just
wanted to make sure it was a conscious decision.

Other than that, the patch looks obviously correct to me. Thanks, and
sorry for inadvertently breaking git on windows (again). :)

-Peff

Re: [PATCH] Do not use SHELL_PATH from build system in prepare_shell_cmd on Windows

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

>> It sounds like the real problem is not the use of a configurable shell,
>> but rather the use of an absolute path. Should you maybe try to pass the
>> basename of SHELL_PATH? ...
>
> I thought about offering a customization point, but decided that it is not
> worth the hassle...

I think you meant by the above that the installer could offer the user to
choose where to place the shell, and if that is the case, I agree it would
probably not worth it.

There is one thing I am confused about your response, though.  I thought
the suggestion by Peff was to build your binary with "make SHELL_PATH=sh"
(not "make SHELL_PATH=/bin/sh").  I do not know if that works or does not
work (I do not see why not, though), but in either case offering a new
customization point sounds like a separate issue.

Re: [PATCH] Do not use SHELL_PATH from build system in prepare_shell_cmd on Windows

[Johannes Sixt]

Am 4/18/2012 18:30, schrieb Junio C Hamano:
> There is one thing I am confused about your response, though.  I thought
> the suggestion by Peff was to build your binary with "make SHELL_PATH=sh"
> (not "make SHELL_PATH=/bin/sh").  I do not know if that works or does not
> work (I do not see why not, though), but in either case offering a new
> customization point sounds like a separate issue.

I tried this, and it does not work. (Shell scripts' shbang line would end
up as '#!sh', and at least bash does not like to run such scripts.)

The alternative is to customize the -DSHELL_PATH=... compiler option for
Windows, and that is primarily what I don't like: It would mean that
either the name "SHELL_PATH" has different meaning in C code and the
Makefile, or we need a new name for the C macro. (The latter would be "a
new customization point".)

I learn from this conversation that I should note failed alternatives more
frequently in the commit message to save reviewers' time.

-- Hannes

Re: [PATCH] Do not use SHELL_PATH from build system in prepare_shell_cmd on Windows

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Am 4/18/2012 18:30, schrieb Junio C Hamano:
>> There is one thing I am confused about your response, though.  I thought
>> the suggestion by Peff was to build your binary with "make SHELL_PATH=sh"
>> (not "make SHELL_PATH=/bin/sh").  I do not know if that works or does not
>> work (I do not see why not, though), but in either case offering a new
>> customization point sounds like a separate issue.
>
> I tried this, and it does not work. (Shell scripts' shbang line would end
> up as '#!sh', and at least bash does not like to run such scripts.)

Ahh, sorry for missing the obvious.  I should have realized about that
one, as customizing that one was the very original motivation of the make
variable.

In any case, as I wrote in the "What's cooking", I'll fast-track the fix
to 'master' shortly, so that Windows port does not have to carry it.

Thanks.

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 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:

> 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

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