ln: replace destination links more atomically

If the file B already exists, commands like 'ln -f A B' and
'cp -fl A B' no longer remove B before creating the new link.
Instead, they arrange for the new link to replace B atomically.
This should fix a race condition reported by Mike Crowe (Bug#25680).
* NEWS, doc/coreutils.texi (cp invocation, ln invocation):
Document this.
* bootstrap.conf (gnulib_modules): Add symlinkat.
* src/copy.c, src/ln.c: Include force-link.h.
* src/copy.c (same_file_ok): It's also OK to remove a destination
symlink when creating symbolic links, or when the source and
destination are on the same file system and when creating hard links.
* src/copy.c (create_hard_link, copy_internal):
* src/ln.c (do_link):
Rewrite using force_linkat and force_symlinkat, to close a window
where the destination temporarily does not exist.
* src/cp.c (main): Do not set x.unlink_dest_before_opening
merely because we are in link-creation mode.
* src/force-link.c, src/force-link.h: New files.
* src/local.mk (copy_sources, src_ln_SOURCES): Add them.
* tests/cp/same-file.sh: Adjust test case to match fixed behavior.

10 files changed, 268 insertions, 94 deletions

diff --git a/NEWS b/NEWS
index deaab7bcf..7473e6e4a 100644
--- a/NEWS
+++ b/NEWS
@@ -2,12 +2,22 @@ GNU coreutils NEWS                                    -*- outline -*-
 
 * Noteworthy changes in release ?.? (????-??-??) [?]
 
+** Improvements
+
+  If the file B already exists, commands like 'ln -f A B' and
+  'cp -fl A B' no longer remove B before creating the new link.
+  That is, there is no longer a brief moment when B does not exist.
+
 ** Bug fixes
 
   date again converts from a specified time zone.  Previously output was
   not converted to the local time zone, and remained in the specified one.
   [bug introduced in coreutils-8.26]
 
+  Commands like 'cp --no-dereference -l A B' are no longer quiet no-ops
+  when A is a regular file and B is a symbolic link that points to A.
+  [bug introduced in fileutils-4.0]
+
   factor no longer goes into an infinite loop for certain numbers like
   158909489063877810457 and 222087527029934481871.
   [bug introduced in coreutils-8.20]
diff --git a/bootstrap.conf b/bootstrap.conf
index 59b3a916e..acec6f08c 100644
--- a/bootstrap.conf
+++ b/bootstrap.conf
@@ -234,7 +234,7 @@ gnulib_modules="
   strtod
   strtoimax
   strtoumax
-  symlink
+  symlinkat
   sys_ioctl
   sys_resource
   sys_stat
diff --git a/doc/coreutils.texi b/doc/coreutils.texi
index 3eac96b7d..2dbfccecc 100644
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -8125,9 +8125,11 @@ Equivalent to @option{--no-dereference --preserve=links}.
 When copying without this option and an existing destination file cannot
 be opened for writing, the copy fails.  However, with @option{--force},
 when a destination file cannot be opened, @command{cp} then removes it and
-tries to open it again.  Contrast this behavior with that enabled by
-@option{--link} and @option{--symbolic-link}, whereby the destination file
-is never opened but rather is removed unconditionally.  Also see the
+tries to open it again.  When this option is combined with
+@option{--link} (@option{-l}) or @option{--symbolic-link}
+(@option{-s}), the destination link is replaced, and unless
+@option{--backup} (@option{-b}) is also given there is no brief
+moment when the destination does not exist.  Also see the
 description of @option{--remove-destination}.
 
 This option is independent of the @option{--interactive} or
@@ -9825,11 +9827,13 @@ directory, using the @var{target}s' names.
 
 @end itemize
 
-Normally @command{ln} does not remove existing files.  Use the
-@option{--force} (@option{-f}) option to remove them unconditionally,
-the @option{--interactive} (@option{-i}) option to remove them
+Normally @command{ln} does not replace existing files.  Use the
+@option{--force} (@option{-f}) option to replace them unconditionally,
+the @option{--interactive} (@option{-i}) option to replace them
 conditionally, and the @option{--backup} (@option{-b}) option to
-rename them.
+rename them.  Unless the @option{--backup} (@option{-b}) option is
+used there is no brief moment when the destination does not exist;
+this is an extension to POSIX.
 
 @cindex hard link, defined
 @cindex inode, and hard links
diff --git a/src/copy.c b/src/copy.c
index e3832c23a..9dbd53655 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -46,6 +46,7 @@
 #include "file-set.h"
 #include "filemode.h"
 #include "filenamecat.h"
+#include "force-link.h"
 #include "full-write.h"
 #include "hash.h"
 #include "hash-triple.h"
@@ -1623,11 +1624,13 @@ same_file_ok (char const *src_name, struct stat const *src_sb,
         }
     }
 
-  /* It's ok to remove a destination symlink.  But that works only when we
-     unlink before opening the destination and when the source and destination
-     files are on the same partition.  */
-  if (x->unlink_dest_before_opening
-      && S_ISLNK (dst_sb_link->st_mode))
+  /* It's ok to remove a destination symlink.  But that works only
+     when creating symbolic links, or when the source and destination
+     are on the same file system and when creating hard links or when
+     unlinking before opening the destination.  */
+  if (x->symbolic_link
+      || ((x->hard_link || x->unlink_dest_before_opening)
+          && S_ISLNK (dst_sb_link->st_mode)))
     return dst_sb_link->st_dev == src_sb_link->st_dev;
 
   if (x->dereference == DEREF_NEVER)
@@ -1779,36 +1782,17 @@ static bool
 create_hard_link (char const *src_name, char const *dst_name,
                   bool replace, bool verbose, bool dereference)
 {
-  /* We want to guarantee that symlinks are not followed, unless requested.  */
-  int flags = 0;
-  if (dereference)
-    flags = AT_SYMLINK_FOLLOW;
-
-  bool link_failed = (linkat (AT_FDCWD, src_name, AT_FDCWD, dst_name, flags)
-                      != 0);
-
-  /* If the link failed because of an existing destination,
-     remove that file and then call link again.  */
-  if (link_failed && replace && errno == EEXIST)
-    {
-      if (unlink (dst_name) != 0)
-        {
-          error (0, errno, _("cannot remove %s"), quoteaf (dst_name));
-          return false;
-        }
-      if (verbose)
-        printf (_("removed %s\n"), quoteaf (dst_name));
-      link_failed = (linkat (AT_FDCWD, src_name, AT_FDCWD, dst_name, flags)
-                     != 0);
-    }
-
-  if (link_failed)
+  int status = force_linkat (AT_FDCWD, src_name, AT_FDCWD, dst_name,
+                             dereference ? AT_SYMLINK_FOLLOW : 0,
+                             replace);
+  if (status < 0)
     {
       error (0, errno, _("cannot create hard link %s to %s"),
              quoteaf_n (0, dst_name), quoteaf_n (1, src_name));
       return false;
     }
-
+  if (0 < status && verbose)
+    printf (_("removed %s\n"), quoteaf (dst_name));
   return true;
 }
 
@@ -2592,7 +2576,9 @@ copy_internal (char const *src_name, char const *dst_name,
               goto un_backup;
             }
         }
-      if (symlink (src_name, dst_name) != 0)
+      if (force_symlinkat (src_name, AT_FDCWD, dst_name,
+                           x->unlink_dest_after_failed_open)
+          < 0)
         {
           error (0, errno, _("cannot create symbolic link %s to %s"),
                  quoteaf_n (0, dst_name), quoteaf_n (1, src_name));
@@ -2617,7 +2603,9 @@ copy_internal (char const *src_name, char const *dst_name,
            && !(! CAN_HARDLINK_SYMLINKS && S_ISLNK (src_mode)
                 && x->dereference == DEREF_NEVER))
     {
-      if (! create_hard_link (src_name, dst_name, false, false, dereference))
+      if (! create_hard_link (src_name, dst_name,
+                              x->unlink_dest_after_failed_open,
+                              false, dereference))
         goto un_backup;
     }
   else if (S_ISREG (src_mode)
@@ -2671,33 +2659,31 @@ copy_internal (char const *src_name, char const *dst_name,
           goto un_backup;
         }
 
-      if (symlink (src_link_val, dst_name) == 0)
-        free (src_link_val);
-      else
+      int symlink_r = force_symlinkat (src_link_val, AT_FDCWD, dst_name,
+                                       x->unlink_dest_after_failed_open);
+      int symlink_err = symlink_r < 0 ? errno : 0;
+      if (symlink_err && x->update && !new_dst && S_ISLNK (dst_sb.st_mode)
+          && dst_sb.st_size == strlen (src_link_val))
         {
-          int saved_errno = errno;
-          bool same_link = false;
-          if (x->update && !new_dst && S_ISLNK (dst_sb.st_mode)
-              && dst_sb.st_size == strlen (src_link_val))
+          /* See if the destination is already the desired symlink.
+             FIXME: This behavior isn't documented, and seems wrong
+             in some cases, e.g., if the destination symlink has the
+             wrong ownership, permissions, or timestamps.  */
+          char *dest_link_val =
+            areadlink_with_size (dst_name, dst_sb.st_size);
+          if (dest_link_val)
             {
-              /* See if the destination is already the desired symlink.
-                 FIXME: This behavior isn't documented, and seems wrong
-                 in some cases, e.g., if the destination symlink has the
-                 wrong ownership, permissions, or timestamps.  */
-              char *dest_link_val =
-                areadlink_with_size (dst_name, dst_sb.st_size);
-              if (dest_link_val && STREQ (dest_link_val, src_link_val))
-                same_link = true;
+              if (STREQ (dest_link_val, src_link_val))
+                symlink_err = 0;
               free (dest_link_val);
             }
-          free (src_link_val);
-
-          if (! same_link)
-            {
-              error (0, saved_errno, _("cannot create symbolic link %s"),
-                     quoteaf (dst_name));
-              goto un_backup;
-            }
+        }
+      free (src_link_val);
+      if (symlink_err)
+        {
+          error (0, symlink_err, _("cannot create symbolic link %s"),
+                 quoteaf (dst_name));
+          goto un_backup;
         }
 
       if (x->preserve_security_context)
diff --git a/src/cp.c b/src/cp.c
index 08055581a..88db3a3e9 100644
--- a/src/cp.c
+++ b/src/cp.c
@@ -1155,11 +1155,6 @@ main (int argc, char **argv)
   if (x.recursive)
     x.copy_as_regular = copy_contents;
 
-  /* If --force (-f) was specified and we're in link-creation mode,
-     first remove any existing destination file.  */
-  if (x.unlink_dest_after_failed_open && (x.hard_link || x.symbolic_link))
-    x.unlink_dest_before_opening = true;
-
   /* Ensure -Z overrides -a.  */
   if ((x.set_security_context || scontext)
       && ! x.require_preserve_context)
diff --git a/src/force-link.c b/src/force-link.c
new file mode 100644
index 000000000..e0db07574
--- /dev/null
+++ b/src/force-link.c
@@ -0,0 +1,187 @@
+/* Implement ln -f "atomically"
+
+   Copyright 2017 Free Software Foundation, Inc.
+
+   This program is free software: you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation, either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* Written by Paul Eggert.  */
+
+/* A naive "ln -f A B" unlinks B and then links A to B.  This module
+   instead links A to a randomly-named temporary T in B's directory,
+   and then renames T to B.  This approach has a window with a
+   randomly-named temporary, which is safer for many applications than
+   a window where B does not exist.  */
+
+#include <config.h>
+
+#include "force-link.h"
+
+#include <dirname.h>
+#include <tempname.h>
+
+#include <errno.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+
+/* A basename pattern suitable for a temporary file.  It should work
+   even on file systems like FAT that support only short names.
+   "Cu" is short for "Coreutils" or for "Changeable unstable",
+   take your pick....  */
+
+static char const simple_pattern[] = "CuXXXXXX";
+enum { x_suffix_len = sizeof "XXXXXX" - 1 };
+
+/* A size for smallish buffers containing file names.  Longer file
+   names can use malloc.  */
+
+enum { smallsize = 256 };
+
+/* Return a template for a file in the same directory as DSTNAME.
+   Use BUF if the template fits, otherwise use malloc and return NULL
+   (setting errno) if unsuccessful.  */
+
+static char *
+samedir_template (char const *dstname, char buf[smallsize])
+{
+  ptrdiff_t dstdirlen = last_component (dstname) - dstname;
+  size_t dsttmpsize = dstdirlen + sizeof simple_pattern;
+  char *dsttmp;
+  if (dsttmpsize <= smallsize)
+    dsttmp = buf;
+  else
+    {
+      dsttmp = malloc (dsttmpsize);
+      if (!dsttmp)
+        return dsttmp;
+    }
+  strcpy (mempcpy (dsttmp, dstname, dstdirlen), simple_pattern);
+  return dsttmp;
+}
+
+
+/* Auxiliaries for force_linkat.  */
+
+struct link_arg
+{
+  int srcdir;
+  char const *srcname;
+  int dstdir;
+  int flags;
+};
+
+static int
+try_link (char *dest, void *arg)
+{
+  struct link_arg *a = arg;
+  return linkat (a->srcdir, a->srcname, a->dstdir, dest, a->flags);
+}
+
+/* Hard-link directory SRCDIR's file SRCNAME to directory DSTDIR's
+   file DSTNAME, using linkat-style FLAGS to control the linking.
+   If FORCE and DSTNAME already exists, replace it atomically.  Return
+   1 if successful and DSTNAME already existed,
+   0 if successful and DSTNAME did not already exist, and
+   -1 (setting errno) on failure.  */
+int
+force_linkat (int srcdir, char const *srcname,
+              int dstdir, char const *dstname, int flags, bool force)
+{
+  int r = linkat (srcdir, srcname, dstdir, dstname, flags);
+  if (!force || r == 0 || errno != EEXIST)
+    return r;
+
+  char buf[smallsize];
+  char *dsttmp = samedir_template (dstname, buf);
+  if (! dsttmp)
+    return -1;
+  struct link_arg arg = { srcdir, srcname, dstdir, flags };
+  int err;
+
+  if (try_tempname_len (dsttmp, 0, &arg, try_link, x_suffix_len) != 0)
+    err = errno;
+  else
+    {
+      err = renameat (dstdir, dsttmp, dstdir, dstname) == 0 ? 0 : errno;
+      /* Unlink DSTTMP even if renameat succeeded, in case DSTTMP
+         and DSTNAME were already the same hard link and renameat
+         was a no-op.  */
+      unlinkat (dstdir, dsttmp, 0);
+    }
+
+  if (dsttmp != buf)
+    free (dsttmp);
+  if (!err)
+    return 1;
+  errno = err;
+  return -1;
+}
+
+
+/* Auxiliaries for force_symlinkat.  */
+
+struct symlink_arg
+{
+  char const *srcname;
+  int dstdir;
+};
+
+static int
+try_symlink (char *dest, void *arg)
+{
+  struct symlink_arg *a = arg;
+  return symlinkat (a->srcname, a->dstdir, dest);
+}
+
+/* Create a symlink containing SRCNAME in directory DSTDIR's file DSTNAME.
+   If FORCE and DSTNAME already exists, replace it atomically.  Return
+   1 if successful and DSTNAME already existed,
+   0 if successful and DSTNAME did not already exist, and
+   -1 (setting errno) on failure.  */
+int
+force_symlinkat (char const *srcname, int dstdir, char const *dstname,
+                 bool force)
+{
+  int r = symlinkat (srcname, dstdir, dstname);
+  if (!force || r == 0 || errno != EEXIST)
+    return r;
+
+  char buf[smallsize];
+  char *dsttmp = samedir_template (dstname, buf);
+  if (!dsttmp)
+    return -1;
+  struct symlink_arg arg = { srcname, dstdir };
+  int err;
+
+  if (try_tempname_len (dsttmp, 0, &arg, try_symlink, x_suffix_len) != 0)
+    err = errno;
+  else if (renameat (dstdir, dsttmp, dstdir, dstname) != 0)
+    {
+      err = errno;
+      unlinkat (dstdir, dsttmp, 0);
+    }
+  else
+    {
+      /* Don't worry about renameat being a no-op, since DSTTMP is
+         newly created.  */
+      err = 0;
+    }
+
+  if (dsttmp != buf)
+    free (dsttmp);
+  if (!err)
+    return 1;
+  errno = err;
+  return -1;
+}
diff --git a/src/force-link.h b/src/force-link.h
new file mode 100644
index 000000000..2a690f680
--- /dev/null
+++ b/src/force-link.h
@@ -0,0 +1,3 @@
+#include <stdbool.h>
+extern int force_linkat (int, char const *, int, char const *, int, bool);
+extern int force_symlinkat (char const *, int, char const *, bool);
diff --git a/src/ln.c b/src/ln.c
index bacf5f8d8..539d238c8 100644
--- a/src/ln.c
+++ b/src/ln.c
@@ -27,6 +27,7 @@
 #include "error.h"
 #include "filenamecat.h"
 #include "file-set.h"
+#include "force-link.h"
 #include "hash.h"
 #include "hash-triple.h"
 #include "relpath.h"
@@ -183,7 +184,6 @@ do_link (const char *source, const char *dest)
   char *rel_source = NULL;
   bool dest_lstat_ok = false;
   bool source_is_dir = false;
-  bool ok;
 
   if (!symbolic_link)
     {
@@ -301,12 +301,7 @@ do_link (const char *source, const char *dest)
   if (relative)
     source = rel_source = convert_abs_rel (source, dest);
 
-  ok = ((symbolic_link ? symlink (source, dest)
-         : linkat (AT_FDCWD, source, AT_FDCWD, dest,
-                   logical ? AT_SYMLINK_FOLLOW : 0))
-        == 0);
-
-  /* If the attempt to create a link failed and we are removing or
+  /* If the attempt to create a link fails and we are removing or
      backing up destinations, unlink the destination and try again.
 
      On the surface, POSIX describes an algorithm that states that
@@ -324,22 +319,12 @@ do_link (const char *source, const char *dest)
      that refer to the same file), rename succeeds and DEST remains.
      If we didn't remove DEST in that case, the subsequent symlink or link
      call would fail.  */
-
-  if (!ok && errno == EEXIST && (remove_existing_files || dest_backup))
-    {
-      if (unlink (dest) != 0)
-        {
-          error (0, errno, _("cannot remove %s"), quoteaf (dest));
-          free (dest_backup);
-          free (rel_source);
-          return false;
-        }
-
-      ok = ((symbolic_link ? symlink (source, dest)
-             : linkat (AT_FDCWD, source, AT_FDCWD, dest,
-                       logical ? AT_SYMLINK_FOLLOW : 0))
-            == 0);
-    }
+  bool ok_to_remove = remove_existing_files || dest_backup;
+  bool ok = 0 <= (symbolic_link
+                  ? force_symlinkat (source, AT_FDCWD, dest, ok_to_remove)
+                  : force_linkat (AT_FDCWD, source, AT_FDCWD, dest,
+                                  logical ? AT_SYMLINK_FOLLOW : 0,
+                                  ok_to_remove));
 
   if (ok)
     {
diff --git a/src/local.mk b/src/local.mk
index 5b25fcb87..84df09916 100644
--- a/src/local.mk
+++ b/src/local.mk
@@ -328,7 +328,9 @@ copy_sources = \
   src/copy.c \
   src/cp-hash.c \
   src/extent-scan.c \
-  src/extent-scan.h
+  src/extent-scan.h \
+  src/force-link.c \
+  src/force-link.h
 
 # Use 'ginstall' in the definition of PROGRAMS and in dependencies to avoid
 # confusion with the 'install' target.  The install rule transforms 'ginstall'
@@ -357,7 +359,9 @@ src_vdir_SOURCES = src/ls.c src/ls-vdir.c
 src_id_SOURCES = src/id.c src/group-list.c
 src_groups_SOURCES = src/groups.c src/group-list.c
 src_ls_SOURCES = src/ls.c src/ls-ls.c
-src_ln_SOURCES = src/ln.c src/relpath.c src/relpath.h
+src_ln_SOURCES = src/ln.c \
+  src/force-link.c src/force-link.h \
+  src/relpath.c src/relpath.h
 src_chown_SOURCES = src/chown.c src/chown-core.c
 src_chgrp_SOURCES = src/chgrp.c src/chown-core.c
 src_kill_SOURCES = src/kill.c src/operand2sig.c
diff --git a/tests/cp/same-file.sh b/tests/cp/same-file.sh
index 978bba81b..9aa6a2112 100755
--- a/tests/cp/same-file.sh
+++ b/tests/cp/same-file.sh
@@ -142,7 +142,7 @@ cat <<\EOF | sed "$remove_these_sed" > expected
 0 -bf (foo symlink symlink.~1~ -> foo)
 0 -bdf (foo symlink symlink.~1~ -> foo)
 1 -l [cp: cannot create hard link 'symlink' to 'foo'] (foo symlink -> foo)
-0 -dl (foo symlink -> foo)
+1 -dl [cp: cannot create hard link 'symlink' to 'foo'] (foo symlink -> foo)
 0 -fl (foo symlink)
 0 -dfl (foo symlink)
 0 -bl (foo symlink symlink.~1~ -> foo)