Subject: Re: bug#63245: Potential regression: cp --preserve=mode or --preserve=all fails to copy files from subdirectory

From: Paul Eggert <[email-masked]>
Date: Fri, 5 May 2023 11:10:41 -0700

On 2023-05-03 09:39, Pádraig Brady wrote:
> The attached patch should address this in coreutils.

Thanks for fixing that. This area of the code is confusing, and I 
attempted to simplify/clarify slightly by installing the attached 
cleanup patch. This shouldn't affect behavior.

From 300af7869161a3618e28cbae3daefc25c8267efe Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Fri, 5 May 2023 11:03:25 -0700
Subject: [PATCH] cp: -p --parents: minor cleanup of previous patch
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This doesn’t change behavior; it just clarifies the code a bit.
* src/cp.c (re_protect): New arg DST_SRC_NAME, for clarity, and so
that we need to skip '/'s only once.  Caller changed.
Rename a couple of local variables to try to make things clearer.
---
 src/cp.c | 55 ++++++++++++++++++++++++++++++-------------------------
 1 file changed, 30 insertions(+), 25 deletions(-)

diff --git a/src/cp.c b/src/cp.c
index 00a5cb813..619eb8260 100644
--- a/src/cp.c
+++ b/src/cp.c
@@ -278,10 +278,13 @@ regular file.\n\
   exit (status);
 }
 
-/* Ensure that parents of CONST_DST_NAME aka DST_DIRFD+DST_RELNAME have the
-   correct protections, for the --parents option.  This is done
-   after all copying has been completed, to allow permissions
-   that don't include user write/execute.
+/* Ensure that parents of CONST_DST_NAME have correct protections, for
+   the --parents option.  This is done after all copying has been
+   completed, to allow permissions that don't include user write/execute.
+
+   DST_SRC_NAME is the suffix of CONST_DST_NAME that is the source file name,
+   DST_DIRFD+DST_RELNAME is equivalent to CONST_DST_NAME, and
+   DST_RELNAME equals DST_SRC_NAME after skipping any leading '/'s.
 
    ATTR_LIST is a null-terminated linked list of structures that
    indicates the end of the filename of each intermediate directory
@@ -296,19 +299,21 @@ regular file.\n\
    when done.  */
 
 static bool
-re_protect (char const *const_dst_name, int dst_dirfd, char const *dst_fullname,
+re_protect (char const *const_dst_name, char const *dst_src_name,
+            int dst_dirfd, char const *dst_relname,
             struct dir_attr *attr_list, const struct cp_options *x)
 {
   struct dir_attr *p;
   char *dst_name;		/* A copy of CONST_DST_NAME we can change. */
-  char *src_name;		/* The relative source name in 'dst_name'. */
-  char *full_src_name;		/* The full source name in 'dst_name'. */
 
   ASSIGN_STRDUPA (dst_name, const_dst_name);
-  full_src_name = dst_name + (dst_fullname - const_dst_name);
-  src_name = full_src_name;
-  while (*src_name == '/')
-    src_name++;
+
+  /* The suffix of DST_NAME that is a copy of the source file name,
+     possibly truncated to name a parent directory.  */
+  char const *src_name = dst_name + (dst_src_name - const_dst_name);
+
+  /* Likewise, but with any leading '/'s skipped.  */
+  char const *relname = dst_name + (dst_relname - const_dst_name);
 
   for (p = attr_list; p; p = p->next)
     {
@@ -325,7 +330,7 @@ re_protect (char const *const_dst_name, int dst_dirfd, char const *dst_fullname,
           timespec[0] = get_stat_atime (&p->st);
           timespec[1] = get_stat_mtime (&p->st);
 
-          if (utimensat (dst_dirfd, src_name, timespec, 0))
+          if (utimensat (dst_dirfd, relname, timespec, 0))
             {
               error (0, errno, _("failed to preserve times for %s"),
                      quoteaf (dst_name));
@@ -335,7 +340,8 @@ re_protect (char const *const_dst_name, int dst_dirfd, char const *dst_fullname,
 
       if (x->preserve_ownership)
         {
-          if (lchownat (dst_dirfd, src_name, p->st.st_uid, p->st.st_gid) != 0)
+          if (lchownat (dst_dirfd, relname, p->st.st_uid, p->st.st_gid)
+              != 0)
             {
               if (! chown_failure_ok (x))
                 {
@@ -345,18 +351,18 @@ re_protect (char const *const_dst_name, int dst_dirfd, char const *dst_fullname,
                 }
               /* Failing to preserve ownership is OK. Still, try to preserve
                  the group, but ignore the possible error. */
-              ignore_value (lchownat (dst_dirfd, src_name, -1, p->st.st_gid));
+              ignore_value (lchownat (dst_dirfd, relname, -1, p->st.st_gid));
             }
         }
 
       if (x->preserve_mode)
         {
-          if (copy_acl (full_src_name, -1, dst_name, -1, p->st.st_mode) != 0)
+          if (copy_acl (src_name, -1, dst_name, -1, p->st.st_mode) != 0)
             return false;
         }
       else if (p->restore_mode)
         {
-          if (lchmodat (dst_dirfd, src_name, p->st.st_mode) != 0)
+          if (lchmodat (dst_dirfd, relname, p->st.st_mode) != 0)
             {
               error (0, errno, _("failed to preserve permissions for %s"),
                      quoteaf (dst_name));
@@ -690,8 +696,7 @@ do_copy (int n_files, char **file, char const *target_directory,
           char *dst_name;
           bool parent_exists = true;  /* True if dir_name (dst_name) exists. */
           struct dir_attr *attr_list;
-          char *arg_in_concat = NULL;
-          char *full_arg_in_concat = NULL;
+          char *arg_in_concat;
           char *arg = file[i];
 
           /* Trailing slashes are meaningful (i.e., maybe worth preserving)
@@ -723,10 +728,6 @@ do_copy (int n_files, char **file, char const *target_directory,
                  (dst_name, arg_in_concat - dst_name, target_dirfd,
                   (x->verbose ? "%s -> %s\n" : NULL),
                   &attr_list, &new_dst, x));
-
-              full_arg_in_concat = arg_in_concat;
-              while (*arg_in_concat == '/')
-                arg_in_concat++;
             }
           else
             {
@@ -748,13 +749,17 @@ do_copy (int n_files, char **file, char const *target_directory,
             }
           else
             {
+              char const *dst_relname = arg_in_concat;
+              while (*dst_relname == '/')
+                dst_relname++;
+
               bool copy_into_self;
-              ok &= copy (arg, dst_name, target_dirfd, arg_in_concat,
+              ok &= copy (arg, dst_name, target_dirfd, dst_relname,
                           new_dst, x, &copy_into_self, NULL);
 
               if (parents_option)
-                ok &= re_protect (dst_name, target_dirfd, full_arg_in_concat,
-                                  attr_list, x);
+                ok &= re_protect (dst_name, arg_in_concat, target_dirfd,
+                                  dst_relname, attr_list, x);
             }
 
           if (parents_option)
-- 
2.39.2