Pádraig Brady wrote: > Since we're only doing u+rw, and we've already stat'd it's > probably better to just (sb.mode & S_IWUSR) rather than access(...). > Also a couple of the if statements are indented too far.
Hopefully ok with the attached patch. > This should now be safer but as Jim says it > only effects file systems mounted user_xattr. > Perhaps we should wait until coreutils-7.7 and > also feedback from libattr devs so as we can put > an accurate comment in the code. As libattr feedback is already here at http://lists.gnu.org/archive/html/bug-coreutils/2009-09/msg00166.html and it seems it is not a bug in libattr (just strange requirement by kernel), I modified the comment about workaround - as the culprit is probably in kernel. Greetings, Ondřej Vašík
From 500acf63e78cc6bdb4fdd8b84c1f5e2305b96fb1 Mon Sep 17 00:00:00 2001 From: =?utf-8?q?Ond=C5=99ej=20Va=C5=A1=C3=ADk?= <[email protected]> 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 | 6 ++++++ src/copy.c | 33 ++++++++++++++++++++++++++++----- tests/misc/xattr | 42 ++++++++++++++++++++++++++++-------------- 3 files changed, 62 insertions(+), 19 deletions(-) diff --git a/NEWS b/NEWS index 980fb54..0ba0d50 100644 --- a/NEWS +++ b/NEWS @@ -2,6 +2,12 @@ GNU coreutils NEWS -*- outline -*- * Noteworthy changes in release ?.? (????-??-??) [?] +** Bug fixes + + 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] + ** Changes in behavior id no longer prints SELinux " context=..." when the POSIXLY_CORRECT diff --git a/src/copy.c b/src/copy.c index f3ff5a2..0cb6094 100644 --- a/src/copy.c +++ b/src/copy.c @@ -834,6 +834,34 @@ copy_reg (char const *src_name, char const *dst_name, } } + /* To allow copying extended attributes on read-only files, change + destination file descriptor access rights to 0600 for a while + if required. + This workaround is required as full permission check is done + by xattr_permission in fs/xattr.c of the kernel tree. */ + if (x->preserve_xattr) + { + bool access_unchanged = true; + + if (geteuid () != 0 && !(sb.st_mode & S_IWUSR) && + (access_unchanged = 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; + + if (!access_unchanged) + fchmod_or_lchmod (dest_desc, dst_name, + dst_mode & ~omitted_permissions); + } + + if (x->preserve_ownership && ! SAME_OWNER_AND_GROUP (*src_sb, sb)) { switch (set_owner (x, dst_name, dest_desc, src_sb, *new_dst, &sb)) @@ -850,11 +878,6 @@ 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; - if (x->preserve_mode || x->move_mode) { if (copy_acl (src_name, source_desc, dst_name, dest_desc, src_mode) != 0 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
