Re: `cp -p` incorrectly sets g+s bit when fs supports acl

[Jim Meyering]

Mike Frysinger wrote:
> On Sunday 22 February 2009 03:34:24 Jim Meyering wrote:
>> Mike Frysinger wrote:
>> > On Saturday 23 February 2008 01:12:24 Mike Frysinger wrote:
>> >> On Saturday 23 February 2008, Paul Eggert wrote:
>> >> > Mike Frysinger <address@hidden> writes:
>> >> > > i'm using coreutils-6.10 with acl-2.2.47 on linux-2.6.24.  when
>> >> > > using ext2 with acl support enabled, `cp -p` on a directory who does
>> >> > > not have the g+s bit set but whose parent does have g+s set, the new
>> >> > > destination directory will have the g+s bit set.  if the filesystem
>> >> > > is remounted with acl support turned off, the g+s bit is (correctly)
>> >> > > not set on the new destination directory.
>> >> >
>> >> > Could you please strace the failing cp?  Thanks.
>> >>
>> >> here is the strace from a good and a bad run
>> >
>> > i just retested with latest software:
>> >  - coreutils-7.1
>> >  - acl-2.2.47
>> >  - linux-2.6.28
>>
>> Thanks for the follow-up.
>>
>> I agree that differences like this can be confusing,
>> and are best avoided, if at all possible.
>>
>> FYI, this appears to be due to different policies:
>> Coreutils (copy.c) lets the mkdir syscall decide how
>> to handle set-GID bits.  Here's the relevant comment:
>>
>>       if (new_dst || !S_ISDIR (dst_sb.st_mode))
>>      {
>>        /* POSIX says mkdir's behavior is implementation-defined when
>>           (src_mode & ~S_IRWXUGO) != 0.  However, common practice is
>>           to ask mkdir to copy all the CHMOD_MODE_BITS, letting mkdir
>>           decide what to do with S_ISUID | S_ISGID | S_ISVTX.  */
>>        if (mkdir (dst_name, dst_mode_bits & ~omitted_permissions) != 0)
>>
>> so on Linux-based systems, people are used to seeing the set-?ID bits
>> inherited.  And that's the behavior you get without ACL support.
>> However, once you add ACLs/XATTRs to the mix, you are subject to their
>> policy, and when you say you want to copy all ACLs from one file to
>> another, you can't really expect the code to skip those special bits.
>>
>> Eventually we might change GNU cp to manually preserve those
>> special bits, too, but first, we'll need to know how other
>> vendor cp programs work.
>
> is that actually necessary ?  POSIX seems to be clear on the behavior of -p:
> http://www.opengroup.org/onlinepubs/9699919799/utilities/cp.html
>
> -p
>     Duplicate the following characteristics of each source file in the
> corresponding destination file:
>        3.
>           The file permission bits and the S_ISUID and S_ISGID bits. Other,
> implementation-defined, bits may be duplicated as well. If this duplication
> fails for any reason, cp shall write a diagnostic message to standard error.
>
> while it does say "file", the context of this page indicates that "file" is
> not strictly a "file" in the common sense as a directory is defined as "a file
> with type directory".  so to me, this means that `cp` needs to preserve the
> S_IS{U,G}ID bits on directories regardless when -p is in play.
>
> or is there other context that i'm missing ?

That seems clear enough, so maybe this *is* a bug.  However,
I haven't had time to look closely, and probably won't until late next
week.  In the mean time, if someone can verify how any other vendor's cp
work, that might help -- you'll probably need root access.

Mike, do you (or anyone else) want to write the patch?
It'd be great if someone volunteered to write the test (probably
a root-only one) based on your reproducer.