> On 2023-02-13, at 4:05 PM, Pádraig Brady wrote: > > On 12/02/2023 20:36, Pádraig Brady wrote: >> On 12/02/2023 14:15, Pádraig Brady wrote: >>> On 11/02/2023 21:53, Paul Eggert wrote: >>>> On 2023-02-10 17:21, George Valkov wrote: >>>> >>>>> remember to trace all code paths errors. >>>> >>>> Yes, I've been doing that. However, I don't use macOS and the two of us >>>> are debugging remotely where I write the code and you debug it but >>>> neither of us know how macOS works. Of course it would be much more >>>> efficient if we weren't operating with these obstacles, but here we are. >>>> >>>> Because the macOS behavior is poorly documented and we lack the source, >>>> if we can't figure this out fairly quickly coreutils should simply stop >>>> using fclonefileat because it's unreliable for users and a time sink for >>>> us. I'm hoping, though, we can get something working with another round >>>> or two. >>>> >>>> >>>>> With CLONE_ACL, I get 22 Invalid argument. >>>> >>>> OK, this means that although Apple has documented CLONE_ACL and it's in >>>> your include files, it doesn't work in your version of macOS, because >>>> either it's not supported by the kernel, or by the filesystem code, or >>>> by the particular filesystem instance, or for some other reason. EINVAL >>>> hints that it's the kernel but that's by no means certain. >>>> >>>> One possible way to defend against this macOS misbehavior is for cp to >>>> try to use CLONE_ACL, and if that fails with errno == EINVAL try again >>>> without CLONE_ACL. I attempted to implement this in the attached patch; >>>> please give it a try. >>> >>> I tested this on macOS 13.2 and the first fclonefileat(..., CLONE_ACL) succeeds. >>> I tested with various umask, --no-preserve=mode, -p, ... combinations >>> and didn't find any inconsistencies in permissions with --reflink=never copies. >>> Actually scratch that. There was an older xcode installed so CLONE_ACL was 0. >>> I've not got access at the moment, so will need to retest later with newer xcode. >> Good news is that with newer xcode supporting macOS 13.1 >> CLONE_ACL is set and that all works as expected, with the >> first fclonefileat() succeeding. >> I checked with actual ACLs (set with `chmod +a "user:$USER allow readattr" file`), >> and they were copied or dropped as expected. Note ls -l on macOS will list >> ACLs with a "+" rather than a "@", as was mentioned elsewhere in this thread. >>> One divergence with fclonefileat() is that it writes through a dangling symlink, >>> where a standard --reflink=never copy will fail. >>> I.e. `make TESTS=tests/cp/thru-dangling.sh SUBDIRS=. check` fails. >>> Now there is nothing wrong with writing through the dangling symlink, >>> and in fact standard cp will also with POSIXLY_CORRECT set in the environment. >>> So I may just need need to adjust the test to work / skip in this case. >> Another divergence I noticed is wrt umask and setuid handling. >> The following transcript shows that with fclonefileat() we do _not_ >> preserve setuid when it is preserved without. >> Also with fclonefileat() the umask is not honored, >> while it is honored without. >> # id -u >> 0 >> # echo create setuid file >> # rm src/cp.s; src/cp src/cp src/cp.s; chmod u+s src/cp.s >> # umask; for reflink in always never; do >> > for preserve in mode timestamps; do >> > rm -f src/cp.s.$reflink.$preserve >> > src/cp --reflink=$reflink --preserve=$preserve src/cp.s src/cp.s.$reflink.$preserve >> > done >> > done >> > ls -le src/cp.s* >> 0022 >> -rwsrwxr-x 1 root staff 234032 12 Feb 20:21 src/cp.s >> -rwxrwxr-x 1 root staff 234032 12 Feb 20:21 src/cp.s.always.mode >> -rwxrwxr-x 1 root staff 234032 12 Feb 20:21 src/cp.s.always.timestamps >> -rwsrwxr-x 1 root staff 234032 12 Feb 20:21 src/cp.s.never.mode >> -rwxr-xr-x 1 root staff 234032 12 Feb 20:21 src/cp.s.never.timestamps > > This amendment seems to make the treatment of umask and setuid consistent > between using fclonefileat() and not. > > diff --git a/src/copy.c b/src/copy.c > index 782fab98d..8370f55bd 100644 > --- a/src/copy.c > +++ b/src/copy.c > @@ -1280,10 +1280,11 @@ copy_reg (char const *src_name, char const *dst_name, > return_val = false; > } > } > - mode_already_preserved = (fc_flags & CLONE_ACL) != 0; > + mode_already_preserved = (fc_flags & CLONE_ACL) != 0 > + && cloned_mode == src_mode; > dest_desc = -1; > omitted_permissions = dst_mode & ~cloned_mode; > - extra_permissions = 0; > + extra_permissions = dst_mode & ~ cached_umask (); > goto set_dest_mode; > } > else if (! handle_clone_fail (dst_dirfd, dst_relname, src_name, > > With this, all tests pass (apart from thru-dangling.sh as mentioned above > which I'll skip in another patch). > Paul I'm happy to merge this amendment, mention the improvement in NEWS, > and push in your name if you're OK with it. To summarise: 1. We apply my original patch to disable sparse-copy on macOS. Otherwise since fclonefileat is not used whenever we overwrite a file, corruption will still occur. An updated copy of the patch is attached below: By doing this we fail the following test so you have to disable it on macOS: FAIL: tests/cp/sparse-perf.sh