>From 376967889ed7ed561e46ff6d88a66779db62737a Mon Sep 17 00:00:00 2001 From: Paul Eggert 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 address@hidden 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 address@hidden (@option{-l}) or @option{--symbolic-link} +(@option{-s}), the destination link is replaced, and unless address@hidden (@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 address@hidden (@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 address@hidden (@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 . */ + +/* 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 + +#include "force-link.h" + +#include +#include + +#include +#include +#include +#include + +/* 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 +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