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

[Jim Meyering]

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

Sounds good.  Thanks.