bug#61386: [PATCH] cp,mv,install: Disable sparse copy on macOS

[George Valkov]

> On 2023-02-13, at 4:05 PM, Pádraig Brady <P@draigBrady.com> 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

0001-cp-mv-install-Disable-sparse-copy-on-macOS.patch
Description: Binary data

2. Next we add support for fclonefileat using cp: improve use of fclonefileat
A few more tests fail.

3. We fix those tests using the patch from your last mail. Then we have to 
disable the following test on macOS:
FAIL: tests/cp/thru-dangling.sh

Final results: copying sparse-files on macOS no longer produces corrupted 
copies. Performance is greatly optimised by cloning instead of copying, where 
possible. The following two tests now fail on macOS and will be disabled:
FAIL: tests/cp/sparse-perf.sh
FAIL: tests/cp/thru-dangling.sh

Looks good to me. Yes, please add me to the commit history. Also feel free to 
add my web site if appropriate. Finally please let me know when the patches are 
pushed, so that I can update OpenWRT to use the latest version of coreutils.

Pádraig and Paul, thank you very much for your kind help! It was nice working 
with you and learning new experience. Cheers, mates!


Georgi Valkov
httpstorm.com
nano RTOS