Re: cp(1) extended attributes bug + [PATCH]: fix the leak in copy_reg()

[Pádraig Brady]

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 <address@hidden> 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?= <address@hidden>
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