Re: FYI: bug-fix: cp would fail to write through a dangling symlink

[Paul Eggert]

Jim Meyering <address@hidden> writes:

>> Yes, it's total overkill for this application.  Another thing: we
>> don't need to resolve internal symlinks, just the last one.
>
> Why would we want to resolve *any* of them, if we can get
> the dir-fd, ent-name pair I mentioned?

Sorry, I lost context; but I take your point to mean is that if the
destination /x/y/z is a symlink to /a/b/c, and /a/b/c is a symlink to
/d/e/f, and /d/e/f is not a symlink, then we want to open /d/e/f with
O_EXCL.  We don't care whether /x or /x/y or /a or /a/b or /d or /d/e
are symlinks.  But canonicalize.c cares, and does more work than we
want.

>> were based on some existing coreutils code that I think is wrong in
>> this area as well.
>
> Oh!  Then I'll hunt for it tomorrow if you don't tell first.

If you like, here's the code I have right now: look for the patches
that replace XSTAT to see the problem areas.  The XSTAT part of this
patch is clearly incorrect (it doesn't pass the test cases) but the
rest of the patch feels "right" and I hope the XSTAT part is on the
right track.  If I make further progress I'll let you know but I have
to work on other stuff right now.


2007-06-13  Paul Eggert  <address@hidden>
        and Jim Meyering  <address@hidden>

        * src/copy.c: Include "canonicalize.h".
        (copy_reg): When following a symlink, used the followed name in
        later chown etc. requests, so that the created file is affected,
        rather than the symlink.  Use O_NOFOLLOW on source when not
        dereferencing symlinks; this avoids a race.  Preserve errno
        correctly when doing multiple open attempts on the destination.
        Use canonicalize_filename_mode to follow the symlink, so that we
        can always open with O_EXCL and avoid a race.
        (copy_internal): Follow destination symlinks when copying a regular
        file, regardless of whether following source symlinks, since POSIX
        and tradition says we should always follow destination symlinks if
        the system calls would ordinarily do so.
        * src/cp.c (make_dir_parents_private): Likewise.

        THIS PATCH IS NOT CORRECT.  <-- (warning if you didn't read the above)

diff --git a/src/copy.c b/src/copy.c
index b46221c..ba61ef4 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -34,6 +34,7 @@
 #include "acl.h"
 #include "backupfile.h"
 #include "buffer-lcm.h"
+#include "canonicalize.h"
 #include "copy.h"
 #include "cp-hash.h"
 #include "euidaccess.h"
@@ -261,14 +262,19 @@ 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;
   mode_t src_mode = src_sb->st_mode;
   struct stat sb;
   struct stat src_open_sb;
   bool return_val = true;
 
-  source_desc = open (src_name, O_RDONLY | O_BINARY);
+  source_desc = open (src_name,
+                     (O_RDONLY | O_BINARY
+                      | (x->dereference == DEREF_NEVER ? O_NOFOLLOW : 0)));
   if (source_desc < 0)
     {
       error (0, errno, _("cannot open %s for reading"), quote (src_name));
@@ -298,6 +304,7 @@ copy_reg (char const *src_name, char const *dst_name,
   if (! *new_dst)
     {
       dest_desc = open (dst_name, O_WRONLY | O_TRUNC | O_BINARY);
+      dest_errno = errno;
 
       /* When using cp --preserve=context to copy to an existing destination,
         use the default context rather than that of the source.  Why?
@@ -353,21 +360,35 @@ copy_reg (char const *src_name, char const *dst_name,
 
   if (*new_dst)
     {
-      int open_flags = O_WRONLY | O_CREAT | O_BINARY;
-      dest_desc = open (dst_name, open_flags | O_EXCL,
+      int open_flags = O_WRONLY | O_CREAT | O_EXCL | O_BINARY;
+      dest_desc = open (dst_name, open_flags,
                        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 without O_EXCL.  */
-      if (dest_desc < 0 && errno == EEXIST)
+        the open call, but this time with the name of the final,
+        missing directory entry.  */
+      if (dest_desc < 0 && dest_errno == EEXIST)
        {
          struct stat dangling_link_sb;
          if (lstat (dst_name, &dangling_link_sb) == 0
              && S_ISLNK (dangling_link_sb.st_mode))
-           dest_desc = open (dst_name, open_flags,
-                             dst_mode & ~omitted_permissions);
+           {
+             /* 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)
+               {
+                 followed_dest_name = name_alloc;
+                 dest_desc = open (followed_dest_name, open_flags,
+                                   dst_mode & ~omitted_permissions);
+                 dest_errno = errno;
+               }
+           }
        }
     }
   else
@@ -375,7 +396,8 @@ copy_reg (char const *src_name, char const *dst_name,
 
   if (dest_desc < 0)
     {
-      error (0, errno, _("cannot create regular file %s"), quote (dst_name));
+      error (0, dest_errno, _("cannot create regular file %s"),
+            quote (dst_name));
       return_val = false;
       goto close_src_desc;
     }
@@ -567,7 +589,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, dst_name, timespec) != 0)
+      if (gl_futimens (dest_desc, followed_dest_name, timespec) != 0)
        {
          error (0, errno, _("preserving times for %s"), quote (dst_name));
          if (x->require_preserve)
@@ -580,7 +602,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, dst_name, dest_desc,
+      switch (set_owner (x, followed_dest_name, dest_desc,
                         src_sb->st_uid, src_sb->st_gid))
        {
        case -1:
@@ -593,24 +615,24 @@ copy_reg (char const *src_name, char const *dst_name,
        }
     }
 
-  set_author (dst_name, dest_desc, src_sb);
+  set_author (followed_dest_name, dest_desc, src_sb);
 
   if (x->preserve_mode || x->move_mode)
     {
-      if (copy_acl (src_name, source_desc, dst_name, dest_desc, src_mode) != 0
+      if (copy_acl (src_name, source_desc, followed_dest_name, dest_desc, 
src_mode) != 0
          && x->require_preserve)
        return_val = false;
     }
   else if (x->set_mode)
     {
-      if (set_acl (dst_name, dest_desc, x->mode) != 0)
+      if (set_acl (followed_dest_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, dst_name, dst_mode) != 0)
+         && fchmod_or_lchmod (dest_desc, followed_dest_name, dst_mode) != 0)
        {
          error (0, errno, _("preserving permissions for %s"),
                 quote (dst_name));
@@ -633,6 +655,7 @@ close_src_desc:
     }
 
   free (buf_alloc);
+  free (name_alloc);
   return return_val;
 }
 
@@ -1137,7 +1160,14 @@ copy_internal (char const *src_name, char const 
*dst_name,
 
   if (!new_dst)
     {
-      if (XSTAT (x, dst_name, &dst_sb) != 0)
+      /* Regular files can be created by writing through symbolic
+        links, but other files cannot.  So use stat on the
+        destination when copying a regular file, and lstat
+        otherwise.  */
+      if ((S_ISREG (src_mode)
+          ? stat (dst_name, &dst_sb)
+          : lstat (dst_name, &dst_sb))
+         != 0)
        {
          if (errno != ENOENT)
            {
@@ -1151,7 +1181,7 @@ copy_internal (char const *src_name, char const *dst_name,
        }
       else
        { /* Here, we know that dst_name exists, at least to the point
-            that it is XSTAT'able.  */
+            that it is stat'able or lstat'table.  */
          bool return_now;
          bool unlink_src;
 
diff --git a/src/copy.h b/src/copy.h
diff --git a/src/cp.c b/src/cp.c
index 35c487b..8b90eba 100644
--- a/src/cp.c
+++ b/src/cp.c
@@ -394,7 +394,7 @@ make_dir_parents_private (char const *const_dir, size_t 
src_offset,
 
   *attr_list = NULL;
 
-  if (XSTAT (x, dst_dir, &stats))
+  if (stat (dst_dir, &stats) != 0)
     {
       /* A parent of CONST_DIR does not exist.
         Make all missing intermediate directories. */
@@ -414,7 +414,7 @@ make_dir_parents_private (char const *const_dir, size_t 
src_offset,
          *attr_list = new;
 
          *slash = '\0';
-         if (XSTAT (x, dir, &stats))
+         if (stat (dir, &stats) != 0)
            {
              mode_t src_mode;
              mode_t omitted_permissions;