patchFix false positives in t3404 due to SHELL=/bin/false

If the user's shell in NSS passwd is /bin/false (eg as found during Gentoo's package building), the git-rebase exec tests will fail, because they call $SHELL around the command, and in the existing testcase, $SHELL was not being cleared sufficently.

This lead to false positive failures of t3404 on systems where the package build user was locked down as noted above.

Signed-off-by: "Robin H. Johnson" <robbat2@gentoo.org>
X-Gentoo-Bug: 349083
X-Gentoo-Bug-URL: http://bugs.gentoo.org/show_bug.cgi?id=349083

diff -Nuar git-1.7.3.4.orig/t/t3404-rebase-interactive.sh git-1.7.3.4/t/t3404-rebase-interactive.sh --- git-1.7.3.4.orig/t/t3404-rebase-interactive.sh 2010-12-16 02:52:11.000000000 +0000 +++ git-1.7.3.4/t/t3404-rebase-interactive.sh 2010-12-26 22:30:47.826421313 +0000

"Robin H. Johnson" <robbat2@gentoo.org> writes:

Thanks.

This probably is still not portable.

	SHELL=
        export SHELL

would be Ok, though.

If the user's shell in NSS passwd is /bin/false (eg as found during Gentoo's package building), the git-rebase exec tests will fail, because they call $SHELL around the command, and in the existing testcase, $SHELL was not being cleared sufficently.

This lead to false positive failures of t3404 on systems where the package build user was locked down as noted above.

Signed-off-by: "Robin H. Johnson" <robbat2@gentoo.org>
X-Gentoo-Bug: 349083
X-Gentoo-Bug-URL: http://bugs.gentoo.org/show_bug.cgi?id=349083
---
 t/t3404-rebase-interactive.sh |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

"Robin H. Johnson" <robbat2@gentoo.org> writes:

Thanks; will queue this version to 'maint'.

I have this nagging suspicion that we may want to revisit this to assign $SHELL_PATH to it before exporting, and that this might be better done in t/test-lib.sh at the beginning. Note that unlike my earlier "your v1 might be less portable than desired", these two points are only speculations and RFCs.

 # "exec" commands are ran with the user shell by default, but this may
 # be non-POSIX. For example, if SHELL=zsh then ">file" doesn't work
 # to create a file. Unseting SHELL avoids such non-portable behavior

Perl's exec and system do not use SHELL (as far as perlfunc states).  It uses /bin/sh -c "$cmd", or a platform-dependent equivalent.

$SHELL is typically only used when a program wants to invoke a user-shell (ie: editor shell-escape, xterm, typescript, screen).

How was SHELL=/bin/false causing problems?  Is git using $SHELL?

On Tue, Jan 04, 2011 at 09:43:12AM -0500, Vallon, Justin wrote:

git-rebase--interactive.sh:
====
${SHELL:-@SHELL_PATH@} -c "$rest" # Actual execution
status=$?
if test "$status" -ne 0
then
	warn "Execution failed: $rest"
====

This always triggers with SHELL=/bin/false if SHELL is unset or empty, SHELL_PATH gets substituted, which tends to be the correct /bin/sh.

"Vallon, Justin" <Justin.Vallon@deshaw.com> writes:

The explanation is in the comment right above the modification in the patch. "user's shell" can be read as "$SHELL":

(my bad, I wrote this SHELL= without exporting it. Since bash re-exports already exported variables when they are assigned, and my /bin/sh points to bash, I didn't notice)

Matthieu Moy wrote:

Isn't that how export works in all Bourne-style shells?  For example:

	$ env var=outside dash -c '
		var=inside;
		dash -c "echo \$var"
	  '
	inside
	$

Maybe in the failing case SHELL was not exported but just set to /bin/false in .bashrc or similar?

Jonathan Nieder <jrnieder@gmail.com> writes:

Thanks, you saved me some time responding ;-)

Matthieu's diagnosis is only half correct in that bash is why he didn't notice the problem, but if in this sequence

	var=foo
        export var
        var=bar
        some-command

some-command does not see "bar" as the value of environment variable "var", your shell is not POSIX (there is no such thing as "re-exporting").

Either a variable is marked with the export attribute, in which case the processes spawned from the shell sees the value of the then-current shell variable in their environments, or they don't for shell variables that are not marked with the export attribute.

The real reason the problem went unnoticed was because bash automatially marks SHELL with the export attribute.

Because POSIX shells are required to mark variables they inherit from the environment with the export attribute, your tests will run with SHELL exported to the environment if your usual shell is bash (i.e. SHELL is already exported to processes it spawns), even if you use another POSIX shell to run your git and tests. That makes the issue doubly harder to notice.

But, when you say "your shell", you are really referring to /bin/sh, because this behavior is being observed in t/t3404-rebase-interactive.sh.  Which leads to...

Robin: have you observed the problem with Gentoo's /bin/sh?

X=1 ; export X ; /bin/sh -c 'X= ; env | grep ^X='

If so, I would qualify the export with a comment about the mis-behavior:

# Reexport in case sh is non-POSIX export SHELL

(or, just unset SHELL)

Else, I don't think the re-export is needed (something else is causing your trouble).

I don't really follow this.  The #! line is /bin/sh.  The user's $SHELL does not come into play.  Either SHELL is in /bin/sh's environment and it should be cleared in the child, or it isn't and it won't matter.

"Vallon, Justin" <Justin.Vallon@deshaw.com> writes:

Read what you are responding to again.

The "doubly harder to notice" is _not_ about gentoo's /bin/sh, but about the experiment Matthieu did (ask: "what shell spawned t3404 that has the she-bang /bin/sh?").

If that shell is bash, which automatically marks SHELL with the export attribute, it places the variable in the environment. t3404 is run under /bin/sh, which presumably is POSIX and initializes its shell variable SHELL with what was in the environment, and while doing so, it also marks the variable with the export attribute. The script does not "unset SHELL" but merely assigns an empty string to it, which is the value to be exported to the processes the script runs.

Imagine that whoever was having trouble did not have SHELL exported to the environment when t3404 is run. The script assigns an empty string to its shell variable SHELL but nothing marks the variable with the export attribute, hence the processes the script runs will never see that as the value of the environment variable (in fact, they wouldn't see SHELL environment variable at all).