GNU bug report logs - #52006
[INSTALLED] cp: fix --preserve=ownership permissions bug

Previous Next

Package: coreutils;

Reported by: Paul Eggert <eggert <at> cs.ucla.edu>

Date: Sat, 20 Nov 2021 21:52:01 UTC

Severity: normal

Done: Paul Eggert <eggert <at> cs.ucla.edu>

Bug is archived. No further changes may be made.

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 52006 in the body.
You can then email your comments to 52006 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to bug-coreutils <at> gnu.org:
bug#52006; Package coreutils. (Sat, 20 Nov 2021 21:52:01 GMT) Full text and rfc822 format available.

Acknowledgement sent to Paul Eggert <eggert <at> cs.ucla.edu>:
New bug report received and forwarded. Copy sent to bug-coreutils <at> gnu.org. (Sat, 20 Nov 2021 21:52:02 GMT) Full text and rfc822 format available.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: bug-coreutils <at> gnu.org
Cc: Paul Eggert <eggert <at> cs.ucla.edu>
Subject: [INSTALLED] cp: fix --preserve=ownership permissions bug
Date: Sat, 20 Nov 2021 13:51:13 -0800
This fixes a bug that I introduced in
2006-12-06T19:44:08Z!eggert <at> cs.ucla.edu.
* src/copy.c (USE_XATTR): New macro.
(copy_reg): Use it to help the compiler.  Prefer open u+w to a
later chmod u=rw; u+r isn’t needed for xattr.  For the later u-r,
do only one (or zero) chmod calls instead of two (or one).
In the last chmod, respect the umask instead of ignoring it.
* tests/cp/preserve-mode.sh: Test for the bug.
---
 NEWS                      |  4 +++
 src/copy.c                | 54 ++++++++++++++++++++++-----------------
 tests/cp/preserve-mode.sh | 10 +++++++-
 3 files changed, 44 insertions(+), 24 deletions(-)

diff --git a/NEWS b/NEWS
index 6350abc3c..3f7ed7218 100644
--- a/NEWS
+++ b/NEWS
@@ -13,6 +13,10 @@ GNU coreutils NEWS                                    -*- outline -*-
   before adjusting it to the correct value.
   [bug introduced in coreutils-8.17]
 
+  'cp --preserve=ownership A B' no longer ignores the umask when creating B.
+  Also, 'cp --preserve-xattr A B' is less likely to temporarily chmod u+w B.
+  [bug introduced in coreutils-6.7]
+
   On macOS, 'cp A B' no longer miscopies when A is in an APFS file system
   and B is in some other file system.
   [bug introduced in coreutils-9.0]
diff --git a/src/copy.c b/src/copy.c
index ba46cd3b7..9f20a34b9 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -66,6 +66,10 @@
 #include "xstrtol.h"
 #include "selinux.h"
 
+#ifndef USE_XATTR
+# define USE_XATTR false
+#endif
+
 #if USE_XATTR
 # include <attr/error_context.h>
 # include <attr/libattr.h>
@@ -1138,11 +1142,13 @@ copy_reg (char const *src_name, char const *dst_name,
   int dest_errno;
   int source_desc;
   mode_t src_mode = src_sb->st_mode;
+  mode_t extra_permissions;
   struct stat sb;
   struct stat src_open_sb;
   union scan_inference scan_inference;
   bool return_val = true;
   bool data_copy_required = x->data_copy_required;
+  bool preserve_xattr = USE_XATTR & x->preserve_xattr;
 
   source_desc = open (src_name,
                       (O_RDONLY | O_BINARY
@@ -1239,9 +1245,16 @@ copy_reg (char const *src_name, char const *dst_name,
 
   if (*new_dst)
     {
+      /* To allow copying xattrs on read-only files, create with u+w.
+         This satisfies an inode permission check done by
+         xattr_permission in fs/xattr.c of the GNU/Linux kernel.  */
+      mode_t open_mode =
+        ((dst_mode & ~omitted_permissions)
+         | (preserve_xattr && !x->owner_privileges ? S_IWUSR : 0));
+      extra_permissions = open_mode & ~dst_mode; /* either 0 or S_IWUSR */
+
       int open_flags = O_WRONLY | O_CREAT | O_BINARY;
-      dest_desc = open (dst_name, open_flags | O_EXCL,
-                        dst_mode & ~omitted_permissions);
+      dest_desc = open (dst_name, open_flags | O_EXCL, open_mode);
       dest_errno = errno;
 
       /* When trying to copy through a dangling destination symlink,
@@ -1262,8 +1275,7 @@ copy_reg (char const *src_name, char const *dst_name,
             {
               if (x->open_dangling_dest_symlink)
                 {
-                  dest_desc = open (dst_name, open_flags,
-                                    dst_mode & ~omitted_permissions);
+                  dest_desc = open (dst_name, open_flags, open_mode);
                   dest_errno = errno;
                 }
               else
@@ -1284,7 +1296,7 @@ copy_reg (char const *src_name, char const *dst_name,
     }
   else
     {
-      omitted_permissions = 0;
+      omitted_permissions = extra_permissions = 0;
     }
 
   if (dest_desc < 0)
@@ -1302,6 +1314,14 @@ copy_reg (char const *src_name, char const *dst_name,
       goto close_src_and_dst_desc;
     }
 
+  /* If extra permissions needed for copy_xattr didn't happen (e.g.,
+     due to umask) chmod to add them temporarily; if that fails give
+     up with extra permissions, letting copy_attr fail later.  */
+  mode_t temporary_mode = sb.st_mode | extra_permissions;
+  if (temporary_mode != sb.st_mode
+      && fchmod_or_lchmod (dest_desc, dst_name, temporary_mode) != 0)
+    extra_permissions = 0;
+
   /* --attributes-only overrides --reflink.  */
   if (data_copy_required && x->reflink_mode)
     {
@@ -1433,25 +1453,11 @@ copy_reg (char const *src_name, char const *dst_name,
         }
     }
 
-  /* To allow copying xattrs on read-only files, temporarily chmod u+rw.
-     This workaround is required as an inode permission check is done
-     by xattr_permission() in fs/xattr.c of the GNU/Linux kernel tree.  */
-  if (x->preserve_xattr)
+  if (preserve_xattr)
     {
-      bool access_changed = false;
-
-      if (!(sb.st_mode & S_IWUSR) && geteuid () != ROOT_UID)
-        {
-          access_changed = fchmod_or_lchmod (dest_desc, dst_name,
-                                             S_IRUSR | S_IWUSR) == 0;
-        }
-
       if (!copy_attr (src_name, source_desc, dst_name, dest_desc, x)
           && x->require_preserve_xattr)
         return_val = false;
-
-      if (access_changed)
-        fchmod_or_lchmod (dest_desc, dst_name, dst_mode & ~omitted_permissions);
     }
 
   set_author (dst_name, dest_desc, src_sb);
@@ -1472,11 +1478,13 @@ copy_reg (char const *src_name, char const *dst_name,
       if (set_acl (dst_name, dest_desc, MODE_RW_UGO & ~cached_umask ()) != 0)
         return_val = false;
     }
-  else if (omitted_permissions)
+  else if (omitted_permissions | extra_permissions)
     {
       omitted_permissions &= ~ cached_umask ();
-      if (omitted_permissions
-          && fchmod_or_lchmod (dest_desc, dst_name, dst_mode) != 0)
+      if ((omitted_permissions | extra_permissions)
+          && (fchmod_or_lchmod (dest_desc, dst_name,
+                                dst_mode & ~ cached_umask ())
+              != 0))
         {
           error (0, errno, _("preserving permissions for %s"),
                  quoteaf (dst_name));
diff --git a/tests/cp/preserve-mode.sh b/tests/cp/preserve-mode.sh
index c47dee650..c764300b6 100755
--- a/tests/cp/preserve-mode.sh
+++ b/tests/cp/preserve-mode.sh
@@ -1,5 +1,5 @@
 #!/bin/sh
-# ensure that cp's --no-preserve=mode works correctly
+# Check whether cp generates files with correct modes.
 
 # Copyright (C) 2002-2021 Free Software Foundation, Inc.
 
@@ -55,4 +55,12 @@ if mkfifo fifo; then
   test "$(get_mode fifo)" = "$(get_mode fifo_copy)" || fail=1
 fi
 
+# Test that plain --preserve=ownership does not affect destination mode.
+rm -f a b c || framework_failure_
+touch a || framework_failure_
+chmod 660 a || framework_failure_
+cp a b || fail=1
+cp --preserve=ownership a c || fail=1
+test "$(get_mode b)" = "$(get_mode c)" || fail=1
+
 Exit $fail
-- 
2.33.1





bug closed, send any further explanations to 52006 <at> debbugs.gnu.org and Paul Eggert <eggert <at> cs.ucla.edu> Request was from Paul Eggert <eggert <at> cs.ucla.edu> to control <at> debbugs.gnu.org. (Sat, 20 Nov 2021 21:55:02 GMT) Full text and rfc822 format available.

bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Sun, 19 Dec 2021 12:24:05 GMT) Full text and rfc822 format available.

This bug report was last modified 2 years and 91 days ago.

Previous Next


GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.