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

From: Paul Eggert <[email-masked]>
Date: Sat, 11 Feb 2023 13:53:43 -0800

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.

> do we want to go the quick route of unlinking the destination and creating a fresh clone?

No, POSIX is clear that plain cp should not unlink and re-create the 
destination. Of course we can add a non-POSIX flag to do that, and we 
already have: cp --remove-destination.

From ea8904606e8e749e2f6d22c1abb0310fac3722f5 Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Fri, 10 Feb 2023 13:34:54 -0800
Subject: [PATCH] cp: improve use of fclonefileat

* src/copy.c (copy_reg): Use CLONE_ACL if available and working.
If the only problem with fclonefileat is that it would create the
file with the wrong timestamp, or with too few permissions,
do that but fix the timestamp and permissions afterwards,
rather than falling back on a traditional copy.
(CLONE_ACL) [HAVE_FCLONEFILEAT && !USE_XATTR]: Default to 0.
---
 src/copy.c | 52 ++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 44 insertions(+), 8 deletions(-)

diff --git a/src/copy.c b/src/copy.c
index dfbb557de..782fab98d 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -1146,6 +1146,7 @@ copy_reg (char const *src_name, char const *dst_name,
   union scan_inference scan_inference;
   bool return_val = true;
   bool data_copy_required = x->data_copy_required;
+  bool mode_already_preserved = false;
   bool preserve_xattr = USE_XATTR & x->preserve_xattr;
 
   source_desc = open (src_name,
@@ -1244,17 +1245,47 @@ copy_reg (char const *src_name, char const *dst_name,
   if (*new_dst)
     {
 #if HAVE_FCLONEFILEAT && !USE_XATTR
-/* CLONE_NOOWNERCOPY only available on macos >= 10.13.  */
+# ifndef CLONE_ACL
+#  define CLONE_ACL 0 /* Added in macOS 12.6.  */
+# endif
 # ifndef CLONE_NOOWNERCOPY
-#  define CLONE_NOOWNERCOPY 0
+#  define CLONE_NOOWNERCOPY 0 /* Added in macOS 10.13.  */
 # endif
-      int fc_flags = x->preserve_ownership ? 0 : CLONE_NOOWNERCOPY;
+      mode_t cloned_mode_bits = S_ISVTX | S_IRWXUGO;
+      mode_t cloned_mode = src_mode & cloned_mode_bits;
+      int fc_flags = ((x->preserve_mode ? CLONE_ACL : 0)
+                      | (x->preserve_ownership ? 0 : CLONE_NOOWNERCOPY));
       if (data_copy_required && x->reflink_mode
-          && x->preserve_mode && x->preserve_timestamps
+          && (x->preserve_mode || ! (cloned_mode & ~dst_mode))
           && (x->preserve_ownership || CLONE_NOOWNERCOPY))
         {
-          if (fclonefileat (source_desc, dst_dirfd, dst_relname, fc_flags) == 0)
-            goto close_src_desc;
+          int s = fclonefileat (source_desc, dst_dirfd, dst_relname, fc_flags);
+          if (s != 0 && (fc_flags & CLONE_ACL) != 0 && errno == EINVAL)
+            {
+              fc_flags &= ~CLONE_ACL;
+              s = fclonefileat (source_desc, dst_dirfd, dst_relname, fc_flags);
+            }
+          if (s == 0)
+            {
+              if (!x->preserve_timestamps)
+                {
+                  struct timespec timespec[2];
+                  timespec[0].tv_nsec = timespec[1].tv_nsec = UTIME_NOW;
+                  if (utimensat (dst_dirfd, dst_relname, timespec,
+                                 AT_SYMLINK_NOFOLLOW)
+                      != 0)
+                    {
+                      error (0, errno, _("updating times for %s"),
+                             quoteaf (dst_name));
+                      return_val = false;
+                    }
+                }
+              mode_already_preserved = (fc_flags & CLONE_ACL) != 0;
+              dest_desc = -1;
+              omitted_permissions = dst_mode & ~cloned_mode;
+              extra_permissions = 0;
+              goto set_dest_mode;
+            }
           else if (! handle_clone_fail (dst_dirfd, dst_relname, src_name,
                                         dst_name,
                                         -1, false /* We didn't create dst  */,
@@ -1485,9 +1516,14 @@ copy_reg (char const *src_name, char const *dst_name,
 
   set_author (dst_name, dest_desc, src_sb);
 
+#if HAVE_FCLONEFILEAT && !USE_XATTR
+set_dest_mode:
+#endif
   if (x->preserve_mode || x->move_mode)
     {
-      if (copy_acl (src_name, source_desc, dst_name, dest_desc, src_mode) != 0
+      if (!mode_already_preserved
+          && (copy_acl (src_name, source_desc, dst_name, dest_desc, src_mode)
+              != 0)
           && x->require_preserve)
         return_val = false;
     }
@@ -1517,7 +1553,7 @@ copy_reg (char const *src_name, char const *dst_name,
     }
 
 close_src_and_dst_desc:
-  if (close (dest_desc) < 0)
+  if (0 <= dest_desc && close (dest_desc) < 0)
     {
       error (0, errno, _("failed to close %s"), quoteaf (dst_name));
       return_val = false;
-- 
2.39.1