Hi, Jim Meyering wrote: > 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? > > > 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.
Ah, I knew I forgot to do something :). Thanks for spotting this.
Restoring to dest_mode & ~omitted_permissions done in attached patch,
dropped redirections from the test as well. Additionally - I modified
the copy.c patch a bit - failure of mode change now doesn't mean that I
don't try to preserve extended attributes (as it still could pass).
Pádraig is right that it looks like some kind of bug in libattr and
fsetxattr() function, as the descriptor should be writable, anyway this
should workaround it - at least until they'll fix/change it or other way
of solution will be found.
Ok with passing to 7.7, although with such small impact and relatively
low danger, it could maybe included to 7.6 (if more snapshots will be
before real release).
Greetings,
Ondřej
From 5fca1fbaf2e7594496c854f4c3eef60bd3013697 Mon Sep 17 00:00:00 2001
From: =?utf-8?q?Ond=C5=99ej=20Va=C5=A1=C3=ADk?= <ova...@redhat.com>
Date: Thu, 3 Sep 2009 16:10:21 +0200
Subject: [PATCH] cp,mv: do 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 | 21 +++++++++++++++++----
tests/misc/xattr | 42 ++++++++++++++++++++++++++++--------------
3 files changed, 49 insertions(+), 18 deletions(-)
diff --git a/NEWS b/NEWS
index 59270eb..8c511c6 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 e604ec5..9506fb4 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -850,10 +850,23 @@ 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 (!x->reduce_diagnostics || x->require_preserve_xattr)
+ error (0, errno,
+ _("failed to set writable mode for %s, copying xattrs may fail"),
+ quote (dst_name));
+ }
+ if (! copy_attr_by_fd (src_name, source_desc, dst_name, dest_desc, x)
+ && x->require_preserve_xattr)
+ return_val = false;
+ fchmod_or_lchmod (dest_desc, dst_name, dst_mode & ~omitted_permissions);
+ }
+
if (x->preserve_mode || x->move_mode)
{
diff --git a/tests/misc/xattr b/tests/misc/xattr
index a27e1f6..404085f 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
+
+#test if --preserve=xattr works even for files without write rights
+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.5.6.1.156.ge903b
signature.asc
Description: Toto je digitálně podepsaná část zprávy
