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.
