[PATCH] mv: fix data loss with repeated source dir and same destination

[Pádraig Brady]

commit v8.23-31-g90aa291 failed to consider this case,
where the previous rename has failed, thus causing the
following to remove the specified directory:

  mv dir dir dir

* src/copy.c (copy_internal): Assume this rename attempt has
succeeded, as a previous failure will already have been handled,
and we don't want to remove the source directory in this case.
* tests/cp/duplicate-sources.sh: Consolidate this test file to...
* tests/mv/dup-source.sh: ...here.  Add test cases for same
source and dest.
* tests/local.mk: Remove the consolidated test.
* NEWS: Mention the bug fix.

Reported at https://bugzilla.redhat.com/1297464
---
 NEWS                          |  4 ++++
 src/copy.c                    | 12 +++++++----
 tests/cp/duplicate-sources.sh | 44 -----------------------------------------
 tests/local.mk                |  1 -
 tests/mv/dup-source.sh        | 46 +++++++++++++++++++++++++++++++++----------
 5 files changed, 48 insertions(+), 59 deletions(-)
 delete mode 100755 tests/cp/duplicate-sources.sh

diff --git a/NEWS b/NEWS
index 6e48a53..1a88d22 100644
--- a/NEWS
+++ b/NEWS
@@ -19,6 +19,10 @@ GNU coreutils NEWS                                    -*- 
outline -*-
   ls no longer prematurely wraps lines when printing short file names.
   [bug introduced in coreutils-5.1.0]
 
+  mv no londer introduces data loss due to removing a source directory 
specified
+  multiple times, when that directory is also specified as the destination.
+  [bug introduced in coreutils-8.24]
+
   shred again uses defined patterns for all iteration counts.
   [bug introduced in coreutils-5.93]
 
diff --git a/src/copy.c b/src/copy.c
index aeb366a..db4c085 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -2278,10 +2278,14 @@ copy_internal (char const *src_name, char const 
*dst_name,
               error (0, 0, _("warning: source directory %s "
                              "specified more than once"),
                      quoteaf (top_level_src_name));
-              /* We only do backups in move mode and for non dirs,
-                 and in move mode this won't be the issue as the source will
-                 be missing for subsequent attempts.
-                 There we just warn and return here.  */
+              /* In move mode, if a previous rename succeeded, then
+                 we won't be in this path as the source is missing.  If the
+                 rename previously failed, then that has been handled.
+                 Pretend this instance succeeded so the source isn't removed.  
*/
+              if (x->move_mode && rename_succeeded)
+                *rename_succeeded = true;
+              /* We only do backups in move mode, and for non directories.
+                 So just ignore this repeated entry.  */
               return true;
             }
           else if (x->dereference == DEREF_ALWAYS
diff --git a/tests/cp/duplicate-sources.sh b/tests/cp/duplicate-sources.sh
deleted file mode 100755
index c0170bb..0000000
--- a/tests/cp/duplicate-sources.sh
+++ /dev/null
@@ -1,44 +0,0 @@
-#!/bin/sh
-# Ensure cp warns about but otherwise ignores source
-# items specified multiple times.
-
-# Copyright (C) 2014-2016 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 <http://www.gnu.org/licenses/>.
-
-. "${srcdir=.}/tests/init.sh"; path_prepend_ ./src
-print_ver_ cp
-
-mkdir a || framework_failure_
-touch f || framework_failure_
-
-# verify multiple files and dir sources only warned about
-mkdir dest || framework_failure_
-cp -a a a f f dest 2>err || fail=1
-rm -Rf dest || framework_failure_
-
-# verify multiple dirs and files with different names copied
-mkdir dest || framework_failure_
-ln -s a al || framework_failure_
-ln -s f fl || framework_failure_
-cp -aH a al f fl dest 2>>err || fail=1
-
-cat <<EOF >exp
-cp: warning: source directory 'a' specified more than once
-cp: warning: source file 'f' specified more than once
-EOF
-
-compare exp err || fail=1
-
-Exit $fail
diff --git a/tests/local.mk b/tests/local.mk
index aa85e26..8898897 100644
--- a/tests/local.mk
+++ b/tests/local.mk
@@ -443,7 +443,6 @@ all_tests =                                 \
   tests/cp/dir-rm-dest.sh                      \
   tests/cp/dir-slash.sh                                \
   tests/cp/dir-vs-file.sh                      \
-  tests/cp/duplicate-sources.sh                        \
   tests/cp/existing-perm-dir.sh                        \
   tests/cp/existing-perm-race.sh               \
   tests/cp/fail-perm.sh                                \
diff --git a/tests/mv/dup-source.sh b/tests/mv/dup-source.sh
index ff9de9d..efa4624 100755
--- a/tests/mv/dup-source.sh
+++ b/tests/mv/dup-source.sh
@@ -24,25 +24,37 @@ print_ver_ cp mv
 
 skip_if_root_
 
+reset_files() { rm -fr a b d; touch a; mkdir b d; }
+
 for i in cp; do
 
   # cp may not fail in this case.
-
-  rm -fr a d; touch a; mkdir d
+  reset_files
   $i a a d/ 2> out || fail=1
-  rm -fr a d; touch a; mkdir d
+  reset_files
   $i ./a a d/ 2>> out || fail=1
 
+  # Similarly for directories, but handle
+  # source == dest appropriately.
+  reset_files
+  $i -a ./b b d/ 2>> out || fail=1
+  reset_files
+  returns_ 1 $i -a ./b b b/ 2>> out || fail=1
+
   # cp succeeds with --backup=numbered.
-  rm -fr a d; touch a; mkdir d
+  reset_files
   $i --backup=numbered a a d/ 2>> out || fail=1
 
   # But not with plain '--backup'
-  rm -fr a d; touch a; mkdir d
-  $i --backup a a d/ 2>> out && fail=1
+  reset_files
+  returns_ 1 $i --backup a a d/ 2>> out || fail=1
+
   cat <<EOF > exp
 $i: warning: source file 'a' specified more than once
 $i: warning: source file 'a' specified more than once
+$i: warning: source directory 'b' specified more than once
+$i: cannot copy a directory, './b', into itself, 'b/b'
+$i: warning: source directory 'b' specified more than once
 $i: will not overwrite just-created 'd/a' with 'a'
 EOF
   compare exp out || fail=1
@@ -50,14 +62,28 @@ done
 
 for i in mv; do
   # But mv *does* fail in this case (it has to).
+  reset_files
+  returns_ 1 $i a a d/ 2> out || fail=1
+  returns_ 1 test -e a || fail=1
+  reset_files
+  returns_ 1 $i ./a a d/ 2>> out || fail=1
+  returns_ 1 test -e a || fail=1
+
+  # Similarly for directories, also handling
+  # source == dest appropriately.
+  reset_files
+  returns_ 1 $i ./b b d/ 2>> out || fail=1
+  returns_ 1 test -e b || fail=1
+  reset_files
+  returns_ 1 $i --verbose ./b b b/ 2>> out || fail=1
+  test -d b || fail=1
 
-  rm -fr a d; touch a; mkdir d
-  $i a a d/ 2> out && fail=1
-  rm -fr a d; touch a; mkdir d
-  $i ./a a d/ 2>> out && fail=1
   cat <<EOF > exp
 $i: cannot stat 'a': No such file or directory
 $i: cannot stat 'a': No such file or directory
+$i: cannot stat 'b': No such file or directory
+$i: cannot move './b' to a subdirectory of itself, 'b/b'
+$i: warning: source directory 'b' specified more than once
 EOF
   compare exp out || fail=1
 done
-- 
2.5.0