Subject: bug#8359: closed (Re: bug#8359: [PATCH] Unit tests: Properly detect whether SELinux is enabled or not.)

From: [email-masked] (GNU bug Tracking System)
Date: Mon, 28 Mar 2011 10:07:02 +0000

Your bug report

#8359: [PATCH] Unit tests: Properly detect whether SELinux is enabled or not.

which was filed against the coreutils package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
8359: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=8359
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: bug#8359: [PATCH] Unit tests: Properly detect whether SELinux is enabled or not.
From: Jim Meyering <[email-masked]>
To: Mathieu Bridon <[email-masked]>
Cc: [email-masked]
Date: Mon, 28 Mar 2011 12:06:15 +0200
List-Id: <debbugs-submit.debbugs.gnu.org>

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 <[email-masked]>
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

Subject: [PATCH] Unit tests: Properly detect whether SELinux is enabled or not.
From: Mathieu Bridon <[email-masked]>
To: [email-masked]
Date: Mon, 28 Mar 2011 11:45:43 +0800
List-Id: <debbugs-submit.debbugs.gnu.org>

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