GNU bug report logs - #8359
[PATCH] Unit tests: Properly detect whether SELinux is enabled or not.

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 8359 in the body.
You can then email your comments to 8359 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Mathieu Bridon <bochecha <at> fedoraproject.org>
To: bug-coreutils <at> gnu.org
Subject: [PATCH] Unit tests: Properly detect whether SELinux is enabled or not.
Date: Mon, 28 Mar 2011 11:45:43 +0800

The unit tests would run ls to see if the files had an SELinux
context, and would assume SELinux is enabled if they did.

This is not ideal, and can cause test failures in some environments:
    https://bugzilla.redhat.com/show_bug.cgi?id=573111#c26

The problem in the case of the above bug report is that the host has
SELinux enabled (and thus files have a context) but the chroot (mock)
fakes SELinux being disabled. Unfortunately, it can't remove the
context, which makes ls thinks that SELinux is enabled.

Later on, when running certain unit tests (e.g id-context), they fail
as they use the libselinux which (correctly) thinks SELinux is disabled
(and in the case of id-context, id will not return the context of the
user).

A better way to test if SELinux is enabled is to search for the SELinux
filesystem (see the above bug report). This is what this commit does.
---
 tests/init.cfg |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/tests/init.cfg b/tests/init.cfg
index f74d50c..ca92297 100644
--- a/tests/init.cfg
+++ b/tests/init.cfg
@@ -216,12 +216,9 @@ skip_if_()

 require_selinux_()
 {
-  case `ls -Zd .` in
-    '? .'|'unlabeled .')
-      skip_test_ "this system (or maybe just" \
-        "the current file system) lacks SELinux support"
-    ;;
-  esac
+  grep selinux /proc/filesystems > /dev/null || \
+    skip_test_ "this system (or maybe just" \
+      "the current file system) lacks SELinux support"
 }

 very_expensive_()
-- 
1.7.4

Message #8 received at 8359 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Mathieu Bridon <bochecha <at> fedoraproject.org>
Cc: 8359 <at> debbugs.gnu.org
Subject: Re: bug#8359: [PATCH] Unit tests: Properly detect whether SELinux is
	enabled or not.
Date: Mon, 28 Mar 2011 09:54:19 +0200

Mathieu Bridon wrote:
> The unit tests would run ls to see if the files had an SELinux
> context, and would assume SELinux is enabled if they did.
>
> This is not ideal, and can cause test failures in some environments:
>     https://bugzilla.redhat.com/show_bug.cgi?id=573111#c26
>
> The problem in the case of the above bug report is that the host has
> SELinux enabled (and thus files have a context) but the chroot (mock)
> fakes SELinux being disabled. Unfortunately, it can't remove the
> context, which makes ls thinks that SELinux is enabled.
>
> Later on, when running certain unit tests (e.g id-context), they fail
> as they use the libselinux which (correctly) thinks SELinux is disabled
> (and in the case of id-context, id will not return the context of the
> user).
>
> A better way to test if SELinux is enabled is to search for the SELinux
> filesystem (see the above bug report). This is what this commit does.

Thank you for the diagnosis and patch.
However, I can't use that as-is, since removing the existing test would
mistakenly enable guaranteed-to-fail tests that are run from a file system
that does not support SELinux on a system for which it is enabled.

> diff --git a/tests/init.cfg b/tests/init.cfg
> index f74d50c..ca92297 100644
> --- a/tests/init.cfg
> +++ b/tests/init.cfg
> @@ -216,12 +216,9 @@ skip_if_()
>
>  require_selinux_()
>  {
> -  case `ls -Zd .` in
> -    '? .'|'unlabeled .')
> -      skip_test_ "this system (or maybe just" \
> -        "the current file system) lacks SELinux support"
> -    ;;
> -  esac
> +  grep selinux /proc/filesystems > /dev/null || \
> +    skip_test_ "this system (or maybe just" \
> +      "the current file system) lacks SELinux support"
>  }

I've adjusted it to address the above.
Also, I've tightened the regexp slightly, just in case,
and made the diagnostic more precise.
I've also rewritten the commit log.

Hmm... actually, I now have mixed feelings about this change.
Having SELinux enabled for id --context is conceptually a very
different thing from having an SELinux-enabled file system.
Now, I'm thinking that your new condition should guard only the id-context
test, rather than causing us to skip all FS-context-requiring tests.
In your environment, does any test other than id-context fail without
this patch?

From 1ff10c3073e2c20c9a7a9ff0e2cc93a3e16b41bd Mon Sep 17 00:00:00 2001
From: Mathieu Bridon <bochecha <at> fedoraproject.org>
Date: Mon, 28 Mar 2011 09:39:53 +0200
Subject: [PATCH] tests: avoid unwarranted failure in mock-simulated
 non-SELinux env.

* tests/init.cfg (require_selinux_): Skip the test also when
/proc/filesystems does not list selinuxfs.
Add comments.
Based on the patch by Mathieu Bridon in http://debbugs.gnu.org/8359.
More discussion in http://bugzilla.redhat.com/573111
---
 tests/init.cfg |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/tests/init.cfg b/tests/init.cfg
index f74d50c..0711455 100644
--- a/tests/init.cfg
+++ b/tests/init.cfg
@@ -216,6 +216,13 @@ skip_if_()

 require_selinux_()
 {
+  # When in a chroot of an SELinux-enabled system, but with a mock-simulated
+  # SELinux-*disabled* system, recognize that SELinux is disabled system wide:
+  grep 'selinuxfs$' /proc/filesystems > /dev/null \
+    || skip_test_ "this system lacks SELinux support"
+
+  # Independent of whether SELinux is enabled system-wide,
+  # the current file system may lack SELinux support.
   case `ls -Zd .` in
     '? .'|'unlabeled .')
       skip_test_ "this system (or maybe just" \
--
1.7.4.1.688.g95e3e

Message #11 received at 8359 <at> debbugs.gnu.org (full text, mbox):

From: Mathieu Bridon <bochecha <at> fedoraproject.org>
To: Jim Meyering <jim <at> meyering.net>
Cc: 8359 <at> debbugs.gnu.org
Subject: Re: bug#8359: [PATCH] Unit tests: Properly detect whether SELinux
	is enabled or not.
Date: Mon, 28 Mar 2011 16:37:44 +0800

On Mon, 2011-03-28 at 09:54 +0200, Jim Meyering wrote:
> Mathieu Bridon wrote:
[... snip ...]
> > A better way to test if SELinux is enabled is to search for the SELinux
> > filesystem (see the above bug report). This is what this commit does.
> 
> Thank you for the diagnosis and patch.
> However, I can't use that as-is, since removing the existing test would
> mistakenly enable guaranteed-to-fail tests that are run from a file system
> that does not support SELinux on a system for which it is enabled.

Right, I didn't think about this case. :-/

> Hmm... actually, I now have mixed feelings about this change.
> Having SELinux enabled for id --context is conceptually a very
> different thing from having an SELinux-enabled file system.
> Now, I'm thinking that your new condition should guard only the id-context
> test, rather than causing us to skip all FS-context-requiring tests.
> In your environment, does any test other than id-context fail without
> this patch?

Yes, 3 tests are failing:
 - misc/id-context
 - id/no-context
 - install/install-C-selinux

The three are skipped (which is expected) after applying the patch I
submitted. I didn't try with your version of the patch, but looking at
it I think it's safe to assume they would be skipped as well.


-- 
Mathieu

Message #16 received at 8359-done <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Mathieu Bridon <bochecha <at> fedoraproject.org>
Cc: 8359-done <at> debbugs.gnu.org
Subject: Re: bug#8359: [PATCH] Unit tests: Properly detect whether SELinux is
	enabled or not.
Date: Mon, 28 Mar 2011 12:06:15 +0200

Mathieu Bridon wrote:
> On Mon, 2011-03-28 at 09:54 +0200, Jim Meyering wrote:
>> Mathieu Bridon wrote:
> [... snip ...]
>> > A better way to test if SELinux is enabled is to search for the SELinux
>> > filesystem (see the above bug report). This is what this commit does.
>>
>> Thank you for the diagnosis and patch.
>> However, I can't use that as-is, since removing the existing test would
>> mistakenly enable guaranteed-to-fail tests that are run from a file system
>> that does not support SELinux on a system for which it is enabled.
>
> Right, I didn't think about this case. :-/
>
>> Hmm... actually, I now have mixed feelings about this change.
>> Having SELinux enabled for id --context is conceptually a very
>> different thing from having an SELinux-enabled file system.
>> Now, I'm thinking that your new condition should guard only the id-context
>> test, rather than causing us to skip all FS-context-requiring tests.
>> In your environment, does any test other than id-context fail without
>> this patch?
>
> Yes, 3 tests are failing:
>  - misc/id-context
>  - id/no-context
>  - install/install-C-selinux
>
> The three are skipped (which is expected) after applying the patch I
> submitted. I didn't try with your version of the patch, but looking at
> it I think it's safe to assume they would be skipped as well.

Thanks.

I've decided not to bother separating the tests after all:
less risk of introducing false-positive failures that way.
I had to make one more change to avoid the syntax-check
failure due to the new use of "filesystems":
(we prefer to spell it "file systems";  I've exempted
the entire init.cfg file rather than obfuscating it e.g., via
/proc/file''systems)

Here's what I've pushed.
I've closed the bug, but you're welcome to reopen if problems persist.

From 17a7e4592727b44d0a5550d1340e354786109af7 Mon Sep 17 00:00:00 2001
From: Mathieu Bridon <bochecha <at> fedoraproject.org>
Date: Mon, 28 Mar 2011 09:39:53 +0200
Subject: [PATCH] tests: avoid unwarranted failure in mock-simulated
 non-SELinux env.

* tests/init.cfg (require_selinux_): Skip the test also when
/proc/filesystems does not list selinuxfs.
Add comments.
* cfg.mk (exclude_file_name_regexp--sc_file_system): Exempt
tests/init.cfg, with its use of /proc/filesystems.
Based on the patch by Mathieu Bridon in http://debbugs.gnu.org/8359.
More discussion in http://bugzilla.redhat.com/573111
---
 cfg.mk         |    3 ++-
 tests/init.cfg |    7 +++++++
 2 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/cfg.mk b/cfg.mk
index fe2dd13..99a6e5e 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -345,7 +345,8 @@ exclude_file_name_regexp--sc_po_check = ^gl/
 exclude_file_name_regexp--sc_prohibit_always-defined_macros = ^src/seq\.c$$
 exclude_file_name_regexp--sc_prohibit_empty_lines_at_EOF = ^tests/pr/
 exclude_file_name_regexp--sc_program_name = ^(gl/.*|lib/euidaccess-stat)\.c$$
-exclude_file_name_regexp--sc_file_system = NEWS|^(src/df\.c|tests/misc/df-P)$$
+exclude_file_name_regexp--sc_file_system = \
+  NEWS|^(tests/init\.cfg|src/df\.c|tests/misc/df-P)$$
 exclude_file_name_regexp--sc_prohibit_always_true_header_tests = \
   ^m4/stat-prog\.m4$$
 exclude_file_name_regexp--sc_prohibit_fail_0 = \
diff --git a/tests/init.cfg b/tests/init.cfg
index f74d50c..0711455 100644
--- a/tests/init.cfg
+++ b/tests/init.cfg
@@ -216,6 +216,13 @@ skip_if_()

 require_selinux_()
 {
+  # When in a chroot of an SELinux-enabled system, but with a mock-simulated
+  # SELinux-*disabled* system, recognize that SELinux is disabled system wide:
+  grep 'selinuxfs$' /proc/filesystems > /dev/null \
+    || skip_test_ "this system lacks SELinux support"
+
+  # Independent of whether SELinux is enabled system-wide,
+  # the current file system may lack SELinux support.
   case `ls -Zd .` in
     '? .'|'unlabeled .')
       skip_test_ "this system (or maybe just" \
--
1.7.4.1.688.g95e3e

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.