patchFix testcase failure when extended attributes are in use

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

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

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?

On Fri, Oct 17, 2008 at 04:58:16PM -0700, Junio C Hamano wrote:

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

Deskin Miller <deskinm@umich.edu> writes:

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

On Sun, Oct 19, 2008 at 12:59:06PM -0700, Junio C Hamano wrote:

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.
 

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.
 

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.

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>

Deskin Miller <deskinm@umich.edu> writes:

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