From df3e53f003533feb9a4ae1daa01ad1a7cd9e0c96 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C3=A1draig=20Brady?= Date: Mon, 14 May 2018 02:26:05 -0700 Subject: [PATCH] cp: fix symlink checks when overwriting files Ensure this _does_ recreate the symlink Given "path1" and "path2" are on different devices. $ touch "path1/file" $ cd path2/; ln -s path1/file $ cp -sf path1/file . Ensure this does _not_ overwrite file $ touch file $ ln -s file l1 $ cp -sf l1 file * src/copy.c (same_file_ok): Remove device ids from consideration, instead deferring to future EXDEV with --link or allowing the first case above to work. Also ensure that we do not exist this function too early, when the destination file is not a symlink, which protects against the second case. * tests/cp/cross-dev-symlink.sh: Add a test for the first case. * tests/cp/same-file.sh: Add a test for the second case above. * NEWS: Mention the bug fixes. * THANKS.in: Mention the reporters who also analyzed the code. Fixes https://bugs.gnu.org/31364 --- NEWS | 7 +++++++ THANKS.in | 2 ++ src/copy.c | 18 ++++++++---------- tests/cp/cross-dev-symlink.sh | 38 ++++++++++++++++++++++++++++++++++++++ tests/cp/same-file.sh | 15 ++++++++++++++- tests/local.mk | 1 + 6 files changed, 70 insertions(+), 11 deletions(-) create mode 100755 tests/cp/cross-dev-symlink.sh diff --git a/NEWS b/NEWS index de02814..7aa2925 100644 --- a/NEWS +++ b/NEWS @@ -4,6 +4,13 @@ GNU coreutils NEWS -*- outline -*- ** Bug fixes + 'cp --symlink SRC DST' will again correctly validate DST. + If DST is a regular file and SRC is a symlink to DST, + then cp will no longer allow that operation to clobber DST. + Also if DST is a symlink, then it can always be replaced, + even if it points to SRC on a separate device. + [bug introduced with coreutils-8.27] + 'ls -aA' is now equivalent to 'ls -A', since -A now overrides -a. [bug introduced in coreutils-5.3.0] diff --git a/THANKS.in b/THANKS.in index c63cc9b..3951b66 100644 --- a/THANKS.in +++ b/THANKS.in @@ -261,6 +261,7 @@ Ian Kent address@hidden Ian Lance Taylor address@hidden Ian Turner address@hidden Iida Yosiaki address@hidden +Illia Bobyr address@hidden Ilya N. Golubev address@hidden Ingo Saitz address@hidden Ivan Labath address@hidden @@ -285,6 +286,7 @@ Jan-Pawel Wrozstinski address@hidden Jari Aalto address@hidden Jarkko Hietaniemi address@hidden Jarod Wilson address@hidden +Jason Smith address@hidden Jean Charles Delepine address@hidden Jean-Pierre Tosoni address@hidden Jeff Moore address@hidden diff --git a/src/copy.c b/src/copy.c index 4998c83..0407c56 100644 --- a/src/copy.c +++ b/src/copy.c @@ -1627,14 +1627,9 @@ 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 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; + /* It's ok to recreate a destination symlink. */ + if (x->symbolic_link && S_ISLNK (dst_sb_link->st_mode)) + return true; if (x->dereference == DEREF_NEVER) { @@ -1651,10 +1646,13 @@ same_file_ok (char const *src_name, struct stat const *src_sb, if ( ! SAME_INODE (tmp_src_sb, tmp_dst_sb)) return true; - /* FIXME: shouldn't this be testing whether we're making symlinks? */ if (x->hard_link) { - *return_now = true; + /* It's ok to attempt to hardlink the same file, + and return early if not replacing a symlink. + Note we need to return early to avoid a later + unlink() of DST (when SRC is a symlink). */ + *return_now = ! S_ISLNK (dst_sb_link->st_mode); return true; } } diff --git a/tests/cp/cross-dev-symlink.sh b/tests/cp/cross-dev-symlink.sh new file mode 100755 index 0000000..e945b40 --- /dev/null +++ b/tests/cp/cross-dev-symlink.sh @@ -0,0 +1,38 @@ +#!/bin/sh +# Ensure symlinks can be replaced across devices + +# Copyright (C) 2018 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 . + +. "${srcdir=.}/tests/init.sh"; path_prepend_ ./src +print_ver_ cp +require_root_ + +cwd=$(pwd) +cleanup_() { cd /; umount "$cwd/mnt"; } + +truncate -s100M fs.img || framework_failure_ +mkfs -t ext4 fs.img || skip_ 'failed to create ext4 file system' +mkdir mnt || framework_failure_ +mount fs.img mnt || skip_ 'failed to mount ext4 file system' + +mkdir mnt/path1 || framework_failure_ +touch mnt/path1/file || framework_failure_ +mkdir path2 || framework_failure_ +cd path2 && ln -s ../mnt/path1/file || framework_failure_ + +cp -sf ../mnt/path1/file . 2>err || fail=1 + +Exit $fail diff --git a/tests/cp/same-file.sh b/tests/cp/same-file.sh index 01bdb98..e50a991 100755 --- a/tests/cp/same-file.sh +++ b/tests/cp/same-file.sh @@ -47,7 +47,7 @@ contents=XYZ for args in 'foo symlink' 'symlink foo' 'foo foo' 'sl1 sl2' \ 'foo hardlink' 'hlsl sl2'; do for options in '' -d -f -df --rem -b -bd -bf -bdf \ - -l -dl -fl -dfl -bl -bdl -bfl -bdfl; do + -l -dl -fl -dfl -bl -bdl -bfl -bdfl -s -sf; do case $args$options in # These tests are not portable. # They all involve making a hard link to a symbolic link. @@ -100,6 +100,7 @@ for args in 'foo symlink' 'symlink foo' 'foo foo' 'sl1 sl2' \ # and put brackets around the output. if test -s _err; then sed ' + s/symbolic link/symlink/ s/^[^:]*:\([^:]*\).*/cp:\1/ 1s/^/[/ $s/$/]/ @@ -149,6 +150,8 @@ cat <<\EOF | sed "$remove_these_sed" > expected 0 -bdl (foo symlink symlink.~1~ -> foo) 0 -bfl (foo symlink symlink.~1~ -> foo) 0 -bdfl (foo symlink symlink.~1~ -> foo) +1 -s [cp: cannot create symlink 'symlink' to 'foo'] (foo symlink -> foo) +0 -sf (foo symlink -> foo) 1 [cp: 'symlink' and 'foo' are the same file] (foo symlink -> foo) 1 -d [cp: 'symlink' and 'foo' are the same file] (foo symlink -> foo) @@ -164,6 +167,8 @@ cat <<\EOF | sed "$remove_these_sed" > expected 0 -fl (foo symlink -> foo) 0 -bl (foo symlink -> foo) 0 -bfl (foo symlink -> foo) +1 -s [cp: 'symlink' and 'foo' are the same file] (foo symlink -> foo) +1 -sf [cp: 'symlink' and 'foo' are the same file] (foo symlink -> foo) 1 [cp: 'foo' and 'foo' are the same file] (foo) 1 -d [cp: 'foo' and 'foo' are the same file] (foo) @@ -182,6 +187,8 @@ cat <<\EOF | sed "$remove_these_sed" > expected 0 -bdl (foo) 0 -bfl (foo foo.~1~) 0 -bdfl (foo foo.~1~) +1 -s [cp: 'foo' and 'foo' are the same file] (foo) +1 -sf [cp: 'foo' and 'foo' are the same file] (foo) 1 [cp: 'sl1' and 'sl2' are the same file] (foo sl1 -> foo sl2 -> foo) 0 -d (foo sl1 -> foo sl2 -> foo) @@ -196,6 +203,8 @@ cat <<\EOF | sed "$remove_these_sed" > expected 0 -fl (foo sl1 -> foo sl2) 0 -bl (foo sl1 -> foo sl2 sl2.~1~ -> foo) 0 -bfl (foo sl1 -> foo sl2 sl2.~1~ -> foo) +1 -s [cp: cannot create symlink 'sl2' to 'sl1'] (foo sl1 -> foo sl2 -> foo) +0 -sf (foo sl1 -> foo sl2 -> sl1) 1 [cp: 'foo' and 'hardlink' are the same file] (foo hardlink) 1 -d [cp: 'foo' and 'hardlink' are the same file] (foo hardlink) @@ -214,6 +223,8 @@ cat <<\EOF | sed "$remove_these_sed" > expected 0 -bdl (foo hardlink) 0 -bfl (foo hardlink) 0 -bdfl (foo hardlink) +1 -s [cp: 'foo' and 'hardlink' are the same file] (foo hardlink) +1 -sf [cp: 'foo' and 'hardlink' are the same file] (foo hardlink) 1 [cp: 'hlsl' and 'sl2' are the same file] (foo hlsl -> foo sl2 -> foo) 0 -d (foo hlsl -> foo sl2 -> foo) @@ -232,6 +243,8 @@ cat <<\EOF | sed "$remove_these_sed" > expected 0 -bdl (foo hlsl -> foo sl2 -> foo) 0 -bfl (foo hlsl -> foo sl2 sl2.~1~ -> foo) 0 -bdfl (foo hlsl -> foo sl2 -> foo) +1 -s [cp: cannot create symlink 'sl2' to 'hlsl'] (foo hlsl -> foo sl2 -> foo) +0 -sf (foo hlsl -> foo sl2 -> hlsl) EOF diff --git a/tests/local.mk b/tests/local.mk index e60ea1d..1f8b189 100644 --- a/tests/local.mk +++ b/tests/local.mk @@ -113,6 +113,7 @@ all_root_tests = \ tests/cp/cp-mv-enotsup-xattr.sh \ tests/cp/capability.sh \ tests/cp/sparse-fiemap.sh \ + tests/cp/cross-dev-symlink.sh \ tests/dd/skip-seek-past-dev.sh \ tests/df/problematic-chars.sh \ tests/df/over-mount-device.sh \ -- 2.9.3