[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Jim Meyering
Subject: Re: cp(1) extended attributes bug + [PATCH]: fix the leak in copy_reg()
Date: Thu, 03 Sep 2009 08:51:24 +0200

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.,,


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".

reply via email to

[Prev in Thread] Current Thread [Next in Thread]