[PATCH] Fix false positives in t3404 due to SHELL=/bin/false

[PATCH] Fix false positives in t3404 due to SHELL=/bin/false

[Robin H. Johnson]

[-- Attachment #1: Type: text/plain, Size: 1405 bytes --]

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
@@ -67,8 +67,8 @@
 # "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
-# in tests.
-SHELL=
+# in tests. It must be exported for it to take effect where needed.
+export SHELL=
 
 test_expect_success 'rebase -i with the exec command' '
 	git checkout master &&

-- 
Robin Hugh Johnson
Gentoo Linux: Developer, Trustee & Infrastructure Lead
E-Mail     : robbat2@gentoo.org
GnuPG FP   : 11AC BA4F 4778 E3F6 E4ED  F38E B27B 944E 3488 4E85

[-- Attachment #2: Type: application/pgp-signature, Size: 330 bytes --]

Re: [PATCH] Fix false positives in t3404 due to SHELL=/bin/false

[Junio C Hamano]

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

>  # "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
> -# in tests.
> -SHELL=
> +# in tests. It must be exported for it to take effect where needed.
> +export SHELL=

Thanks.

This probably is still not portable.

	SHELL=
        export SHELL

would be Ok, though.

[PATCH v2] Fix false positives in t3404 due to SHELL=/bin/false

[Robin H. Johnson]

[-- Attachment #1: Type: text/plain, Size: 1442 bytes --]

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(-)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index d3a3bd2..7d8147b 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -71,8 +71,9 @@ test_expect_success 'setup' '
 # "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
-# in tests.
+# in tests. It must be exported for it to take effect where needed.
 SHELL=
+export SHELL
 
 test_expect_success 'rebase -i with the exec command' '
 	git checkout master &&

-- 
Robin Hugh Johnson
Gentoo Linux: Developer, Trustee & Infrastructure Lead
E-Mail     : robbat2@gentoo.org
GnuPG FP   : 11AC BA4F 4778 E3F6 E4ED  F38E B27B 944E 3488 4E85

[-- Attachment #2: Type: application/pgp-signature, Size: 330 bytes --]

Re: [PATCH v2] Fix false positives in t3404 due to SHELL=/bin/false

[Junio C Hamano]

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

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

> ---
>  t/t3404-rebase-interactive.sh |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index d3a3bd2..7d8147b 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -71,8 +71,9 @@ test_expect_success 'setup' '
>  # "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
> -# in tests.
> +# in tests. It must be exported for it to take effect where needed.
>  SHELL=
> +export SHELL

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.

RE: [PATCH v2] Fix false positives in t3404 due to SHELL=/bin/false

[Vallon, Justin]

 # "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?

-- 
-Justin


-----Original Message-----
From: git-owner@vger.kernel.org [mailto:git-owner@vger.kernel.org] On Behalf Of Robin H. Johnson
Sent: Monday, December 27, 2010 3:04 AM
To: Junio C Hamano; git@vger.kernel.org
Subject: [PATCH v2] Fix 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
---
 t/t3404-rebase-interactive.sh |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index d3a3bd2..7d8147b 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -71,8 +71,9 @@ test_expect_success 'setup' '
 # "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
-# in tests.
+# in tests. It must be exported for it to take effect where needed.
 SHELL=
+export SHELL
 
 test_expect_success 'rebase -i with the exec command' '
 	git checkout master &&

-- 
Robin Hugh Johnson
Gentoo Linux: Developer, Trustee & Infrastructure Lead
E-Mail     : robbat2@gentoo.org
GnuPG FP   : 11AC BA4F 4778 E3F6 E4ED  F38E B27B 944E 3488 4E85

Re: [PATCH v2] Fix false positives in t3404 due to SHELL=/bin/false

[Robin H. Johnson]

[-- Attachment #1: Type: text/plain, Size: 1132 bytes --]

On Tue, Jan 04, 2011 at 09:43:12AM -0500, Vallon, Justin wrote:
>  # "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?
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.

-- 
Robin Hugh Johnson
Gentoo Linux: Developer, Trustee & Infrastructure Lead
E-Mail     : robbat2@gentoo.org
GnuPG FP   : 11AC BA4F 4778 E3F6 E4ED  F38E B27B 944E 3488 4E85

[-- Attachment #2: Type: application/pgp-signature, Size: 330 bytes --]

Re: [PATCH v2] Fix false positives in t3404 due to SHELL=/bin/false

[Matthieu Moy]

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

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

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

> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -71,8 +71,9 @@ test_expect_success 'setup' '
>  # "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
> -# in tests.
> +# in tests. It must be exported for it to take effect where needed.
>  SHELL=
> +export 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
http://www-verimag.imag.fr/~moy/

Re: [PATCH v2] Fix false positives in t3404 due to SHELL=/bin/false

[Jonathan Nieder]

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

>> --- a/t/t3404-rebase-interactive.sh
>> +++ b/t/t3404-rebase-interactive.sh
>> @@ -71,8 +71,9 @@ test_expect_success 'setup' '
>>  # "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
>> -# in tests.
>> +# in tests. It must be exported for it to take effect where needed.
>>  SHELL=
>> +export 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)

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?

Re: [PATCH v2] Fix false positives in t3404 due to SHELL=/bin/false

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Matthieu Moy wrote:
>> "Vallon, Justin" <Justin.Vallon@deshaw.com> writes:
>
>>> --- a/t/t3404-rebase-interactive.sh
>>> +++ b/t/t3404-rebase-interactive.sh
>>> @@ -71,8 +71,9 @@ test_expect_success 'setup' '
>>>  # "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
>>> -# in tests.
>>> +# in tests. It must be exported for it to take effect where needed.
>>>  SHELL=
>>> +export 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)
>
> 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?

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.

RE: [PATCH v2] Fix false positives in t3404 due to SHELL=/bin/false

[Vallon, Justin]

>-----Original Message-----
>From: Junio C Hamano [mailto:gitster@pobox.com]
>Sent: Tuesday, January 04, 2011 6:39 PM
>To: Jonathan Nieder
>Cc: Matthieu Moy; Vallon, Justin; Robin H. Johnson; git@vger.kernel.org
>Subject: Re: [PATCH v2] Fix false positives in t3404 due to
>SHELL=/bin/false
>
>Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> Matthieu Moy wrote:
>>>
>>> (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)
>>
>> 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?
>
>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").

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).

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

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.

-- 
-Justin

Re: [PATCH v2] Fix false positives in t3404 due to SHELL=/bin/false

[Junio C Hamano]

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

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

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).