Subject: bug#5940 closed by Pádraig Brady <[email-masked]> (Re: bug#5940: [PATCH] cp: treat selinux warnings consistently)

From: [email-masked] (GNU bug Tracking System)
Date: Wed, 14 Apr 2010 00:00:04 +0000

This is an automatic notification regarding your bug report
which was filed against the coreutils package:

#5940: [PATCH] cp: treat selinux warnings consistently

It has been closed by Pádraig Brady <[email-masked]>.

Their explanation is attached below along with your original report.
If this explanation is unsatisfactory and you have not received a
better one in a separate message then please contact Pádraig Brady <[email-masked]> by
replying to this email.

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

Subject: Re: bug#5940: [PATCH] cp: treat selinux warnings consistently
From: Pádraig Brady <[email-masked]>
To: [email-masked]
Date: Wed, 14 Apr 2010 00:58:38 +0100
List-Id: <debbugs-submit.debbugs.gnu.org>

On 13/04/10 13:14, Pádraig Brady wrote:
> I noticed some inconsistencies in how ENOTSUP errors were handled by cp.
> 
> The attached should fix it up.

I've just pushed this.

cheers,
Pádraig

Subject: [PATCH] cp: treat selinux warnings consistently
From: Pádraig Brady <[email-masked]>
To: Report bugs to <[email-masked]>
Cc: Ondřej Vašík <[email-masked]>
Date: Tue, 13 Apr 2010 13:14:36 +0100
List-Id: <debbugs-submit.debbugs.gnu.org>

I noticed some inconsistencies in how ENOTSUP errors were handled by cp.

$ dd bs=1M count=10 if=/dev/zero of=t.vfat
$ mkfs.vfat t.vfat
$ mkdir m.vfat
$ sudo mount -o loop t.vfat m.vfat
$ ./src/cp --preserve=all a m.vfat/
./src/cp: setting attribute `user.foo' for `user.foo': Permission denied
./src/cp: setting attribute `user.foo2' for `user.foo2': Permission denied
./src/cp: setting attribute `security.capability' for `security.capability': Operation not permitted
./src/cp: preserving permissions for `m.vfat/a': Operation not supported
$ sudo ./src/cp --preserve=all a m.vfat/a
./src/cp: failed to set the security context of `m.vfat/a' to `system_u:object_r:user_home_t:s0': Operation not supported
./src/cp: failed to preserve ownership for `m.vfat/a': Operation not permitted

Notice how one gets "Operation not supported" when the target exists.
The attached should fix it up.

cheers,
Pádraig.
>From 584e38d8b3199924ce24f9fa075d27d0d82a2a8b Mon Sep 17 00:00:00 2001
From: =?utf-8?q?P=C3=A1draig=20Brady?= <[email-masked]>
Date: Tue, 13 Apr 2010 12:49:05 +0100
Subject: [PATCH] cp: treat selinux warnings consistently

* src/copy.c (copy_reg): Suppress SELinux ENOTSUP warnings consistently
between the destination being present or not.  Previously we did
not suppress ENOTSUP messages when the destination was present.
(copy_internal): Use the same ENOTSUP supression method as
copy_reg() even though the issue was not seen in this case.
* tests/cp/cp-a-selinux: Add a test case for the issue and
group the other test cases in the file more coherently.
* tests/cp/cp-mv-enotsup-xattr: Do the same check for xattr
warnings, even though they did not have the issue.
---
 src/copy.c                   |   23 ++++++++++--------
 tests/cp/cp-a-selinux        |   51 ++++++++++++++++++++++++-----------------
 tests/cp/cp-mv-enotsup-xattr |    7 +++++-
 3 files changed, 49 insertions(+), 32 deletions(-)

diff --git a/src/copy.c b/src/copy.c
index 3c32fa3..0fa148e 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -531,7 +531,8 @@ copy_reg (char const *src_name, char const *dst_name,
           security_context_t con = NULL;
           if (getfscreatecon (&con) < 0)
             {
-              if (!x->reduce_diagnostics || x->require_preserve_context)
+              if (x->require_preserve_context ||
+                  (!x->reduce_diagnostics && !errno_unsupported (errno)))
                 error (0, errno, _("failed to get file system create context"));
               if (x->require_preserve_context)
                 {
@@ -544,7 +545,8 @@ copy_reg (char const *src_name, char const *dst_name,
             {
               if (fsetfilecon (dest_desc, con) < 0)
                 {
-                  if (!x->reduce_diagnostics || x->require_preserve_context)
+                  if (x->require_preserve_context ||
+                      (!x->reduce_diagnostics && !errno_unsupported (errno)))
                     error (0, errno,
                            _("failed to set the security context of %s to %s"),
                            quote_n (0, dst_name), quote_n (1, con));
@@ -1825,7 +1827,8 @@ copy_internal (char const *src_name, char const *dst_name,
         {
           if (setfscreatecon (con) < 0)
             {
-              if (!x->reduce_diagnostics || x->require_preserve_context)
+              if (x->require_preserve_context ||
+                  (!x->reduce_diagnostics && !errno_unsupported (errno)))
                 error (0, errno,
                        _("failed to set default file creation context to %s"),
                        quote (con));
@@ -1839,15 +1842,15 @@ copy_internal (char const *src_name, char const *dst_name,
         }
       else
         {
-          if (!errno_unsupported (errno) || x->require_preserve_context)
+          if (x->require_preserve_context ||
+              (!x->reduce_diagnostics && !errno_unsupported (errno)))
             {
-              if (!x->reduce_diagnostics || x->require_preserve_context)
-                error (0, errno,
-                       _("failed to get security context of %s"),
-                       quote (src_name));
-              if (x->require_preserve_context)
-                return false;
+              error (0, errno,
+                     _("failed to get security context of %s"),
+                     quote (src_name));
             }
+          if (x->require_preserve_context)
+            return false;
         }
     }
 
diff --git a/tests/cp/cp-a-selinux b/tests/cp/cp-a-selinux
index 770dcc4..b65070a 100755
--- a/tests/cp/cp-a-selinux
+++ b/tests/cp/cp-a-selinux
@@ -60,51 +60,60 @@ test $skip = 1 \
 cd mnt                                       || framework_failure
 
 echo > f                                     || framework_failure
-echo > g                                     || framework_failure
-
 
+echo > g                                     || framework_failure
 # /bin/cp from coreutils-6.7-3.fc7 would fail this test by letting cp
 # succeed (giving no diagnostics), yet leaving the destination file empty.
 cp -a f g 2>err || fail=1
 test -s g       || fail=1     # The destination file must not be empty.
 test -s err     && fail=1     # There must be no stderr output.
 
-rm -f g err
+# =====================================================
+# Here, we expect cp to succeed and not warn with "Operation not supported"
+rm -f g
 echo > g
+cp --preserve=all f g 2>err || fail=1
+test -s g || fail=1
+grep "Operation not supported" err && fail=1
 
 # =====================================================
+# The same as above except destination does not exist
+rm -f g
+cp --preserve=all f g 2>err || fail=1
+test -s g || fail=1
+grep "Operation not supported" err && fail=1
+
+# An alternative to the following approach would be to run in a confined
+# domain (maybe creating/loading it) that lacks the required permissions
+# to the file type.
+# Note: this test could also be run by a regular (non-root) user in an
+# NFS mounted directory.  When doing that, I get this diagnostic:
+# cp: failed to set the security context of `g' to `system_u:object_r:nfs_t': \
+#   Operation not supported
+cat <<\EOF > exp || framework_failure=1
+cp: failed to set the security context of
+EOF
+
+rm -f g
+echo > g
+# =====================================================
 # Here, we expect cp to fail, because it cannot set the SELinux
 # security context through NFS or a mount with fixed context.
 cp --preserve=context f g 2> out && fail=1
-
 # Here, we *do* expect the destination to be empty.
 test -s g && fail=1
+sed "s/ .g' to .*//" out > k
+mv k out
+compare out exp || fail=1
 
 rm -f g
 echo > g
 # Check if -a option doesn't silence --preserve=context option diagnostics
 cp -a --preserve=context f g 2> out2 && fail=1
-
 # Here, we *do* expect the destination to be empty.
 test -s g && fail=1
-
-# An alternative to the current approach would be to run in a confined
-# domain (maybe creating/loading it) that lacks the required permissions
-# to the file type.
-# Note: this test could also be run by a regular (non-root) user in an
-# NFS mounted directory.  When doing that, I get this diagnostic:
-# cp: failed to set the security context of `g' to `system_u:object_r:nfs_t': \
-#   Operation not supported
-sed "s/ .g' to .*//" out > k
-mv k out
 sed "s/ .g' to .*//" out2 > k
 mv k out2
-
-cat <<\EOF > exp || fail=1
-cp: failed to set the security context of
-EOF
-
-compare out exp || fail=1
 compare out2 exp || fail=1
 
 Exit $fail
diff --git a/tests/cp/cp-mv-enotsup-xattr b/tests/cp/cp-mv-enotsup-xattr
index 0239abb..7e7b645 100755
--- a/tests/cp/cp-mv-enotsup-xattr
+++ b/tests/cp/cp-mv-enotsup-xattr
@@ -77,7 +77,12 @@ test -s err         && fail=1  # there must be no stderr output
 
 rm -f err noxattr/a
 
-# This should pass without diagnostics
+# This should pass without diagnostics (new file)
+cp --preserve=all xattr/a noxattr/ 2>err || fail=1
+test -s noxattr/a   || fail=1  # destination file must not be empty
+test -s err         && fail=1  # there must be no stderr output
+
+# This should pass without diagnostics (existing file)
 cp --preserve=all xattr/a noxattr/ 2>err || fail=1
 test -s noxattr/a   || fail=1  # destination file must not be empty
 test -s err         && fail=1  # there must be no stderr output
-- 
1.6.2.5