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

[Jim Meyering]

Pádraig Brady wrote:
> 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.

Thanks Ernest, Ondřej, and Pádraig!

Pádraig, you're welcome to push this fix.
However, please adjust the log to
s/exit/return/ or s/exit/return early/

Also, for each regression-fix like this, I find it worthwhile
to include a pointer in the log to the commit that introduced it.  E.g.,,

  http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commit;h=1762092901adf0404
  http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commit;h=a977dbbe78f4dcb64
  http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commit;h=493c48168490247c8

I've been including the coreutils version number, 8 bytes of the commit SHA1,
date, and "summary line", e.g., for the last one listed above,

    ...
    The bug was introduced in coreutils-7.0 via commit 0647f3eb, 2008-06-02,
    "accommodate older SELinux which lacks matchpathcon_init_prefix".