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