Re: [PATCH] cp, mv: do preserve extended attributes even for read-only source files

[Jim Meyering]

Pádraig Brady wrote:
> Ondřej Vašík wrote:
>> As reported in
>> http://lists.gnu.org/archive/html/bug-coreutils/2009-08/msg00342.html by
>> Ernest N. Mamikonyan, cp/mv fails to preserve extended attributes for
>> read-only source files.
>> Following patch fixes the issue for me, although maybe it's not perfect
>> solution. But I don't know about better one at the moment.
>> Test included...
>
> Thanks for that, and especially the test.
> It looks to me that the cause of this is actually a bug
> in fsetxattr() (called by attr_copy_fd) as it's being
> passed a writable fd, but still giving permission denied?
>
> rsync does copy the xattrs as it uses a different approach,
> which you can see comparing the strace output of cp and
> rsync below:

Hi Ondřej,

Thanks for working on that.
Note that your patch relaxes permissions
on the destination and does not restore them.

If you continue to work on this, please use the adjusted patch below.
It makes the test script detect that failure, and also removes most
of the redirections to /dev/null.  Now that nearly all test-related
output is directed to a log file, there's no point in redirecting small
outputs like that, and seeing them in the log can even make it easier
to diagnose problems.

Since this problem affects only users of file systems
mounted with "user_xattr", I may defer the fix until coreutils-7.7.


>From 158ca2f14ff61d8984b1b13bbf76ae0ce12b83c1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ond=C5=99ej=20Va=C5=A1=C3=ADk?= <address@hidden>
Date: Thu, 3 Sep 2009 16:10:21 +0200
Subject: [PATCH] cp,mv: preserve extended attributes even for read-only source 
files

* src/copy.c (copy_reg): Set mode on file descriptor to 0600 for copying
extended attributes to prevent failures when source file doesn't have
write access rights.  Reported by Ernest N. Mamikonyan.
* tests/misc/xattr: Test that change.
* NEWS (Bug fixes): Mention it.
---
 NEWS             |    4 ++++
 src/copy.c       |   24 ++++++++++++++++++++----
 tests/misc/xattr |   42 ++++++++++++++++++++++++++++--------------
 3 files changed, 52 insertions(+), 18 deletions(-)

diff --git a/NEWS b/NEWS
index cb01227..843f76b 100644
--- a/NEWS
+++ b/NEWS
@@ -15,6 +15,10 @@ GNU coreutils NEWS                                    -*- 
outline -*-
   cp --preserve=xattr no longer leaks resources on each preservation failure.
   [bug introduced in coreutils-7.1]

+  cp --preserve=xattr and --archive now preserves extended attributes even
+  when the source file doesn't have write access rights
+  [bug introduced in coreutils-7.1]
+
   dd now returns non-zero status if it encountered a write error while
   printing a summary to stderr.
   [bug introduced in coreutils-6.11]
diff --git a/src/copy.c b/src/copy.c
index 178a640..b299313 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -850,10 +850,26 @@ copy_reg (char const *src_name, char const *dst_name,

   set_author (dst_name, dest_desc, src_sb);

-  if (x->preserve_xattr && ! copy_attr_by_fd (src_name, source_desc,
-                                              dst_name, dest_desc, x)
-      && x->require_preserve_xattr)
-    return_val = false;
+  /* to allow copying extended attributes on read-only files, change
+     destination file descriptor access rights to 0600 for a while. */
+  if (x->preserve_xattr)
+   {
+     if (! fchmod_or_lchmod (dest_desc, dst_name, 0600))
+       {
+         if (! copy_attr_by_fd (src_name, source_desc, dst_name, dest_desc, x)
+            && x->require_preserve_xattr)
+           return_val = false;
+       }
+     else
+       {
+          if (!x->reduce_diagnostics || x->require_preserve_xattr)
+            error (0, errno, _("can't write extended attributes to %s"),
+                   quote (dst_name));
+          if (x->require_preserve_xattr)
+            return_val = false;
+       }
+    }
+

   if (x->preserve_mode || x->move_mode)
     {
diff --git a/tests/misc/xattr b/tests/misc/xattr
index a27e1f6..5da8bf4 100755
--- a/tests/misc/xattr
+++ b/tests/misc/xattr
@@ -29,7 +29,7 @@ fi

 # Skip this test if cp was built without xattr support:
 touch src dest || framework_failure
-cp --preserve=xattr -n src dest 2>/dev/null \
+cp --preserve=xattr -n src dest \
   || skip_test_ "coreutils built without xattr support"

 # this code was taken from test mv/backup-is-src
@@ -46,13 +46,13 @@ xattr_pair="$xattr_name=\"$xattr_value\""
 # create new file and check its xattrs
 touch a || framework_failure
 getfattr -d a >out_a || skip_test_ "failed to get xattr of file"
-grep -F "$xattr_pair" out_a >/dev/null && framework_failure
+grep -F "$xattr_pair" out_a && framework_failure

 # try to set user xattr on file
 setfattr -n "$xattr_name" -v "$xattr_value" a >out_a \
   || skip_test_ "failed to set xattr of file"
 getfattr -d a >out_a || skip_test_ "failed to get xattr of file"
-grep -F "$xattr_pair" out_a >/dev/null \
+grep -F "$xattr_pair" out_a \
   || skip_test_ "failed to set xattr of file"

 fail=0
@@ -60,36 +60,50 @@ fail=0
 # cp should not preserve xattr by default
 cp a b || fail=1
 getfattr -d b >out_b || skip_test_ "failed to get xattr of file"
-grep -F "$xattr_pair" out_b >/dev/null && fail=1
+grep -F "$xattr_pair" out_b && fail=1

 # test if --preserve=xattr option works
 cp --preserve=xattr a b || fail=1
 getfattr -d b >out_b || skip_test_ "failed to get xattr of file"
-grep -F "$xattr_pair" out_b >/dev/null || fail=1
+grep -F "$xattr_pair" out_b || fail=1

 #test if --preserve=all option works
 cp --preserve=all a c || fail=1
 getfattr -d c >out_c || skip_test_ "failed to get xattr of file"
-grep -F "$xattr_pair" out_c >/dev/null || fail=1
+grep -F "$xattr_pair" out_c || fail=1

 #test if -a option works without any diagnostics
 cp -a a d 2>err && test -s err && fail=1
 getfattr -d d >out_d || skip_test_ "failed to get xattr of file"
-grep -F "$xattr_pair" out_d >/dev/null || fail=1
+grep -F "$xattr_pair" out_d || fail=1
+
+# Ensure that --preserve=xattr works even for an unwritable source file.
+chmod a-w a || framework_failure
+rm -f e
+cp --preserve=xattr a e || fail=1
+getfattr -d e >out_e || skip_test_ "failed to get xattr of file"
+grep -F  "$xattr_pair" out_e || fail=1
+
+# Ensure that permission bits are preserved, too.
+src_perm=$(stat --format=%a a)
+dst_perm=$(stat --format=%a e)
+test "$dst_perm" = "$src_perm" || fail=1
+
+chmod u+w a || framework_failure

 rm b || framework_failure

 # install should never preserve xattr
 ginstall a b || fail=1
 getfattr -d b >out_b || skip_test_ "failed to get xattr of file"
-grep -F "$xattr_pair" out_b >/dev/null && fail=1
+grep -F "$xattr_pair" out_b && fail=1

 # mv should preserve xattr when renaming within a file system.
 # This is implicitly done by rename () and doesn't need explicit
 # xattr support in mv.
 mv a b || fail=1
 getfattr -d b >out_b || skip_test_ "failed to get xattr of file"
-grep -F "$xattr_pair" out_b >/dev/null || cat >&2 <<EOF
+grep -F "$xattr_pair" out_b || cat >&2 <<EOF
 =================================================================
 $0: WARNING!!!
 rename () does not preserve extended attributes
@@ -99,18 +113,18 @@ EOF
 # try to set user xattr on file on other partition
 test_mv=1
 touch "$b_other" || framework_failure
-setfattr -n "$xattr_name" -v "$xattr_value" "$b_other" >out_a 2>/dev/null \
+setfattr -n "$xattr_name" -v "$xattr_value" "$b_other" >out_a \
   || test_mv=0
-getfattr -d "$b_other" >out_b 2>/dev/null || test_mv=0
-grep -F "$xattr_pair" out_b >/dev/null || test_mv=0
+getfattr -d "$b_other" >out_b || test_mv=0
+grep -F "$xattr_pair" out_b || test_mv=0
 rm -f "$b_other" || framework_failure

 if test $test_mv -eq 1; then
   # mv should preserve xattr when copying content from one partition to another
   mv b "$b_other" || fail=1
-  getfattr -d "$b_other" >out_b 2>/dev/null ||
+  getfattr -d "$b_other" >out_b ||
     skip_test_ "failed to get xattr of file"
-  grep -F "$xattr_pair" out_b >/dev/null || fail=1
+  grep -F "$xattr_pair" out_b || fail=1
 else
   cat >&2 <<EOF
 =================================================================
-- 
1.6.4.2.409.g85dc3