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.

6 files changed, 78 insertions, 33 deletions

diff --git a/ChangeLog b/ChangeLog
index ddeedf615..769becc3d 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,31 @@
+2007-06-18  Paul Eggert  <eggert@cs.ucla.edu>
+
+	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.
+
 2007-06-18  Jim Meyering  <jim@meyering.net>
 
 	Use mreadlink_with_size (doesn't exit), not xreadlink_with_size.
diff --git a/NEWS b/NEWS
index f1b81f0cc..c587fe7cb 100644
--- a/NEWS
+++ b/NEWS
@@ -18,7 +18,10 @@ GNU coreutils NEWS                                    -*- outline -*-
 ** Bug fixes
 
   cp no longer fails to write through a dangling symlink
-  [bug introduced in coreutils-6.7].  Also, 'cp' no longer considers a
+  [bug introduced in coreutils-6.7].  cp --parents no
+  longer mishandles symlinks to directories in file name
+  components in the source, e.g., "cp --parents symlink/a/b
+  d" no longer fails.  Also, 'cp' no longer considers a
   destination symlink to be the same as the referenced file when
   copying links or making backups.  For example, if SYM is a symlink
   to FILE, "cp -l FILE SYM" now reports an error instead of silently
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
diff --git a/src/cp.c b/src/cp.c
index 1d66bc382..9436be6fb 100644
--- a/src/cp.c
+++ b/src/cp.c
@@ -36,6 +36,10 @@
 #include "utimens.h"
 #include "acl.h"
 
+#if ! HAVE_LCHOWN
+# define lchown(name, uid, gid) chown (name, uid, gid)
+#endif
+
 #define ASSIGN_BASENAME_STRDUPA(Dest, File_name)	\
   do							\
     {							\
@@ -56,7 +60,7 @@
    need to be fixed after copying. */
 struct dir_attr
 {
-  mode_t mode;
+  struct stat st;
   bool restore_mode;
   size_t slash_offset;
   struct dir_attr *next;
@@ -290,17 +294,8 @@ re_protect (char const *const_dst_name, size_t src_offset,
 
   for (p = attr_list; p; p = p->next)
     {
-      struct stat src_sb;
-
       dst_name[p->slash_offset] = '\0';
 
-      if (XSTAT (x, src_name, &src_sb))
-	{
-	  error (0, errno, _("failed to get attributes of %s"),
-		 quote (src_name));
-	  return false;
-	}
-
       /* Adjust the times (and if possible, ownership) for the copy.
 	 chown turns off set[ug]id bits for non-root,
 	 so do the chmod last.  */
@@ -309,8 +304,8 @@ re_protect (char const *const_dst_name, size_t src_offset,
 	{
 	  struct timespec timespec[2];
 
-	  timespec[0] = get_stat_atime (&src_sb);
-	  timespec[1] = get_stat_mtime (&src_sb);
+	  timespec[0] = get_stat_atime (&p->st);
+	  timespec[1] = get_stat_mtime (&p->st);
 
 	  if (utimens (dst_name, timespec))
 	    {
@@ -322,7 +317,7 @@ re_protect (char const *const_dst_name, size_t src_offset,
 
       if (x->preserve_ownership)
 	{
-	  if (chown (dst_name, src_sb.st_uid, src_sb.st_gid) != 0
+	  if (lchown (dst_name, p->st.st_uid, p->st.st_gid) != 0
 	      && ! chown_failure_ok (x))
 	    {
 	      error (0, errno, _("failed to preserve ownership for %s"),
@@ -333,12 +328,12 @@ re_protect (char const *const_dst_name, size_t src_offset,
 
       if (x->preserve_mode)
 	{
-	  if (copy_acl (src_name, -1, dst_name, -1, src_sb.st_mode))
+	  if (copy_acl (src_name, -1, dst_name, -1, p->st.st_mode) != 0)
 	    return false;
 	}
       else if (p->restore_mode)
 	{
-	  if (lchmod (dst_name, p->mode) != 0)
+	  if (lchmod (dst_name, p->st.st_mode) != 0)
 	    {
 	      error (0, errno, _("failed to preserve permissions for %s"),
 		     quote (dst_name));
@@ -421,14 +416,14 @@ make_dir_parents_private (char const *const_dir, size_t src_offset,
 	      int src_errno;
 
 	      /* This component does not exist.  We must set
-		 *new_dst and new->mode inside this loop because,
+		 *new_dst and new->st.st_mode inside this loop because,
 		 for example, in the command `cp --parents ../a/../b/c e_dir',
 		 make_dir_parents_private creates only e_dir/../a if
 		 ./b already exists. */
 	      *new_dst = true;
-	      src_errno = (stat (src, &stats) != 0
+	      src_errno = (stat (src, &new->st) != 0
 			   ? errno
-			   : S_ISDIR (stats.st_mode)
+			   : S_ISDIR (new->st.st_mode)
 			   ? 0
 			   : ENOTDIR);
 	      if (src_errno)
@@ -437,7 +432,7 @@ make_dir_parents_private (char const *const_dir, size_t src_offset,
 			 quote (src));
 		  return false;
 		}
-	      src_mode = stats.st_mode;
+	      src_mode = new->st.st_mode;
 
 	      /* If the ownership or special mode bits might change,
 		 omit some permissions at first, so unauthorized users
@@ -485,7 +480,7 @@ make_dir_parents_private (char const *const_dir, size_t src_offset,
 		  if (omitted_permissions & ~stats.st_mode
 		      || (stats.st_mode & S_IRWXU) != S_IRWXU)
 		    {
-		      new->mode = stats.st_mode | omitted_permissions;
+		      new->st.st_mode = stats.st_mode | omitted_permissions;
 		      new->restore_mode = true;
 		    }
 		}
diff --git a/src/install.c b/src/install.c
index dc7ff0141..39d514957 100644
--- a/src/install.c
+++ b/src/install.c
@@ -62,6 +62,10 @@ static bool use_default_selinux_context = true;
 # define endpwent() ((void) 0)
 #endif
 
+#if ! HAVE_LCHOWN
+# define lchown(name, uid, gid) chown (name, uid, gid)
+#endif
+
 /* Initial number of entries in each hash table entry's table of inodes.  */
 #define INITIAL_HASH_MODULE 100
 
@@ -606,7 +610,7 @@ change_attributes (char const *name)
      want to know.  */
 
   if (! (owner_id == (uid_t) -1 && group_id == (gid_t) -1)
-      && chown (name, owner_id, group_id) != 0)
+      && lchown (name, owner_id, group_id) != 0)
     error (0, errno, _("cannot change ownership of %s"), quote (name));
   else if (chmod (name, mode) != 0)
     error (0, errno, _("cannot change permissions of %s"), quote (name));
diff --git a/tests/cp/cp-parents b/tests/cp/cp-parents
index 384f6c926..274f47d8d 100755
--- a/tests/cp/cp-parents
+++ b/tests/cp/cp-parents
@@ -48,7 +48,8 @@ cd $tmp || framework_failure=1
 . "$abs_srcdir/../setgid-check"
 
 mkdir foo bar || framework_failure=1
-mkdir -p a/b/c d e || framework_failure=1
+mkdir -p a/b/c d e g || framework_failure=1
+ln -s d/a sym || framework_failure=1
 touch f || framework_failure=1
 
 if test $framework_failure = 1; then
@@ -75,8 +76,11 @@ test -d d/f && fail=1
 # Check that re_protect works.
 chmod go=w d/a
 cp -a --parents d/a/b/c e || fail=1
+cp -a --parents sym/b/c g || fail=1
 p=`ls -ld e/d|cut -b-10`; case $p in drwxr-xr-x);; *) fail=1;; esac
 p=`ls -ld e/d/a|cut -b-10`; case $p in drwx-w--w-);; *) fail=1;; esac
+p=`ls -ld g/sym|cut -b-10`; case $p in drwx-w--w-);; *) fail=1;; esac
 p=`ls -ld e/d/a/b/c|cut -b-10`; case $p in drwxr-xr-x);; *) fail=1;; esac
+p=`ls -ld g/sym/b/c|cut -b-10`; case $p in drwxr-xr-x);; *) fail=1;; esac
 
 (exit $fail); exit $fail