A few more symlink-related fixes. Fix a bug triggered by cp

--parents and symlinks.  Close some race conditions possible when
the destination replaces a newly-created file with a symlink.
* NEWS: Document that 'cp --parents' no longer mishandles
symlinks in file name components of source.
* src/copy.c (HAVE_LCHOWN): Default to false.
(lchown) [!defined HAVE_LCHOWN]: Define to chown, for convenience.
* src/cp.c (lchown) [!HAVE_LCHOWN]: Likewise.
* src/install.c (lchown [!HAVE_LCHOWN]: Likewise.
* src/copy.c (set_owner): Use lchown instead of chown, for safety
in case the file got replaced by a symlink in the meantime.
* src/cp.c (re_protect): Likewise.
* src/install.c (change_attributes): Likewise.
* src/copy.c (copy_internal): Use ordinary C rather than an #if.
* src/cp.c (lchown) [!HAVE_LCHOWN]: Define to chown, for convenience.
(struct dir_attr): Cache the entire struct stat of the directory,
rather than just its mode, so that we needn't stat the directory
twice (which can lead to races).
(re_protect): Don't use XSTAT as that's not appropriate in
this context (symlinks should be followed here).  Instead, use
the cached stat value.
(make_dir_parents_private): Save dir's entire struct stat, not
just its mode.
* tests/cp/cp-parents: Add test to check against bug with
cp --parents and symlinks.

1 files changed, 21 insertions, 10 deletions

diff --git a/src/copy.c b/src/copy.c
index fd0519096..fec8c231b 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -62,6 +62,11 @@
 # define fchown(fd, uid, gid) (-1)
 #endif
 
+#ifndef HAVE_LCHOWN
+# define HAVE_LCHOWN false
+# define lchown(name, uid, gid) chown (name, uid, gid)
+#endif
+
 #define SAME_OWNER(A, B) ((A).st_uid == (B).st_uid)
 #define SAME_GROUP(A, B) ((A).st_gid == (B).st_gid)
 #define SAME_OWNER_AND_GROUP(A, B) (SAME_OWNER (A, B) && SAME_GROUP (A, B))
@@ -172,7 +177,9 @@ copy_dir (char const *src_name_in, char const *dst_name_in, bool new_dst,
 
 /* Set the owner and owning group of DEST_DESC to the st_uid and
    st_gid fields of SRC_SB.  If DEST_DESC is undefined (-1), set
-   the owner and owning group of DST_NAME instead.  DEST_DESC must
+   the owner and owning group of DST_NAME instead; for
+   safety prefer lchown if the system supports it since no
+   symbolic links should be involved.  DEST_DESC must
    refer to the same file as DEST_NAME if defined.
    Return 1 if the syscall succeeds, 0 if it fails but it's OK
    not to preserve ownership, -1 otherwise.  */
@@ -188,7 +195,7 @@ set_owner (const struct cp_options *x, char const *dst_name, int dest_desc,
     }
   else
     {
-      if (chown (dst_name, uid, gid) == 0)
+      if (lchown (dst_name, uid, gid) == 0)
 	return 1;
     }
 
@@ -212,6 +219,9 @@ static void
 set_author (const char *dst_name, int dest_desc, const struct stat *src_sb)
 {
 #if HAVE_STRUCT_STAT_ST_AUTHOR
+  /* FIXME: Modify the following code so that it does not
+     follow symbolic links.  */
+
   /* Preserve the st_author field.  */
   file_t file = (dest_desc < 0
 		 ? file_name_lookup (dst_name, 0, 0)
@@ -1909,20 +1919,21 @@ copy_internal (char const *src_name, char const *dst_name,
 	{
 	  /* Preserve the owner and group of the just-`copied'
 	     symbolic link, if possible.  */
-#if HAVE_LCHOWN
-	  if (lchown (dst_name, src_sb.st_uid, src_sb.st_gid) != 0
+	  if (HAVE_LCHOWN
+	      && lchown (dst_name, src_sb.st_uid, src_sb.st_gid) != 0
 	      && ! chown_failure_ok (x))
 	    {
 	      error (0, errno, _("failed to preserve ownership for %s"),
 		     dst_name);
 	      goto un_backup;
 	    }
-#else
-	  /* Can't preserve ownership of symlinks.
-	     FIXME: maybe give a warning or even error for symlinks
-	     in directories with the sticky bit set -- there, not
-	     preserving owner/group is a potential security problem.  */
-#endif
+	  else
+	    {
+	      /* Can't preserve ownership of symlinks.
+		 FIXME: maybe give a warning or even error for symlinks
+		 in directories with the sticky bit set -- there, not
+		 preserving owner/group is a potential security problem.  */
+	    }
 	}
     }
   else