From: Paul Eggert Date: Fri, 19 Oct 2018 19:19:43 +0000 (-0700) Subject: ln: avoid directory hard-link races X-Git-Tag: v8.31~72 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=571f63f5010b047a8a3250304053f05949faded4;p=thirdparty%2Fcoreutils.git ln: avoid directory hard-link races Previously, 'ln A B' did 'stat("B"), lstat("A"), link("A","B")' where the stat and lstat were necessary to avoid hard-linking directories on systems that can hard-link directories. Now, in situations that prohibit hard links to directories, 'ln A B' merely does 'link("A","B")'. The new behavior avoids some races and should be more efficient. This patch was inspired by Bug#10020, which was about 'ln'. * bootstrap.conf (gnulib_modules): Add unlinkdir. * src/force-link.c (force_linkat, force_symlinkat): New arg for error number of previous try. Return error number, 0, or -1 if error, success, or success after removal. All callers changed. * src/ln.c: Include priv-set.h, unlinkdir.h. (beware_hard_dir_link): New static var. (errnoize, atomic_link): New functions. (target_directory_operand): Use errnoize for simplicity. (do_link): New arg for error number of previous try. All callers changed. Do each link atomically if possible. (main): Do -r check earlier. Remove linkdir privileges so we can use a single linkat/symlinkat instead of a racy substitute for the common case of 'ln A B' and 'ln -s A B'. Set beware_hard_dir_link to disable this optimization. --- diff --git a/NEWS b/NEWS index 0fea1a1186..ae9667e61a 100644 --- a/NEWS +++ b/NEWS @@ -10,6 +10,12 @@ GNU coreutils NEWS -*- outline -*- df no longer corrupts displayed multibyte characters on macOS. + When possible 'ln A B' now merely links A to B and reports an error + if this fails, instead of statting A and B before linking. This + uses fewer system calls and avoids some races. The old statting + approach is still used in situations where hard links to directories + are allowed (e.g., NetBSD when superuser). + ** New features id now supports specifying multiple users. diff --git a/bootstrap.conf b/bootstrap.conf index fcf29dc231..f193a5825c 100644 --- a/bootstrap.conf +++ b/bootstrap.conf @@ -257,6 +257,7 @@ gnulib_modules=" unistd-safer unlink-busy unlinkat + unlinkdir unlocked-io unsetenv update-copyright diff --git a/src/copy.c b/src/copy.c index 417147bc2b..1f4d47737d 100644 --- a/src/copy.c +++ b/src/copy.c @@ -1783,16 +1783,16 @@ static bool create_hard_link (char const *src_name, char const *dst_name, bool replace, bool verbose, bool dereference) { - int status = force_linkat (AT_FDCWD, src_name, AT_FDCWD, dst_name, - dereference ? AT_SYMLINK_FOLLOW : 0, - replace); - if (status < 0) + int err = force_linkat (AT_FDCWD, src_name, AT_FDCWD, dst_name, + dereference ? AT_SYMLINK_FOLLOW : 0, + replace, -1); + if (0 < err) { - error (0, errno, _("cannot create hard link %s to %s"), + error (0, err, _("cannot create hard link %s to %s"), quoteaf_n (0, dst_name), quoteaf_n (1, src_name)); return false; } - if (0 < status && verbose) + if (err < 0 && verbose) printf (_("removed %s\n"), quoteaf (dst_name)); return true; } @@ -2630,11 +2630,12 @@ copy_internal (char const *src_name, char const *dst_name, goto un_backup; } } - if (force_symlinkat (src_name, AT_FDCWD, dst_name, - x->unlink_dest_after_failed_open) - < 0) + + int err = force_symlinkat (src_name, AT_FDCWD, dst_name, + x->unlink_dest_after_failed_open, -1); + if (0 < err) { - error (0, errno, _("cannot create symbolic link %s to %s"), + error (0, err, _("cannot create symbolic link %s to %s"), quoteaf_n (0, dst_name), quoteaf_n (1, src_name)); goto un_backup; } @@ -2713,10 +2714,9 @@ copy_internal (char const *src_name, char const *dst_name, goto un_backup; } - 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) + int symlink_err = force_symlinkat (src_link_val, AT_FDCWD, dst_name, + x->unlink_dest_after_failed_open, -1); + if (0 < symlink_err && 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. @@ -2733,7 +2733,7 @@ copy_internal (char const *src_name, char const *dst_name, } } free (src_link_val); - if (symlink_err) + if (0 < symlink_err) { error (0, symlink_err, _("cannot create symbolic link %s"), quoteaf (dst_name)); diff --git a/src/force-link.c b/src/force-link.c index 1794a5786c..f2242fc360 100644 --- a/src/force-link.c +++ b/src/force-link.c @@ -85,22 +85,27 @@ try_link (char *dest, void *arg) /* 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, + If FORCE and DSTNAME already exists, replace it atomically. + If LINKAT_ERRNO is 0, the hard link is already done; if positive, + the hard link was tried and failed with errno == LINKAT_ERRNO. Return + -1 if successful and DSTNAME already existed, 0 if successful and DSTNAME did not already exist, and - -1 (setting errno) on failure. */ + a positive errno value on failure. */ extern int force_linkat (int srcdir, char const *srcname, - int dstdir, char const *dstname, int flags, bool force) + int dstdir, char const *dstname, int flags, bool force, + int linkat_errno) { - int r = linkat (srcdir, srcname, dstdir, dstname, flags); - if (!force || r == 0 || errno != EEXIST) - return r; + if (linkat_errno < 0) + linkat_errno = (linkat (srcdir, srcname, dstdir, dstname, flags) == 0 + ? 0 : errno); + if (!force || linkat_errno != EEXIST) + return linkat_errno; char buf[smallsize]; char *dsttmp = samedir_template (dstname, buf); if (! dsttmp) - return -1; + return errno; struct link_arg arg = { srcdir, srcname, dstdir, flags }; int err; @@ -108,7 +113,7 @@ force_linkat (int srcdir, char const *srcname, err = errno; else { - err = renameat (dstdir, dsttmp, dstdir, dstname) == 0 ? 0 : errno; + err = renameat (dstdir, dsttmp, dstdir, dstname) == 0 ? -1 : errno; /* Unlink DSTTMP even if renameat succeeded, in case DSTTMP and DSTNAME were already the same hard link and renameat was a no-op. */ @@ -117,10 +122,7 @@ force_linkat (int srcdir, char const *srcname, if (dsttmp != buf) free (dsttmp); - if (!err) - return 1; - errno = err; - return -1; + return err; } @@ -140,22 +142,25 @@ try_symlink (char *dest, void *arg) } /* 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, + If FORCE and DSTNAME already exists, replace it atomically. + If SYMLINKAT_ERRNO is 0, the symlink is already done; if positive, + the symlink was tried and failed with errno == SYMLINKAT_ERRNO. Return + -1 if successful and DSTNAME already existed, 0 if successful and DSTNAME did not already exist, and - -1 (setting errno) on failure. */ + a positive errno value on failure. */ extern int force_symlinkat (char const *srcname, int dstdir, char const *dstname, - bool force) + bool force, int symlinkat_errno) { - int r = symlinkat (srcname, dstdir, dstname); - if (!force || r == 0 || errno != EEXIST) - return r; + if (symlinkat_errno < 0) + symlinkat_errno = symlinkat (srcname, dstdir, dstname) == 0 ? 0 : errno; + if (!force || symlinkat_errno != EEXIST) + return symlinkat_errno; char buf[smallsize]; char *dsttmp = samedir_template (dstname, buf); if (!dsttmp) - return -1; + return errno; struct symlink_arg arg = { srcname, dstdir }; int err; @@ -170,13 +175,10 @@ force_symlinkat (char const *srcname, int dstdir, char const *dstname, { /* Don't worry about renameat being a no-op, since DSTTMP is newly created. */ - err = 0; + err = -1; } if (dsttmp != buf) free (dsttmp); - if (!err) - return 1; - errno = err; - return -1; + return err; } diff --git a/src/force-link.h b/src/force-link.h index 7ea3817de8..595c93f1a2 100644 --- a/src/force-link.h +++ b/src/force-link.h @@ -1,2 +1,2 @@ -extern int force_linkat (int, char const *, int, char const *, int, bool); -extern int force_symlinkat (char const *, int, char const *, bool); +extern int force_linkat (int, char const *, int, char const *, int, bool, int); +extern int force_symlinkat (char const *, int, char const *, bool, int); diff --git a/src/ln.c b/src/ln.c index 7bbd42f71b..9f197a4b77 100644 --- a/src/ln.c +++ b/src/ln.c @@ -30,8 +30,10 @@ #include "force-link.h" #include "hash.h" #include "hash-triple.h" +#include "priv-set.h" #include "relpath.h" #include "same.h" +#include "unlinkdir.h" #include "yesno.h" #include "canonicalize.h" @@ -69,6 +71,9 @@ static bool verbose; directories on most existing systems (Solaris being an exception). */ static bool hard_dir_link; +/* If true, watch out for creating or removing hard links to directories. */ +static bool beware_hard_dir_link; + /* If nonzero, and the specified destination is a symbolic link to a directory, treat it just as if it were a directory. Otherwise, the command 'ln --force --no-dereference file symlink-to-dir' deletes @@ -113,6 +118,14 @@ errno_nonexisting (int err) return err == ENOENT || err == ENAMETOOLONG || err == ENOTDIR || err == ELOOP; } +/* Return an errno value for a system call that returned STATUS. + This is zero if STATUS is zero, and is errno otherwise. */ + +static int +errnoize (int status) +{ + return status < 0 ? errno : 0; +} /* FILE is the last operand of this command. Return true if FILE is a directory. But report an error if there is a problem accessing FILE, @@ -126,9 +139,8 @@ target_directory_operand (char const *file) size_t blen = strlen (b); bool looks_like_a_dir = (blen == 0 || ISSLASH (b[blen - 1])); struct stat st; - int stat_result = - (dereference_dest_dir_symlinks ? stat (file, &st) : lstat (file, &st)); - int err = (stat_result == 0 ? 0 : errno); + int flags = dereference_dest_dir_symlinks ? 0 : AT_SYMLINK_NOFOLLOW; + int err = errnoize (fstatat (AT_FDCWD, file, &st, flags)); bool is_a_dir = !err && S_ISDIR (st.st_mode); if (err && ! errno_nonexisting (errno)) die (EXIT_FAILURE, err, _("failed to access %s"), quoteaf (file)); @@ -171,162 +183,181 @@ convert_abs_rel (const char *from, const char *target) return relative_from ? relative_from : xstrdup (from); } +/* Link SOURCE to DEST atomically. Return 0 if successful, a positive + errno value on failure, and -1 if an atomic link cannot be done. + This handles the common case where DEST does not already exist and + -r is not specified. */ + +static int +atomic_link (char const *source, char const *dest) +{ + return (symbolic_link + ? (relative ? -1 : errnoize (symlink (source, dest))) + : beware_hard_dir_link + ? -1 + : errnoize (linkat (AT_FDCWD, source, AT_FDCWD, dest, + logical ? AT_SYMLINK_FOLLOW : 0))); +} + /* Make a link DEST to the (usually) existing file SOURCE. Symbolic links to nonexistent files are allowed. + LINK_ERRNO is zero if the link has already been made, + positive if attempting the link failed with errno == LINK_ERRNO, + -1 if no attempt has been made to create the link. Return true if successful. */ static bool -do_link (const char *source, const char *dest) +do_link (const char *source, const char *dest, int link_errno) { struct stat source_stats; - struct stat dest_stats; + int source_status = 1; char *dest_backup = NULL; char *rel_source = NULL; - bool dest_lstat_ok = false; - bool source_is_dir = false; + int nofollow_flag = logical ? 0 : AT_SYMLINK_NOFOLLOW; + if (link_errno < 0) + link_errno = atomic_link (source, dest); - if (!symbolic_link) + /* Get SOURCE_STATS if later code will need it, if only for sharper + diagnostics. */ + if ((link_errno || dest_set) && !symbolic_link) { - /* Which stat to use depends on whether linkat will follow the - symlink. We can't use the shorter - (logical?stat:lstat) (source, &source_stats) - since stat might be a function-like macro. */ - if ((logical ? stat (source, &source_stats) - : lstat (source, &source_stats)) - != 0) + source_status = fstatat (AT_FDCWD, source, &source_stats, nofollow_flag); + if (source_status != 0) { error (0, errno, _("failed to access %s"), quoteaf (source)); return false; } - - if (S_ISDIR (source_stats.st_mode)) - { - source_is_dir = true; - if (! hard_dir_link) - { - error (0, 0, _("%s: hard link not allowed for directory"), - quotef (source)); - return false; - } - } } - if (remove_existing_files || interactive || backup_type != no_backups) + if (link_errno) { - dest_lstat_ok = (lstat (dest, &dest_stats) == 0); - if (!dest_lstat_ok && errno != ENOENT) + if (!symbolic_link && !hard_dir_link && S_ISDIR (source_stats.st_mode)) { - error (0, errno, _("failed to access %s"), quoteaf (dest)); + error (0, 0, _("%s: hard link not allowed for directory"), + quotef (source)); return false; } - } - - /* If the current target was created as a hard link to another - source file, then refuse to unlink it. */ - if (dest_lstat_ok - && dest_set != NULL - && seen_file (dest_set, dest, &dest_stats)) - { - error (0, 0, - _("will not overwrite just-created %s with %s"), - quoteaf_n (0, dest), quoteaf_n (1, source)); - return false; - } - - /* If --force (-f) has been specified without --backup, then before - making a link ln must remove the destination file if it exists. - (with --backup, it just renames any existing destination file) - But if the source and destination are the same, don't remove - anything and fail right here. */ - if ((remove_existing_files - /* Ensure that "ln --backup f f" fails here, with the - "... same file" diagnostic, below. Otherwise, subsequent - code would give a misleading "file not found" diagnostic. - This case is different than the others handled here, since - the command in question doesn't use --force. */ - || (!symbolic_link && backup_type != no_backups)) - && dest_lstat_ok - /* Allow 'ln -sf --backup k k' to succeed in creating the - self-referential symlink, but don't allow the hard-linking - equivalent: 'ln -f k k' (with or without --backup) to get - beyond this point, because the error message you'd get is - misleading. */ - && (backup_type == no_backups || !symbolic_link) - && (!symbolic_link || stat (source, &source_stats) == 0) - && SAME_INODE (source_stats, dest_stats) - /* The following detects whether removing DEST will also remove - SOURCE. If the file has only one link then both are surely - the same link. Otherwise check whether they point to the same - name in the same directory. */ - && (source_stats.st_nlink == 1 || same_name (source, dest))) - { - error (0, 0, _("%s and %s are the same file"), - quoteaf_n (0, source), quoteaf_n (1, dest)); - return false; - } - if (dest_lstat_ok) - { - if (S_ISDIR (dest_stats.st_mode)) - { - error (0, 0, _("%s: cannot overwrite directory"), quotef (dest)); - return false; - } - if (interactive) - { - fprintf (stderr, _("%s: replace %s? "), program_name, quoteaf (dest)); - if (!yesno ()) - return true; - remove_existing_files = true; - } + if (relative) + source = rel_source = convert_abs_rel (source, dest); - if (backup_type != no_backups) + bool force = (remove_existing_files || interactive + || backup_type != no_backups); + if (force) { - dest_backup = find_backup_file_name (dest, backup_type); - if (rename (dest, dest_backup) != 0) + struct stat dest_stats; + if (lstat (dest, &dest_stats) != 0) { - int rename_errno = errno; - free (dest_backup); - dest_backup = NULL; - if (rename_errno != ENOENT) + if (errno != ENOENT) { - error (0, rename_errno, _("cannot backup %s"), - quoteaf (dest)); + error (0, errno, _("failed to access %s"), quoteaf (dest)); return false; } + force = false; + } + else if (S_ISDIR (dest_stats.st_mode)) + { + error (0, 0, _("%s: cannot overwrite directory"), quotef (dest)); + return false; + } + else if (seen_file (dest_set, dest, &dest_stats)) + { + /* The current target was created as a hard link to another + source file. */ + error (0, 0, + _("will not overwrite just-created %s with %s"), + quoteaf_n (0, dest), quoteaf_n (1, source)); + return false; + } + else + { + /* Beware removing DEST if it is the same directory entry as + SOURCE, because in that case removing DEST can cause the + subsequent link creation either to fail (for hard links), or + to replace a non-symlink DEST with a self-loop (for symbolic + links) which loses the contents of DEST. So, when backing + up, worry about creating hard links (since the backups cover + the symlink case); otherwise, worry about about -f. */ + if (backup_type != no_backups + ? !symbolic_link + : remove_existing_files) + { + /* Detect whether removing DEST would also remove SOURCE. + If the file has only one link then both are surely the + same directory entry. Otherwise check whether they point + to the same name in the same directory. */ + if (source_status != 0) + source_status = stat (source, &source_stats); + if (source_status == 0 + && SAME_INODE (source_stats, dest_stats) + && (source_stats.st_nlink == 1 + || same_name (source, dest))) + { + error (0, 0, _("%s and %s are the same file"), + quoteaf_n (0, source), quoteaf_n (1, dest)); + return false; + } + } + + if (link_errno < 0 || link_errno == EEXIST) + { + if (interactive) + { + fprintf (stderr, _("%s: replace %s? "), + program_name, quoteaf (dest)); + if (!yesno ()) + return true; + } + + if (backup_type != no_backups) + { + dest_backup = find_backup_file_name (dest, backup_type); + if (rename (dest, dest_backup) != 0) + { + int rename_errno = errno; + free (dest_backup); + dest_backup = NULL; + if (rename_errno != ENOENT) + { + error (0, rename_errno, _("cannot backup %s"), + quoteaf (dest)); + return false; + } + force = false; + } + } + } } } + + /* 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 states that 'ln -f A B' unlinks B before trying + to link A to B. But strictly following this has the counterintuitive + effect of losing the contents of B if A does not exist. Fortunately, + POSIX 2008 clarified that an application is free to fail early if it + can prove that continuing onwards cannot succeed, so we can try to + link A to B before blindly unlinking B, thus sometimes attempting to + link a second time during a successful 'ln -f A B'. + + Try to unlink DEST even if we may have backed it up successfully. + In some unusual cases (when DEST and DEST_BACKUP are hard-links + 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. */ + link_errno + = (symbolic_link + ? force_symlinkat (source, AT_FDCWD, dest, force, link_errno) + : force_linkat (AT_FDCWD, source, AT_FDCWD, dest, + logical ? AT_SYMLINK_FOLLOW : 0, + force, link_errno)); + /* Until now, link_errno < 0 meant the link has not been tried. + From here on, link_errno < 0 means the link worked but + required removing the destination first. */ } - if (relative) - source = rel_source = convert_abs_rel (source, dest); - - /* 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 - 'ln -f A B' will call unlink() on B before ever attempting - link() on A. But strictly following this has the counterintuitive - effect of losing the contents of B, if A does not exist. - Fortunately, POSIX 2008 clarified that an application is free - to fail early if it can prove that continuing onwards cannot - succeed, so we are justified in trying link() before blindly - removing B, thus sometimes calling link() a second time during - a successful 'ln -f A B'. - - Try to unlink DEST even if we may have backed it up successfully. - In some unusual cases (when DEST and DEST_BACKUP are hard-links - 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. */ - 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) + if (link_errno <= 0) { /* Right after creating a hard link, do this: (note dest name and source_stats, which are also the just-linked-destinations stats) */ @@ -343,15 +374,15 @@ do_link (const char *source, const char *dest) } else { - error (0, errno, + error (0, link_errno, (symbolic_link - ? (errno != ENAMETOOLONG && *source + ? (link_errno != ENAMETOOLONG && *source ? _("failed to create symbolic link %s") : _("failed to create symbolic link %s -> %s")) - : (errno == EMLINK && !source_is_dir + : (link_errno == EMLINK ? _("failed to create hard link to %.0s%s") - : (errno == EDQUOT || errno == EEXIST || errno == ENOSPC - || errno == EROFS) + : (link_errno == EDQUOT || link_errno == EEXIST + || link_errno == ENOSPC || link_errno == EROFS) ? _("failed to create hard link %s") : _("failed to create hard link %s => %s"))), quoteaf_n (0, dest), quoteaf_n (1, source)); @@ -365,7 +396,7 @@ do_link (const char *source, const char *dest) free (dest_backup); free (rel_source); - return ok; + return link_errno <= 0; } void @@ -444,6 +475,7 @@ main (int argc, char **argv) bool no_target_directory = false; int n_files; char **file; + int link_errno = -1; initialize_main (&argc, &argv); set_program_name (argv[0]); @@ -535,6 +567,15 @@ main (int argc, char **argv) usage (EXIT_FAILURE); } + if (relative && !symbolic_link) + die (EXIT_FAILURE, 0, _("cannot do --relative without --symbolic")); + + if (!hard_dir_link) + { + priv_set_remove_linkdir (); + beware_hard_dir_link = !cannot_unlink_dir (); + } + if (no_target_directory) { if (target_directory) @@ -556,11 +597,17 @@ main (int argc, char **argv) { if (n_files < 2) target_directory = "."; - else if (2 <= n_files && target_directory_operand (file[n_files - 1])) - target_directory = file[--n_files]; - else if (2 < n_files) - die (EXIT_FAILURE, 0, _("target %s is not a directory"), - quoteaf (file[n_files - 1])); + else + { + if (n_files == 2) + link_errno = atomic_link (file[0], file[1]); + if ((link_errno < 0 || link_errno == EEXIST || link_errno == ENOTDIR) + && target_directory_operand (file[n_files - 1])) + target_directory = file[--n_files]; + else if (2 < n_files) + die (EXIT_FAILURE, 0, _("target %s is not a directory"), + quoteaf (file[n_files - 1])); + } } backup_type = (make_backups @@ -568,12 +615,6 @@ main (int argc, char **argv) : no_backups); set_simple_backup_suffix (backup_suffix); - if (relative && !symbolic_link) - { - die (EXIT_FAILURE, 0, - _("cannot do --relative without --symbolic")); - } - if (target_directory) { @@ -607,12 +648,12 @@ main (int argc, char **argv) last_component (file[i]), &dest_base); strip_trailing_slashes (dest_base); - ok &= do_link (file[i], dest); + ok &= do_link (file[i], dest, -1); free (dest); } } else - ok = do_link (file[0], file[1]); + ok = do_link (file[0], file[1], link_errno); return ok ? EXIT_SUCCESS : EXIT_FAILURE; }