Re: [PATCH] cp, mv: do preserve extended attributes even for read-only source files

[Pádraig Brady]

Ondřej Vašík wrote:
> Pádraig Brady wrote:
>> Since we're only doing u+rw, and we've already stat'd it's
>> probably better to just (sb.mode & S_IWUSR) rather than access(...).
>> Also a couple of the  if statements are indented too far.
> 
> Hopefully ok with the attached patch.
> 
>> This should now be safer but as Jim says it
>> only effects file systems mounted user_xattr.
>> Perhaps we should wait until coreutils-7.7 and
>> also feedback from libattr devs so as we can put
>> an accurate comment in the code.
> 
> As libattr feedback is already here at
> http://lists.gnu.org/archive/html/bug-coreutils/2009-09/msg00166.html
> and it seems it is not a bug in libattr (just strange requirement by
> kernel), I modified the comment about workaround - as the culprit is
> probably in kernel. 

Yes I agree that the change is required.
I've tweaked it so that the geteuid() syscall is only called
if readonly files. Also I removed the error message on chmod failure
as the user will still get an error message _if_ the copy_xattr fails.
Also I ran it through indent and did s/write access rights/write access/.
The crux of the patch is now:

  if (x->preserve_xattr)
    {
      bool access_changed = true;

      if (!(sb.st_mode & S_IWUSR) && geteuid() != 0)
        access_changed = fchmod_or_lchmod (dest_desc, dst_name, 0600) == 0;

      if (!copy_attr_by_fd (src_name, source_desc, dst_name, dest_desc, x)
          && x->require_preserve_xattr)
        return_val = false;

      if (access_changed)
        fchmod_or_lchmod (dest_desc, dst_name, dst_mode & ~omitted_permissions);
    }

I'll push it soon if there are no objections.

cheers,
Pádraig.