Mike Crowe wrote:
Do you think that if I added such a limit and diagnostic then the patch
would be acceptable?
I'd rather fix the underlying problem more systematically. How about the
attached patch instead? I've installed this on Savannah so you can give it a
whirl easily. It fixes the problem for me so I'm boldly marking this bug report
as done; we can unmark it later if it turns out that I'm wrong.
>From 376967889ed7ed561e46ff6d88a66779db62737a Mon Sep 17 00:00:00 2001
From: Paul Eggert <egg...@cs.ucla.edu>
Date: Sat, 11 Feb 2017 23:12:31 -0800
Subject: [PATCH] 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.
---
NEWS | 10 +++
bootstrap.conf | 2 +-
doc/coreutils.texi | 18 +++--
src/copy.c | 96 +++++++++++---------------
src/cp.c | 5 --
src/force-link.c | 187 ++++++++++++++++++++++++++++++++++++++++++++++++++
src/force-link.h | 3 +
src/ln.c | 31 +++------
src/local.mk | 8 ++-
tests/cp/same-file.sh | 2 +-
10 files changed, 268 insertions(+), 94 deletions(-)
create mode 100644 src/force-link.c
create mode 100644 src/force-link.h
diff --git a/NEWS b/NEWS
index deaab7b..7473e6e 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 59b3a91..acec6f0 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 3eac96b..2dbfcce 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 e3832c2..9dbd536 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 0805558..88db3a3 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 0000000..e0db075
--- /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 0000000..2a690f6
--- /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 bacf5f8..539d238 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 5b25fcb..84df099 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 978bba8..9aa6a21 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)
--
2.7.4
