From cdec7e6e93db547a80525c24167345a090a00273 Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Fri, 15 Jun 2007 22:47:16 +0200 Subject: Correct cp's handling of destination symlinks in some cases. * NEWS: "cp" no longer considers a destination symlink to be the same as the referenced file when copying links or making backups. * src/copy.c (copy_reg): When following a symlink, use 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. (copy_internal): Follow destination symlinks only when copying a regular file and only when we don't intend to remove or rename the destination first, regardless of whether following source symlinks; this is because since POSIX and tradition (e.g., FreeBSD) say we should ordinarily follow destination symlinks if the system calls would ordinarily do so. * src/copy.h (struct cp_options): Add comment that 'dereference' is only for source files. * src/cp.c (usage): Note that --derereference etc. are only for source files. (make_dir_parents_private): Follow symlinks, regardless of whether --dereference is specified, because these are destination symlinks. * tests/cp/same-file: Adjust tests to match revised behavior. Filter out perror output since it might vary from host to host. Use sed alone instead of also using echo. * doc/coreutils.texi (cp invocation): Document the behavior better when the destination is a symlink. Clarify source versus destination symlinks. Describe the new behavior for destination symlinks. 2007-06-15 Jim Meyering * src/copy.c: Include "canonicalize.h". (copy_reg): Use canonicalize_filename_mode to follow the symlink, so that we can always open with O_EXCL and avoid a race. --- ChangeLog | 37 +++++++++++++++++++++++++++++ NEWS | 7 +++++- doc/coreutils.texi | 24 +++++++++++++------ src/copy.c | 69 +++++++++++++++++++++++++++++++++++++++++------------- src/copy.h | 2 +- src/cp.c | 12 +++++----- tests/cp/same-file | 24 ++++++++++++------- 7 files changed, 135 insertions(+), 40 deletions(-) diff --git a/ChangeLog b/ChangeLog index c1cc6aa02..f456c6afb 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,40 @@ +2007-06-15 Paul Eggert + + Correct cp's handling of destination symlinks in some cases. + * NEWS: "cp" no longer considers a destination symlink to be the + same as the referenced file when copying links or making backups. + * src/copy.c (copy_reg): When following a symlink, use 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. + (copy_internal): Follow destination symlinks only when copying a + regular file and only when we don't intend to remove or rename the + destination first, regardless of whether following source + symlinks; this is because since POSIX and tradition (e.g., + FreeBSD) say we should ordinarily follow destination symlinks if + the system calls would ordinarily do so. + * src/copy.h (struct cp_options): Add comment that 'dereference' + is only for source files. + * src/cp.c (usage): Note that --derereference etc. are only for + source files. + (make_dir_parents_private): Follow symlinks, regardless of whether + --dereference is specified, because these are destination symlinks. + * tests/cp/same-file: Adjust tests to match revised behavior. + Filter out perror output since it might vary from host to host. + Use sed alone instead of also using echo. + + * doc/coreutils.texi (cp invocation): Document the behavior better when + the destination is a symlink. Clarify source versus destination + symlinks. Describe the new behavior for destination symlinks. + +2007-06-15 Jim Meyering + + * src/copy.c: Include "canonicalize.h". + (copy_reg): Use canonicalize_filename_mode to follow the symlink, + so that we can always open with O_EXCL and avoid a race. + 2007-06-15 Jim Meyering Don't include "quote.h" when it is not used. diff --git a/NEWS b/NEWS index 9db7902d0..f1b81f0cc 100644 --- a/NEWS +++ b/NEWS @@ -18,7 +18,12 @@ GNU coreutils NEWS -*- outline -*- ** Bug fixes cp no longer fails to write through a dangling symlink - [bug introduced in coreutils-6.7] + [bug introduced in coreutils-6.7]. Also, 'cp' no longer considers a + destination symlink to be the same as the referenced file when + copying links or making backups. For example, if SYM is a symlink + to FILE, "cp -l FILE SYM" now reports an error instead of silently + doing nothing. The behavior of 'cp' is now better documented when + the destination is a symlink. cut now diagnoses a range starting with zero (e.g., -f 0-2) as invalid; before, it would treat it as if it started with 1 (-f 1-2). diff --git a/doc/coreutils.texi b/doc/coreutils.texi index 4fdee3ecb..7290ab27f 100644 --- a/doc/coreutils.texi +++ b/doc/coreutils.texi @@ -6910,13 +6910,22 @@ By default, @command{cp} does not copy directories. However, the copy recursively by descending into source directories and copying files to corresponding destination directories. -By default, @command{cp} follows symbolic links only when not copying +When copying from a symbolic link, @command{cp} normally follows the +link only when not copying recursively. This default can be overridden with the @option{--archive} (@option{-a}), @option{-d}, @option{--dereference} (@option{-L}), @option{--no-dereference} (@option{-P}), and @option{-H} options. If more than one of these options is specified, the last one silently overrides the others. +When copying to a symbolic link, @command{cp} normally follows the +link when creating or copying to a regular file, even if the link is +dangling. However, @command{cp} does not follow these links when +creating directories or special files. Also, when an option like +@option{--backup} or @option{--link} acts to rename or remove the +destination before copying, @command{cp} renames or removes the +symbolic link rather than the file it points to. + By default, @command{cp} copies the contents of special files only when not copying recursively. This default can be overridden with the @option{--copy-contents} option. @@ -6996,10 +7005,10 @@ Equivalent to @option{--no-dereference --preserve=links}. @opindex --force 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 unlinks it and +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 unlinked unconditionally. Also see the +is never opened but rather is removed unconditionally. Also see the description of @option{--remove-destination}. This option is independent of the @option{--interactive} or @@ -7029,7 +7038,7 @@ Make hard links instead of copies of non-directories. @itemx --dereference @opindex -L @opindex --dereference -Always follow symbolic links. +Follow symbolic links when copying from them. @item -P @itemx --no-dereference @@ -7037,7 +7046,8 @@ Always follow symbolic links. @opindex --no-dereference @cindex symbolic links, copying Copy symbolic links as symbolic links rather than copying the files that -they point to. +they point to. This option affects only symbolic links in the source; +symbolic links in the destination are always followed if possible. @item -p @itemx @w{@kbd{--preserve}[=@var{attribute_list}]} @@ -7127,8 +7137,8 @@ about each existing destination file. @cindex copying directories recursively @cindex recursively copying directories @cindex non-directories, copying as special files -Copy directories recursively. Symbolic links are not followed by -default; see the @option{--archive} (@option{-a}), @option{-d}, +Copy directories recursively. By default, do not follow symbolic +links in the source; see the @option{--archive} (@option{-a}), @option{-d}, @option{--dereference} (@option{-L}), @option{--no-dereference} (@option{-P}), and @option{-H} options. Special files are copied by creating a destination file of the same type as the source; see the diff --git a/src/copy.c b/src/copy.c index b46221c01..0e9f2d791 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,21 @@ 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. + However, if we intend to unlink or remove the destination + first, use lstat, since a copy won't actually be made to the + destination in that case. */ + if ((((S_ISREG (src_mode) + || (x->copy_as_regular + && ! (S_ISDIR (src_mode) || S_ISLNK (src_mode)))) + && ! (x->move_mode || x->symbolic_link || x->hard_link + || x->backup_type != no_backups + || x->unlink_dest_before_opening)) + ? stat (dst_name, &dst_sb) + : lstat (dst_name, &dst_sb)) + != 0) { if (errno != ENOENT) { @@ -1151,7 +1188,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 index 0d6233f95..23cde2bee 100644 --- a/src/copy.h +++ b/src/copy.h @@ -87,7 +87,7 @@ struct cp_options them, symbolic links,) as if they were regular files. */ bool copy_as_regular; - /* How to handle symlinks. */ + /* How to handle symlinks in the source. */ enum Dereference_symlink dereference; /* If true, remove each existing destination nondirectory before diff --git a/src/cp.c b/src/cp.c index 3023408db..1d66bc382 100644 --- a/src/cp.c +++ b/src/cp.c @@ -181,14 +181,14 @@ Mandatory arguments to long options are mandatory for short options too.\n\ -f, --force if an existing destination file cannot be\n\ opened, remove it and try again\n\ -i, --interactive prompt before overwrite\n\ - -H follow command-line symbolic links\n\ + -H follow command-line symbolic links in SOURCE\n\ "), stdout); fputs (_("\ -l, --link link files instead of copying\n\ - -L, --dereference always follow symbolic links\n\ + -L, --dereference always follow symbolic links in SOURCE\n\ "), stdout); fputs (_("\ - -P, --no-dereference never follow symbolic links\n\ + -P, --no-dereference never follow symbolic links in SOURCE\n\ "), stdout); fputs (_("\ -p same as --preserve=mode,ownership,timestamps\n\ @@ -393,7 +393,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. */ @@ -413,7 +413,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; @@ -426,7 +426,7 @@ make_dir_parents_private (char const *const_dir, size_t src_offset, make_dir_parents_private creates only e_dir/../a if ./b already exists. */ *new_dst = true; - src_errno = (XSTAT (x, src, &stats) != 0 + src_errno = (stat (src, &stats) != 0 ? errno : S_ISDIR (stats.st_mode) ? 0 diff --git a/tests/cp/same-file b/tests/cp/same-file index 44d5dd74c..8e0593e40 100755 --- a/tests/cp/same-file +++ b/tests/cp/same-file @@ -89,9 +89,15 @@ for args in 'foo symlink' 'symlink foo' 'foo foo' 'sl1 sl2' 'foo hardlink'; do cp $options $args 2>_err echo $? $options - # Normalize the program name in the error output, + # Normalize the program name and diagnostics in the error output, # and put brackets around the output. - test -s _err && echo "[`sed 's/^[^:][^:]*:/cp:/' _err`]" + if test -s _err; then + sed ' + s/^[^:]*:\([^:]*\).*/cp:\1/ + 1s/^/[/ + $s/$/]/ + ' _err + fi # Strip off all but the file names. ls="`ls -gG --ignore=_err . \ | sed \ @@ -128,13 +134,13 @@ cat <<\EOF > $expected 0 -bd (foo symlink symlink.~1~ -> foo) 0 -bf (foo symlink symlink.~1~ -> foo) 0 -bdf (foo symlink symlink.~1~ -> foo) -0 -l (foo symlink -> foo) +1 -l [cp: cannot create link `symlink'] (foo symlink -> foo) 0 -dl (foo symlink -> foo) -0 -fl (foo symlink -> foo) +0 -fl (foo symlink) 0 -dfl (foo symlink) -0 -bl (foo symlink -> foo) +0 -bl (foo symlink symlink.~1~ -> foo) 0 -bdl (foo symlink symlink.~1~ -> foo) -0 -bfl (foo symlink -> foo) +0 -bfl (foo symlink symlink.~1~ -> foo) 0 -bdfl (foo symlink symlink.~1~ -> foo) 1 [cp: `symlink' and `foo' are the same file] (foo symlink -> foo) @@ -179,10 +185,10 @@ cat <<\EOF > $expected 0 -bd (foo sl1 -> foo sl2 -> foo sl2.~1~ -> foo) 0 -bf (foo sl1 -> foo sl2 sl2.~1~ -> foo) 0 -bdf (foo sl1 -> foo sl2 -> foo sl2.~1~ -> foo) -0 -l (foo sl1 -> foo sl2 -> foo) +1 -l [cp: cannot create link `sl2'] (foo sl1 -> foo sl2 -> foo) 0 -fl (foo sl1 -> foo sl2 -> foo) -0 -bl (foo sl1 -> foo sl2 -> foo) -0 -bfl (foo sl1 -> foo sl2 -> foo) +0 -bl (foo sl1 -> foo sl2 -> foo sl2.~1~ -> foo) +0 -bfl (foo sl1 -> foo sl2 -> foo sl2.~1~ -> foo) 1 [cp: `foo' and `hardlink' are the same file] (foo hardlink) 1 -d [cp: `foo' and `hardlink' are the same file] (foo hardlink) -- cgit v1.2.3-70-g09d2