[Toybox] cp --preserve questions.

[Rob Landley]

Note to self, remember to hit "send".

On 12/28/2015 08:30 AM, José Bollo wrote:
> Le dimanche 27 décembre 2015 à 14:37 -0600, Rob Landley a écrit :
>> I recently went through Jose's cp --preserve patch from way back when, and hit:
>>
>> +  if ((listlen = flistxattr(fdin, NULL, 0)) > 0 && (list = malloc(listlen))) {
>> +    flistxattr(fdin, list, listlen);
>> +    for (name = list ; (name - list) < listlen ; name += strlen(name)+1) {
>> +      /* test if the xattribute have to be copied */
>> +      if ((TT.preserve & p_xattr)
>> +      || ((TT.preserve & p_mode) &&
>> +              !strncmp(name,"system.posix_acl_",sizeof("system.posix_acl_")-1))
>> +      || ((TT.preserve & p_context) &&
>> +              !strncmp(name,"security.",sizeof("security.")-1)))
>> +      {
>>
>> Questions:
>>
>> 1) Is the "security." prefix good enough for both smack and selinux?
> 
> Yes. I use "man 5 attr" to get details. See
> http://linux.die.net/man/5/attr

Oh good.

>> 2) You call flistxattr() to grab the length, then call flistxattr() again
>> to fetch the data. Anybody else spot the race condition of an attribute
>> being added between the two calls? If you increase the size of an existing
>> xattr, will it be properly null terminate the last entry?
> 
> Right. the status of the second call must be checked. It returns -1 if
> there is not enough space and errno is set to ERANGE.

Sigh. This is seriously not an API the kernel guys have given a lot of
TLC in the past 20 years.

>> 3) p_mode means you're preserving the suid/sgid/sticky bits. I.E. you're
>> calling fchown() on the thing to feed it bits you can't set in open().
>> Why is there now a SECOND meaning, having to do with xattrs, meaning
>> simply doing cp -p is now doing all these extra xattr system calls
>> to check for "system.posix_acl_"?
> 
> IIRC, I studied the cp command coming from coreutils to produce that
> behaviour. A quick check here
> http://lingrok.org/xref/coreutils/src/copy.c#1360 should be a good entry
> point. But it becomes far in the past...

Ew, gnu source. (I try not to look at it.)

That said, remember that cpio "force writeable" thing that I put in and
then took out again when Rich Felker convinced me it wasn't needed?

  /* To allow copying xattrs on read-only files, temporarily chmod u+rw.
     This workaround is required as an inode permission check is done
     by xattr_permission() in fs/xattr.c of the GNU/Linux kernel tree.
  */

Sigh.

>> 4) It's only copying xattrs for files. Not for directories or anything
>> else. Is this normal? (It's doing it because there's no openat() versions
>> of these commands, see "nobody's updated this API since the 90's" rant
>> earlier...)
> 
> That is maybe the same reason: copying regular files
> http://lingrok.org/xref/coreutils/src/copy.c#1008 copies more things
> than copying directories
> http://lingrok.org/xref/coreutils/src/copy.c#1008
> But I am not sure. Maybe is it an error.

Ow, what a mess.

But "this never worked for anybody" at least means it's not a regression.

>> Anyway, I've done a first pass. Haven't got an environment set up to test
>> it, but it compiled and "scripts/test.sh cp" didn't regress.
> 
> Great.
> I heard that you had bad weather. Hopefully you're alright.

Dallas, a 4 hour drive north of here, had really bad weather. We just
had a thunderstorm an some hail as the temperature dropped 40 degrees
(farenheit).

> Regards
> José

Rob