From d3daa95096e2692e5c8a0402e270b0e2ec7918e5 Mon Sep 17 00:00:00 2001 From: =?utf8?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 -dsf 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 | 26 +++++++++++++++--------- 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, 80 insertions(+), 20 deletions(-) create mode 100755 tests/cp/cross-dev-symlink.sh diff --git a/NEWS b/NEWS index 3f34a11094..f981596ba9 100644 --- a/NEWS +++ b/NEWS @@ -4,15 +4,12 @@ GNU coreutils NEWS -*- outline -*- ** Bug fixes - 'ls -aA' is now equivalent to 'ls -A', since -A now overrides -a. - [bug introduced in coreutils-5.3.0] - - 'mv -n A B' no longer suffers from a race condition that can - overwrite a simultaneously-created B. This bug fix requires - platform support for the renameat2 or renameatx_np syscalls, found - in recent Linux and macOS kernels. As a side effect, ‘mv -n A A’ - now silently does nothing if A exists. - [bug introduced with coreutils-7.1] + '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 with -d, if DST is a symlink, then it can always be replaced, + even if it points to SRC on a separate device. + [bugs introduced with coreutils-8.27] 'cp -n -u' and 'mv -n -u' now consistently ignore the -u option. Previously, this option combination suffered from race conditions @@ -32,6 +29,17 @@ GNU coreutils NEWS -*- outline -*- display width between 6 and 12 inclusive. Previously this would have output ambiguous months for Arabic or Catalan locales. + 'ls -aA' is now equivalent to 'ls -A', since -A now overrides -a. + [bug introduced in coreutils-5.3.0] + + 'mv -n A B' no longer suffers from a race condition that can + overwrite a simultaneously-created B. This bug fix requires + platform support for the renameat2 or renameatx_np syscalls, found + in recent Linux and macOS kernels. As a side effect, ‘mv -n A A’ + now silently does nothing if A exists. + [bug introduced with coreutils-7.1] + + ** Improvements cut supports line lengths up to the max file size on 32 bit systems. diff --git a/THANKS.in b/THANKS.in index c63cc9bc7f..3951b66d69 100644 --- a/THANKS.in +++ b/THANKS.in @@ -261,6 +261,7 @@ Ian Kent ikent@redhat.com Ian Lance Taylor ian@cygnus.com Ian Turner vectro@pipeline.com Iida Yosiaki iida@gnu.org +Illia Bobyr ibobyr@google.com Ilya N. Golubev gin@mo.msk.ru Ingo Saitz ingo@debian.org Ivan Labath labath3@st.fmph.uniba.sk @@ -285,6 +286,7 @@ Jan-Pawel Wrozstinski jpwroz@gmail.com Jari Aalto jari.aalto@cante.net Jarkko Hietaniemi jhi@epsilon.hut.fi Jarod Wilson jwilson@redhat.com +Jason Smith jasonmsmith@google.com Jean Charles Delepine delepine@u-picardie.fr Jean-Pierre Tosoni jpt.7196@gmail.com Jeff Moore jbm@mordor.com diff --git a/src/copy.c b/src/copy.c index 4998c83e6a..0407c5689b 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 0000000000..12ef258efd --- /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 -dsf ../mnt/path1/file . || fail=1 + +Exit $fail diff --git a/tests/cp/same-file.sh b/tests/cp/same-file.sh index 01bdb987f2..e50a991d4c 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 e60ea1d1e0..1f8b189c23 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.47.2