Ondřej Vašík wrote:
> Ernest N. Mamikonyan wrote:
> [returning mailing list to CC, please keep it there]
>
>> On Tue, 01 Sep 2009 02:16:28 -0400, Ondřej Vašík <ova...@redhat.com> wrote:
>>> Ernest N. Mamikonyan wrote:
>>>> Cp(1) doesn't correctly copy extended attributes for read-only files:
>>>>
>>>> touch foo
>>>> setfattr -n user.key -v value foo
>>>> chmod a-w foo
>>>> cp --preserve=xattr foo bar
>>>> cp: setting attribute `user.key' for `user.key': Permission denied
>>> This error message is not produced by coreutils sources, but by libattr
>>> - all messages about extended attributes are generated there. I'm not
>>> sure why they are trying to set attributes for source file - maybe they
>>> are not and access rights for destination file are more relevant.
>>> Strace/ltrace of the failure could be helpful as well.
>> The problem is quite simple! Cp(1) tries to change the xattrs of a file
>> with mode 0400 (see attached trace). Is opening the destination file with
>> initial an mode of 0600 a security (or some other) risk? I suppose that's
>> what needs to be done.
>
> Sorry, I got confused ... it's obvious - not sure about the risk, so not
> trying to fix this now. What do you think, Jim/Pádraig?
I haven't looked at it, but it seems like a bug from the description.
> Additionally it looks like there is a leak (about 36k per file for
> failing xattr preserve) in copy_reg() - as the return false; should be
> changed to return_val=false; - sending patch for this... but it's not
> fixing the reported issue.
Eek! Well spotted. That also leaks the file descriptors.
I've adjusted the logs slightly in the attached patch.
cheers,
Pádraig.
>From 7877731f6e7c59905355e71519bbee7d6eac5d32 Mon Sep 17 00:00:00 2001
From: =?utf-8?q?Ond=C5=99ej=20Va=C5=A1=C3=ADk?= <ova...@redhat.com>
Date: Wed, 2 Sep 2009 10:34:27 +0100
Subject: [PATCH] cp: don't leak resources for each xattr preservation failure
* src/copy.c (copy_reg): Don't exit from the function after an
unsuccessful and required preservation of extended attributes.
This resulted in leaking the copy buffer and file descriptors.
---
NEWS | 3 +++
src/copy.c | 2 +-
2 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/NEWS b/NEWS
index 50c40be..59270eb 100644
--- a/NEWS
+++ b/NEWS
@@ -12,6 +12,9 @@ GNU coreutils NEWS -*- outline -*-
cp --reflink --preserve now preserves attributes when cloning a file.
[bug introduced in coreutils-7.5]
+ cp --preserve=xattr no longer leaks resources on each preservation failure.
+ [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 d1e508d..e604ec5 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -853,7 +853,7 @@ copy_reg (char const *src_name, char const *dst_name,
if (x->preserve_xattr && ! copy_attr_by_fd (src_name, source_desc,
dst_name, dest_desc, x)
&& x->require_preserve_xattr)
- return false;
+ return_val = false;
if (x->preserve_mode || x->move_mode)
{
--
1.6.2.5
