[PATCH] Fix testcase failure when extended attributes are in use

[PATCH] Fix testcase failure when extended attributes are in use

[Deskin Miller]

06cbe855 (Make core.sharedRepository more generic, 2008-04-16) made
several testcases in t1301-shared-repo.sh which fail if on a system
which creates files with extended attributes (e.g. SELinux), since ls
appends a '+' sign to the permission set in such cases.  This fixes the
testcase to strip any such sign prior to verifying the permission set.

Signed-off-by: Deskin Miller <deskinm@umich.edu>
---
Shawn, I read an email that said you'd maintain until Sunday the 12th, so I'm
sending this to you; if you want to punt to Junio, feel free.

Deskin Miller

 t/t1301-shared-repo.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
index dc85e8b..b244f3e 100755
--- a/t/t1301-shared-repo.sh
+++ b/t/t1301-shared-repo.sh
@@ -83,7 +83,7 @@ do
 		rm -f .git/info/refs &&
 		git update-server-info &&
 		actual="$(ls -l .git/info/refs)" &&
-		actual=${actual%% *} &&
+		actual=$(echo "$actual" | sed -e "s/[+]\? .*$//") &&
 		test "x$actual" = "x-$y" || {
 			ls -lt .git/info
 			false
@@ -96,7 +96,7 @@ do
 		rm -f .git/info/refs &&
 		git update-server-info &&
 		actual="$(ls -l .git/info/refs)" &&
-		actual=${actual%% *} &&
+		actual=$(echo "$actual" | sed -e "s/[+]\? .*$//") &&
 		test "x$actual" = "x-$x" || {
 			ls -lt .git/info
 			false
-- 
1.6.0.2.307.gc427

[PATCH v2] Fix testcase failure when extended attributes are in use

[Deskin Miller]

06cbe855 (Make core.sharedRepository more generic, 2008-04-16) made
several testcases in t1301-shared-repo.sh which fail if on a system
which creates files with extended attributes (e.g. SELinux), since ls
appends a '+' sign to the permission set in such cases.  This fixes the
testcase to strip any such sign prior to verifying the permission set.

Signed-off-by: Deskin Miller <deskinm@umich.edu>
---
...and then I discovered Documentation/CodingGuidelines: no ? in regular
expressions.  I really need to learn these shell variable-manipulation
builtins.

Apologies if for some reason the previous version got used, I haven't seen it
anywhere, however.

Deskin Miller

 t/t1301-shared-repo.sh |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
index dc85e8b..4d2db62 100755
--- a/t/t1301-shared-repo.sh
+++ b/t/t1301-shared-repo.sh
@@ -84,6 +84,7 @@ do
 		git update-server-info &&
 		actual="$(ls -l .git/info/refs)" &&
 		actual=${actual%% *} &&
+		actual=${actual%+} &&
 		test "x$actual" = "x-$y" || {
 			ls -lt .git/info
 			false
@@ -97,6 +98,7 @@ do
 		git update-server-info &&
 		actual="$(ls -l .git/info/refs)" &&
 		actual=${actual%% *} &&
+		actual=${actual%+} &&
 		test "x$actual" = "x-$x" || {
 			ls -lt .git/info
 			false
-- 
1.6.0.2.514.g23abd3

Re: [PATCH v2] Fix testcase failure when extended attributes are in use

[Junio C Hamano]

With 8ed0a74 (t1301-shared-repo.sh: don't let a default ACL interfere with
the test, 2008-10-16) applied is this still needed, or can I drop it from
my review box?

Re: [PATCH v2] Fix testcase failure when extended attributes are in use

[Deskin Miller]

On Fri, Oct 17, 2008 at 04:58:16PM -0700, Junio C Hamano wrote:
> With 8ed0a74 (t1301-shared-repo.sh: don't let a default ACL interfere with
> the test, 2008-10-16) applied is this still needed, or can I drop it from
> my review box?

Apologies for a tardy response, was out of town and away from keyboard for a
day.
 
This patch is still needed, as my and Matt's patch are solving two different
issues with t1301.  As he pointed out in the thread regarding his patch, the
issue was that the testcase was intended to specifically test Git's interaction
with permissions set via the umask, but a default ACL on the 'trash directory'
could interfere with this, since they'd override the umask settings.  Remove
the ACL, no problem.

My patch, on the other hand, is to deal with 'ls' output in case a file has
certain filesystem extended attributes.  These could be e.g. POSIX ACLs, or a
SELinux security context, or perhaps others.  If such an extended attribute is
present, 'ls -l' will print permissions with a '+' appended, e.g.
-rw-r--r--+
Instead of
-rw-r--r--
However, t1301 reads permissions output by ls for several tests, and compares
them to string representations such as that above.  Without removing the '+',
if present, the strings will not match.  Furthermore, since this occurs for
other filesystem extended attributes, and not just ACLs, it is not possible to
simply strip all extended attributes from the file in question (with SELinux,
the kernel won't let you remove a file's security context anyway).

For what it's worth, I've experienced this failure on my Ubuntu 8.04 laptop
with SELinux permissive mode, so it's possible ls behaves slightly differently
on other systems; I've not been able to determine this one way or another.
However, I see no harm in accounting for this situation in the general case,
since the typical output of ls on other systems will not be affected nor
modified with this patch.

Hope that helps,
Deskin Miller

Re: [PATCH v2] Fix testcase failure when extended attributes are in use

[Junio C Hamano]

Deskin Miller <deskinm@umich.edu> writes:

> My patch, on the other hand, is to deal with 'ls' output in case a file has
> certain filesystem extended attributes.  These could be e.g. POSIX ACLs, or a
> SELinux security context, or perhaps others.  If such an extended attribute is
> present, 'ls -l' will print permissions with a '+' appended, e.g.
> -rw-r--r--+
> Instead of
> -rw-r--r--
> ...
> For what it's worth, I've experienced this failure on my Ubuntu 8.04 laptop
> with SELinux permissive mode, so it's possible ls behaves slightly differently
> on other systems; I've not been able to determine this one way or another.

Is there way to explicitly tell "ls -l" not to do this, I have to wonder?

POSIX.1 says that the file mode written under the -l option is
"%c%s%s%s%c" (where the first %c is for type, three %s are for owner,
group and other perm, and the last %c is "optional alternate access method
flag").  If there is no alternate or additional access control method
associated with the file, the "optional alternate access method flag"
would be a single SP, otherwise it would be a printable character.

If we drop the default ACL from the trash directory like Matt's patch
does, does a file created in there (i.e. the ones we check with /bin/ls)
still have "alternate or additional access control method associated with"
it?

Somehow it feels wrong that you need your patch, but if you do, stripping
only the trailing '+' as your patch does not look sufficient, either.
Shouldn't we be stripping the last letter if the length of actual is
longer than strlen("-rwxrwxrwx"), as any printable can come there?

 t/t1301-shared-repo.sh |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git c/t/t1301-shared-repo.sh i/t/t1301-shared-repo.sh
index 2275caa..653362b 100755
--- c/t/t1301-shared-repo.sh
+++ i/t/t1301-shared-repo.sh
@@ -20,6 +20,10 @@ test_expect_success 'shared = 0400 (faulty permission u-w)' '
 	test $ret != "0"
 '
 
+modebits () {
+	ls -l "$1" | sed -e 's|^\(..........\).*|\1|'
+}
+
 for u in 002 022
 do
 	test_expect_success "shared=1 does not clear bits preset by umask $u" '
@@ -85,8 +89,7 @@ do
 
 		rm -f .git/info/refs &&
 		git update-server-info &&
-		actual="$(ls -l .git/info/refs)" &&
-		actual=${actual%% *} &&
+		actual="$(modebits .git/info/refs)" &&
 		test "x$actual" = "x-$y" || {
 			ls -lt .git/info
 			false
@@ -98,8 +101,7 @@ do
 
 		rm -f .git/info/refs &&
 		git update-server-info &&
-		actual="$(ls -l .git/info/refs)" &&
-		actual=${actual%% *} &&
+		actual="$(modebits .git/info/refs)" &&
 		test "x$actual" = "x-$x" || {
 			ls -lt .git/info
 			false

Re: [PATCH v2] Fix testcase failure when extended attributes are in use

[Deskin Miller]

On Sun, Oct 19, 2008 at 12:59:06PM -0700, Junio C Hamano wrote:
> Deskin Miller <deskinm@umich.edu> writes:
> 
> > My patch, on the other hand, is to deal with 'ls' output in case a file has
> > certain filesystem extended attributes.  These could be e.g. POSIX ACLs, or a
> > SELinux security context, or perhaps others.  If such an extended attribute is
> > present, 'ls -l' will print permissions with a '+' appended, e.g.
> > -rw-r--r--+
> > Instead of
> > -rw-r--r--
> > ...
> > For what it's worth, I've experienced this failure on my Ubuntu 8.04 laptop
> > with SELinux permissive mode, so it's possible ls behaves slightly differently
> > on other systems; I've not been able to determine this one way or another.
> 
> Is there way to explicitly tell "ls -l" not to do this, I have to wonder?

I looked through all the options to ls several times, and there wasn't anything
which stood out to me.  The output format of ls is rather rigid.
 
> POSIX.1 says that the file mode written under the -l option is
> "%c%s%s%s%c" (where the first %c is for type, three %s are for owner,
> group and other perm, and the last %c is "optional alternate access method
> flag").  If there is no alternate or additional access control method
> associated with the file, the "optional alternate access method flag"
> would be a single SP, otherwise it would be a printable character.

My fault for not reading up more carefully.  In that case, yes, I agree that we
should be able to deal with any such flag character, which my patch does not
do.
 
> If we drop the default ACL from the trash directory like Matt's patch
> does, does a file created in there (i.e. the ones we check with /bin/ls)
> still have "alternate or additional access control method associated with"
> it?

Yes.  SELinux, in particular, associates a 'security context' with *every*
file, which is used in SELinux access control checks.  This is stored as a
filesystem extended attribute in the 'security.selinux' namespace, and one will
exist from the moment the file is created.

One could dream up other access control methods, such as a kernel module which
would deny access to any files with the string 'frob' in the filename, and
suppose that ls could be extended to recognise the presence of this module and
alter its display accordingly.  There is clearly no way to ensure that no
'alternate or additional access control method' is associated with a file,
since the POSIX.1 language is sufficiently general to allow for almost
anything.

> Somehow it feels wrong that you need your patch, but if you do, stripping
> only the trailing '+' as your patch does not look sufficient, either.
> Shouldn't we be stripping the last letter if the length of actual is
> longer than strlen("-rwxrwxrwx"), as any printable can come there?

I hope I've made the case for the necessity of a patch (if nothing else, t1301
fails 10/16 tests on my system without some sort of patch), and I'll happily
agree that stripping any printable is a better choice.  Thanks for the
feedback; FWIW, your suggested patch here is

Tested-by: Deskin Miller <deskinm@umich.edu>

>  t/t1301-shared-repo.sh |   10 ++++++----
>  1 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git c/t/t1301-shared-repo.sh i/t/t1301-shared-repo.sh
> index 2275caa..653362b 100755
> --- c/t/t1301-shared-repo.sh
> +++ i/t/t1301-shared-repo.sh
> @@ -20,6 +20,10 @@ test_expect_success 'shared = 0400 (faulty permission u-w)' '
>  	test $ret != "0"
>  '
>  
> +modebits () {
> +	ls -l "$1" | sed -e 's|^\(..........\).*|\1|'
> +}
> +
>  for u in 002 022
>  do
>  	test_expect_success "shared=1 does not clear bits preset by umask $u" '
> @@ -85,8 +89,7 @@ do
>  
>  		rm -f .git/info/refs &&
>  		git update-server-info &&
> -		actual="$(ls -l .git/info/refs)" &&
> -		actual=${actual%% *} &&
> +		actual="$(modebits .git/info/refs)" &&
>  		test "x$actual" = "x-$y" || {
>  			ls -lt .git/info
>  			false
> @@ -98,8 +101,7 @@ do
>  
>  		rm -f .git/info/refs &&
>  		git update-server-info &&
> -		actual="$(ls -l .git/info/refs)" &&
> -		actual=${actual%% *} &&
> +		actual="$(modebits .git/info/refs)" &&
>  		test "x$actual" = "x-$x" || {
>  			ls -lt .git/info
>  			false

Re: [PATCH v2] Fix testcase failure when extended attributes are in use

[Junio C Hamano]

Deskin Miller <deskinm@umich.edu> writes:

> I hope I've made the case for the necessity of a patch (if nothing else, t1301
> fails 10/16 tests on my system without some sort of patch), and I'll happily
> agree that stripping any printable is a better choice.  Thanks for the
> feedback; FWIW, your suggested patch here is
>
> Tested-by: Deskin Miller <deskinm@umich.edu>

Thanks.  Applied to 'maint' and will appear in the next pushout (but not
tonight).