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

From: Pádraig Brady <[email-masked]>
Date: Wed, 3 May 2023 17:39:57 +0100

On 03/05/2023 10:27, Schlomo Schapiro wrote:
> Hello,
> 
> I'm a maintainer of the Relax-and-Recover (https://relax-and-recover.org/)
> Open Source project and think that I might have found a major regression in
> cp, starting somewhere with version 9.
> 
> Please see https://bugs.launchpad.net/ubuntu/+source/coreutils/+bug/2017414
> and https://github.com/rear/rear/issues/2972 for how I found out about this.
> 
> Problem:
> 
> We use a cp call like the following to copy various files and directory
> into a destination path with preserving the structure:
> 
> cp --verbose -t DESTINATION -L --preserve=all --parents SOURCE...
> 
> Over the last 10+ years that worked well on all Linux distros (ReaR is
> build for and tested on nearly all distros), but I recently found out that
> on Ubuntu 23.04 this fails like this:
> 
> # rm -Rf /tmp/f && mkdir /tmp/f && cp --verbose -t /tmp/f -L --preserve=all
> --parents /etc/apt/sources.list && echo yes ; ls -lR
> /tmp/f/etc/apt/sources.list
> /etc/apt/sources.list
> /etc -> /tmp/f/etc
> /etc/apt -> /tmp/f/etc/apt
> '/etc/apt/sources.list' -> '/tmp/f/etc/apt/sources.list'
> cp: ‘etc/apt’: No such file or directory
> -rw-r--r-- 1 root root 2437 Apr 23 09:53 /etc/apt/sources.list
> -rw-r--r-- 1 root root 2437 Apr 23 09:53 /tmp/f/etc/apt/sources.list
> #
> 
> Ubuntu 23.04 uses cp (GNU coreutils) 9.1
> 
> On Ubuntu 22.04 there is cp (GNU coreutils) 8.32 and the same example works
> as expected:
> 
> # rm -Rf /tmp/f && mkdir /tmp/f && cp --verbose -t /tmp/f -L --preserve=all
> --parents /etc/apt/sources.list && echo yes ; ls -lR
> /tmp/f/etc/apt/sources.list /etc/apt/sources.list
> /etc -> /tmp/f/etc
> /etc/apt -> /tmp/f/etc/apt
> '/etc/apt/sources.list' -> '/tmp/f/etc/apt/sources.list'
> yes
> -rw-r--r-- 1 root root 263 Mär 26 15:20 /etc/apt/sources.list
> -rw-r--r-- 1 root root 263 Mär 26 15:20 /tmp/f/etc/apt/sources.list
> #
> 
> BTW, I checked also on many other distros that ReaR supports and all
> distros with cp version 9.1 fail in the same way.
> 
> Can you please have a look and advise how to proceed? We at the ReaR
> project can of course change our code to use tar for example, but I won't
> be surprised if other users will also meet this changed behaviour and maybe
> it is indeed a bug.

Looks like a bug indeed.
The attached patch should address this in coreutils.
Any deployments of coreutils 9.1-9.3 inclusive would need to apply this.

thanks,
Pádraig
From 8ad4a486fe03203e00f31262cd106e58385da0f6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email-masked]>
Date: Wed, 3 May 2023 17:01:37 +0100
Subject: [PATCH] cp: -p --parents: fix failure to preserve permissions for
 absolute paths

* src/cp.c (re_protect): Ensure copy_acl() is passed an absolute path.
* tests/cp/cp-parents.sh: Add a test case.
* NEWS: Mention the bug.
Fixes https://bugs.gnu.org/63245
---
 NEWS                   |  4 ++++
 src/cp.c               | 16 +++++++++++-----
 tests/cp/cp-parents.sh |  6 ++++++
 3 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/NEWS b/NEWS
index 3d34a1b3c..9fad8a775 100644
--- a/NEWS
+++ b/NEWS
@@ -4,6 +4,10 @@ GNU coreutils NEWS                                    -*- outline -*-
 
 ** Bug fixes
 
+  cp --parents again succeeds to preserve mode for absolute directories.
+  Previously it would have failed with a "No such file or directory" error.
+  [bug introduced in coreutils-9.1]
+
   cksum again diagnoses read errors in its default CRC32 mode.
   [bug introduced in coreutils-9.0]
 
diff --git a/src/cp.c b/src/cp.c
index 488770a0b..00a5cb813 100644
--- a/src/cp.c
+++ b/src/cp.c
@@ -296,15 +296,19 @@ regular file.\n\
    when done.  */
 
 static bool
-re_protect (char const *const_dst_name, int dst_dirfd, char const *dst_relname,
+re_protect (char const *const_dst_name, int dst_dirfd, char const *dst_fullname,
             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 source name in 'dst_name'. */
+  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);
-  src_name = dst_name + (dst_relname - const_dst_name);
+  full_src_name = dst_name + (dst_fullname - const_dst_name);
+  src_name = full_src_name;
+  while (*src_name == '/')
+    src_name++;
 
   for (p = attr_list; p; p = p->next)
     {
@@ -347,7 +351,7 @@ re_protect (char const *const_dst_name, int dst_dirfd, char const *dst_relname,
 
       if (x->preserve_mode)
         {
-          if (copy_acl (src_name, -1, dst_name, -1, p->st.st_mode) != 0)
+          if (copy_acl (full_src_name, -1, dst_name, -1, p->st.st_mode) != 0)
             return false;
         }
       else if (p->restore_mode)
@@ -687,6 +691,7 @@ do_copy (int n_files, char **file, char const *target_directory,
           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 = file[i];
 
           /* Trailing slashes are meaningful (i.e., maybe worth preserving)
@@ -719,6 +724,7 @@ do_copy (int n_files, char **file, char const *target_directory,
                   (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++;
             }
@@ -747,7 +753,7 @@ do_copy (int n_files, char **file, char const *target_directory,
                           new_dst, x, &copy_into_self, NULL);
 
               if (parents_option)
-                ok &= re_protect (dst_name, target_dirfd, arg_in_concat,
+                ok &= re_protect (dst_name, target_dirfd, full_arg_in_concat,
                                   attr_list, x);
             }
 
diff --git a/tests/cp/cp-parents.sh b/tests/cp/cp-parents.sh
index 20963eace..84da28ae7 100755
--- a/tests/cp/cp-parents.sh
+++ b/tests/cp/cp-parents.sh
@@ -66,4 +66,10 @@ p=$(ls -ld g/sym/b/c|cut -b-10); case $p in drwxr-xr-x);; *) fail=1;; esac
 cp --parents --no-preserve=mode np/b/file np_dest/ || fail=1
 p=$(ls -ld np_dest/np|cut -b-10); case $p in drwxr-xr-x);; *) fail=1;; esac
 
+# coreutils 9.1-9.3 inclusive would fail to copy acls for absolute files
+mkdir dest || framework_failure_
+if test -f /bin/ls; then
+  cp -t dest -L --preserve=all --parents /bin/ls || fail=1
+fi
+
 Exit $fail
-- 
2.26.2