cp: by default, refuse to copy through a dangling destination symlink

* NEWS: Mention this change.
* doc/coreutils.texi (cp invocation): Describe the new behavior.
* src/copy.c: No longer include "canonicalize.h".
(copy_reg): Upon failure to open a dangling destination symlink, don't
canonicalize the name, but rather fail (default) or, with POSIXLY_CORRECT,
repeat the open call without O_EXCL (potentially dangerous).
* src/copy.h (struct cp_options) [open_dangling_dest_symlink]:
New member.  Reorder the others, grouping "bool" and "enum"
members together.
* tests/cp/thru-dangling: Test for changed and new behavior.
* src/cp.c (cp_option_init): Initialize new member.
* src/install.c (cp_option_init): Likewise.
* src/mv.c (cp_option_init): Likewise.

Signed-off-by: Jim Meyering <meyering@redhat.com>

9 files changed, 94 insertions, 43 deletions

diff --git a/ChangeLog b/ChangeLog
index 1d9655edc..2b47da4b4 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,21 @@
+2007-11-22  Jim Meyering  <meyering@redhat.com>
+
+	cp: by default, refuse to copy through a dangling destination symlink
+	* NEWS: Mention this change.
+	* doc/coreutils.texi (cp invocation): Describe the new behavior.
+	* src/copy.c: No longer include "canonicalize.h".
+	(copy_reg): Upon failure to open a dangling destination symlink,
+	don't canonicalize the name, but rather fail (default) or, with
+	POSIXLY_CORRECT, repeat the open call without O_EXCL (potentially
+	dangerous).
+	* src/copy.h (struct cp_options) [open_dangling_dest_symlink]:
+	New member.  Reorder the others, grouping "bool" and "enum"
+	members together.
+	* tests/cp/thru-dangling: Test for changed and new behavior.
+	* src/cp.c (cp_option_init): Initialize new member.
+	* src/install.c (cp_option_init): Likewise.
+	* src/mv.c (cp_option_init): Likewise.
+
 2007-11-21  Pádraig Brady <P@draigBrady.com>
 
 	* doc/coreutils.texi (split invocation): Improve the
diff --git a/NEWS b/NEWS
index 11efa7582..a5936f896 100644
--- a/NEWS
+++ b/NEWS
@@ -15,6 +15,9 @@ GNU coreutils NEWS                                    -*- outline -*-
 
 ** Changes in behavior
 
+  cp, by default, refuses to copy through a dangling destination symlink
+  Set POSIXLY_CORRECT if you require the old, risk-prone behavior.
+
   pr -F no longer suppresses the footer or the first two blank lines in
   the header.  This is for compatibility with BSD and POSIX.
 
diff --git a/doc/coreutils.texi b/doc/coreutils.texi
index 136a3f7fa..6ed1171de 100644
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -6920,10 +6920,15 @@ recursively.  This default can be overridden with the
 @option{-H} options.  If more than one of these options is specified,
 the last one silently overrides the others.
 
-When copying to a symbolic link, @command{cp} normally follows the
-link when creating or copying to a regular file, even if the link is
-dangling.  However, @command{cp} does not follow these links when
-creating directories or special files.  Also, when an option like
+When copying to a symbolic link, @command{cp} follows the
+link only when it refers to an existing regular file.
+However, when copying to a dangling symbolic link, @command{cp}
+refuses by default, and fails with a diagnostic, since the operation
+is inherently dangerous.  This behavior is contrary to historical
+practice and to @acronym{POSIX}.
+Set @env{POSIXLY_CORRECT} to make @command{cp} attempt to create
+the target of a dangling destination symlink, in spite of the possible risk.
+Also, when an option like
 @option{--backup} or @option{--link} acts to rename or remove the
 destination before copying, @command{cp} renames or removes the
 symbolic link rather than the file it points to.
diff --git a/src/copy.c b/src/copy.c
index 1a265e3c2..31e29b1bf 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -33,7 +33,6 @@
 #include "acl.h"
 #include "backupfile.h"
 #include "buffer-lcm.h"
-#include "canonicalize.h"
 #include "copy.h"
 #include "cp-hash.h"
 #include "euidaccess.h"
@@ -265,7 +264,6 @@ copy_reg (char const *src_name, char const *dst_name,
   char *buf;
   char *buf_alloc = NULL;
   char *name_alloc = NULL;
-  char const *followed_dest_name = dst_name;
   int dest_desc;
   int dest_errno;
   int source_desc;
@@ -362,35 +360,39 @@ copy_reg (char const *src_name, char const *dst_name,
 
   if (*new_dst)
     {
-      int open_flags = O_WRONLY | O_CREAT | O_EXCL | O_BINARY;
-      dest_desc = open (dst_name, open_flags,
+      int open_flags = O_WRONLY | O_CREAT | O_BINARY;
+      dest_desc = open (dst_name, open_flags | O_EXCL ,
 			dst_mode & ~omitted_permissions);
       dest_errno = errno;
 
       /* When trying to copy through a dangling destination symlink,
 	 the above open fails with EEXIST.  If that happens, and
-	 lstat'ing the DST_NAME shows that it is a symlink, repeat
-	 the open call, but this time with the name of the final,
-	 missing directory entry.  All of this is relevant only for
-	 cp, i.e., not in move_mode. */
+	 lstat'ing the DST_NAME shows that it is a symlink, then we
+	 have a problem: trying to resolve this dangling symlink to
+	 a directory/destination-entry pair is fundamentally racy,
+	 so punt.  If POSIXLY_CORRECT is set, simply call open again,
+	 but without O_EXCL (potentially dangerous).  If not, fail
+	 with a diagnostic.  These shenanigans are necessary only
+	 when copying, i.e., not in move_mode.  */
       if (dest_desc < 0 && dest_errno == EEXIST && ! x->move_mode)
 	{
 	  struct stat dangling_link_sb;
 	  if (lstat (dst_name, &dangling_link_sb) == 0
 	      && S_ISLNK (dangling_link_sb.st_mode))
 	    {
-	      /* FIXME: This is way overkill, since all that's needed
-		 is to follow the symlink that is the last file name
-		 component.  */
-	      name_alloc =
-		canonicalize_filename_mode (dst_name, CAN_MISSING);
-	      if (name_alloc)
+	      if (x->open_dangling_dest_symlink)
 		{
-		  followed_dest_name = name_alloc;
-		  dest_desc = open (followed_dest_name, open_flags,
+		  dest_desc = open (dst_name, open_flags,
 				    dst_mode & ~omitted_permissions);
 		  dest_errno = errno;
 		}
+	      else
+		{
+		  error (0, 0, _("not writing through dangling symlink %s"),
+			 quote (dst_name));
+		  return_val = false;
+		  goto close_src_desc;
+		}
 	    }
 	}
     }
@@ -591,7 +593,7 @@ copy_reg (char const *src_name, char const *dst_name,
       timespec[0] = get_stat_atime (src_sb);
       timespec[1] = get_stat_mtime (src_sb);
 
-      if (gl_futimens (dest_desc, followed_dest_name, timespec) != 0)
+      if (gl_futimens (dest_desc, dst_name, timespec) != 0)
 	{
 	  error (0, errno, _("preserving times for %s"), quote (dst_name));
 	  if (x->require_preserve)
@@ -604,7 +606,7 @@ copy_reg (char const *src_name, char const *dst_name,
 
   if (x->preserve_ownership && ! SAME_OWNER_AND_GROUP (*src_sb, sb))
     {
-      switch (set_owner (x, followed_dest_name, dest_desc,
+      switch (set_owner (x, dst_name, dest_desc,
 			 src_sb->st_uid, src_sb->st_gid))
 	{
 	case -1:
@@ -617,24 +619,24 @@ copy_reg (char const *src_name, char const *dst_name,
 	}
     }
 
-  set_author (followed_dest_name, dest_desc, src_sb);
+  set_author (dst_name, dest_desc, src_sb);
 
   if (x->preserve_mode || x->move_mode)
     {
-      if (copy_acl (src_name, source_desc, followed_dest_name, dest_desc, src_mode) != 0
+      if (copy_acl (src_name, source_desc, dst_name, dest_desc, src_mode) != 0
 	  && x->require_preserve)
 	return_val = false;
     }
   else if (x->set_mode)
     {
-      if (set_acl (followed_dest_name, dest_desc, x->mode) != 0)
+      if (set_acl (dst_name, dest_desc, x->mode) != 0)
 	return_val = false;
     }
   else if (omitted_permissions)
     {
       omitted_permissions &= ~ cached_umask ();
       if (omitted_permissions
-	  && fchmod_or_lchmod (dest_desc, followed_dest_name, dst_mode) != 0)
+	  && fchmod_or_lchmod (dest_desc, dst_name, dst_mode) != 0)
 	{
 	  error (0, errno, _("preserving permissions for %s"),
 		 quote (dst_name));
diff --git a/src/copy.h b/src/copy.h
index 2ea7c0549..47c97f389 100644
--- a/src/copy.h
+++ b/src/copy.h
@@ -82,13 +82,25 @@ struct cp_options
 {
   enum backup_type backup_type;
 
+  /* How to handle symlinks in the source.  */
+  enum Dereference_symlink dereference;
+
+  /* This value is used to determine whether to prompt before removing
+     each existing destination file.  It works differently depending on
+     whether move_mode is set.  See code/comments in copy.c.  */
+  enum Interactive interactive;
+
+  /* Control creation of sparse files.  */
+  enum Sparse_type sparse_mode;
+
+  /* Set the mode of the destination file to exactly this value
+     if SET_MODE is nonzero.  */
+  mode_t mode;
+
   /* If true, copy all files except (directories and, if not dereferencing
      them, symbolic links,) as if they were regular files.  */
   bool copy_as_regular;
 
-  /* How to handle symlinks in the source.  */
-  enum Dereference_symlink dereference;
-
   /* If true, remove each existing destination nondirectory before
      trying to open it.  */
   bool unlink_dest_before_opening;
@@ -104,11 +116,6 @@ struct cp_options
      Create destination directories as usual. */
   bool hard_link;
 
-  /* This value is used to determine whether to prompt before removing
-     each existing destination file.  It works differently depending on
-     whether move_mode is set.  See code/comments in copy.c.  */
-  enum Interactive interactive;
-
   /* If true, rather than copying, first attempt to use rename.
      If that fails, then resort to copying.  */
   bool move_mode;
@@ -168,13 +175,6 @@ struct cp_options
      set it based on current umask modified by UMASK_KILL.  */
   bool set_mode;
 
-  /* Set the mode of the destination file to exactly this value
-     if SET_MODE is nonzero.  */
-  mode_t mode;
-
-  /* Control creation of sparse files.  */
-  enum Sparse_type sparse_mode;
-
   /* If true, create symbolic links instead of copying files.
      Create destination directories as usual. */
   bool symbolic_link;
@@ -189,6 +189,11 @@ struct cp_options
   /* If true, stdin is a tty.  */
   bool stdin_tty;
 
+  /* If true, open a dangling destination symlink when not in move_mode.
+     Otherwise, copy_reg gives a diagnostic (it refuses to write through
+     such a symlink) and returns false.  */
+  bool open_dangling_dest_symlink;
+
   /* This is a set of destination name/inode/dev triples.  Each such triple
      represents a file we have created corresponding to a source file name
      that was specified on the command line.  Use it to avoid clobbering
diff --git a/src/cp.c b/src/cp.c
index 625376b53..5859f8c1d 100644
--- a/src/cp.c
+++ b/src/cp.c
@@ -761,6 +761,13 @@ cp_option_init (struct cp_options *x)
 
   x->update = false;
   x->verbose = false;
+
+  /* By default, refuse to open a dangling destination symlink, because
+     in general one cannot do that safely, give the current semantics of
+     open's O_EXCL flag, (which POSIX doesn't even allow cp to use, btw).
+     But POSIX requires it.  */
+  x->open_dangling_dest_symlink = getenv ("POSIXLY_CORRECT") != NULL;
+
   x->dest_info = NULL;
   x->src_info = NULL;
 }
diff --git a/src/install.c b/src/install.c
index 8e4875844..802dfcf77 100644
--- a/src/install.c
+++ b/src/install.c
@@ -190,6 +190,7 @@ cp_option_init (struct cp_options *x)
   x->mode = S_IRUSR | S_IWUSR;
   x->stdin_tty = false;
 
+  x->open_dangling_dest_symlink = false;
   x->update = false;
   x->preserve_security_context = false;
   x->verbose = false;
diff --git a/src/mv.c b/src/mv.c
index 8b70d0048..3b1ba8e9e 100644
--- a/src/mv.c
+++ b/src/mv.c
@@ -146,6 +146,7 @@ cp_option_init (struct cp_options *x)
   x->mode = 0;
   x->stdin_tty = isatty (STDIN_FILENO);
 
+  x->open_dangling_dest_symlink = false;
   x->update = false;
   x->verbose = false;
   x->dest_info = NULL;
diff --git a/tests/cp/thru-dangling b/tests/cp/thru-dangling
index 0503af943..0256a167c 100755
--- a/tests/cp/thru-dangling
+++ b/tests/cp/thru-dangling
@@ -1,5 +1,5 @@
 #!/bin/sh
-# Ensure that cp works when the destination is a dangling symlink
+# Ensure that cp works as documented, when the destination is a dangling symlink
 
 # Copyright (C) 2007 Free Software Foundation, Inc.
 
@@ -26,10 +26,19 @@ fi
 ln -s no-such dangle || framework_failure
 echo hi > f || framework_failure
 echo hi > exp || framework_failure
+echo "cp: not writing through dangling symlink \`dangle'" \
+    > exp-err || framework_failure
 
 fail=0
 
-cp f dangle > out 2>&1 || fail=1
+# Starting with 6.9.90, this usage fails, by default:
+cp f dangle > err 2>&1 && fail=1
+
+compare err exp-err || fail=1
+test -f no-such && fail=1
+
+# But you can set POSIXLY_CORRECT to get the historical behavior.
+POSIXLY_CORRECT=1 cp f dangle > out 2>&1 || fail=1
 cat no-such >> out || fail=1
 
 compare out exp || fail=1