Re: cp -rl dir dir in 7.1 not so great

[Jim Meyering]

Mikael Magnusson wrote:
> In my old 6.x something, cp would just say
>   cp: cannot copy a directory, `dir', into itself, `dir/dir'
> (it would still do it, but only once)
> In 7.1 I get a result like this:
>
> % timeout 1 cp -rl dir dir
> % ls -R dir|wc -l
> 1063
>
> Without the -l it still behaves properly.

Thank you for the report!
That is indeed a new (introduced in 7.1) bug.

Two consolations for me:
  - the buggy cp still diagnoses the invalid command -- just later ;-)
  - in fixing it, I extended an optimization from the offending change
      to apply to all uses of cp, not just those that use --link

Here's the fix I expect to push:

>From 362797e50939573b1ab7f20190fa944be38b3630 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Fri, 27 Feb 2009 09:23:44 +0100
Subject: [PATCH] cp: diagnose invalid "cp -rl dir dir" right away, once again

Running "mkdir dir; cp -rl dir dir" would create dir/dir/dir/...
rather than diagnosing the "copy-into-self" failure.

The easy fix would have been to revert this part of the change
[3ece0355 2008-11-09 cp: use far less memory in some cases]
that introduced the bug:

-         remember_copied (dst_name, dst_sb.st_ino, dst_sb.st_dev);
+         if (!x->hard_link)
+           remember_copied (dst_name, dst_sb.st_ino, dst_sb.st_dev);

However, that would have induced the failure of the new cp/link-heap
test, due to the added memory pressure of recording 10k dev/ino pairs.
And besides, I liked that improvement and wanted to keep it.

Now that it's obvious recording the just-created-directory dev/ino
needn't depend on the setting of hard_link, I realized it is necessary
to record the pair only for the first directory created for each
source command-line argument.

I made that change, then noticed the new test, cp -rl a d d, would pass
when run once, yet output the into-self diagnostic twice.  Also note
the side effect: it creates d/a and d/d.  However, running that same
command a second time, now with the modified directory, would fail.

That turned out to be due to the fact that although the first into-self
failure was detected in copy_dir, that function would continue copying
other entries regardless -- and that would make it fail (eventually)
with the unwanted recursion.

* src/copy.c (copy_internal): This function needed an indicator of
whether, for a give command line argument, it had already created its
first directory.  If so, no more need to record dev/ino pairs.  If this
is the first, then do record its pair.  Hence, the new parameter.
(copy_dir, copy): Update callers.
(copy_dir): Upon any into-self failure, break out of the loop.
* tests/cp/into-self: Test for the above.
Reported by Mikael Magnusson.
---
 NEWS               |    5 +++++
 THANKS             |    1 +
 src/copy.c         |   35 ++++++++++++++++++++++++++++-------
 tests/cp/into-self |   21 +++++++++++++++++++--
 4 files changed, 53 insertions(+), 9 deletions(-)

diff --git a/NEWS b/NEWS
index 05d22cb..87589c4 100644
--- a/NEWS
+++ b/NEWS
@@ -4,6 +4,11 @@ GNU coreutils NEWS                                    -*- 
outline -*-

 ** Bug fixes

+  cp once again diagnoses the invalid "cp -rl dir dir" right away,
+  rather than after creating a very deep dir/dir/dir/... hierarchy.
+  The bug strikes only with both --recursive (-r, -R) and --link (-l).
+  [bug introduced in coreutils-7.1]
+
   sort now handles specified key ends correctly.
   Previously -k1,1b would have caused leading space from field 2 to be
   included in the sort while -k2,3.0 would have not included field 3.
diff --git a/THANKS b/THANKS
index 724a9e3..253fc40 100644
--- a/THANKS
+++ b/THANKS
@@ -398,6 +398,7 @@ Michal Politowski                   address@hidden
 Michal Svec                         address@hidden
 Michel Robitaille                   address@hidden
 Michiel Bacchiani                   address@hidden
+Mikael Magnusson                    address@hidden
 Mike Castle                         address@hidden
 Mike Coleman                        address@hidden
 Mike Jetzer                         address@hidden
diff --git a/src/copy.c b/src/copy.c
index 7a7fae4..1918671 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -104,6 +104,7 @@ static bool copy_internal (char const *src_name, char const 
*dst_name,
                           struct dir_list *ancestors,
                           const struct cp_options *x,
                           bool command_line_arg,
+                          bool *first_dir_created_per_command_line_arg,
                           bool *copy_into_self,
                           bool *rename_succeeded);
 static bool owner_failure_ok (struct cp_options const *x);
@@ -201,13 +202,16 @@ copy_attr_by_name (char const *src_path, char const 
*dst_path)
    DST_NAME_IN is a directory that was created previously in the
    recursion.   SRC_SB and ANCESTORS describe SRC_NAME_IN.
    Set *COPY_INTO_SELF if SRC_NAME_IN is a parent of
+   FIRST_DIR_CREATED_PER_COMMAND_LINE_ARG  FIXME
    (or the same as) DST_NAME_IN; otherwise, clear it.
    Return true if successful.  */

 static bool
 copy_dir (char const *src_name_in, char const *dst_name_in, bool new_dst,
          const struct stat *src_sb, struct dir_list *ancestors,
-         const struct cp_options *x, bool *copy_into_self)
+         const struct cp_options *x,
+         bool *first_dir_created_per_command_line_arg,
+         bool *copy_into_self)
 {
   char *name_space;
   char *namep;
@@ -237,12 +241,19 @@ copy_dir (char const *src_name_in, char const 
*dst_name_in, bool new_dst,

       ok &= copy_internal (src_name, dst_name, new_dst, src_sb->st_dev,
                           ancestors, &non_command_line_options, false,
+                          first_dir_created_per_command_line_arg,
                           &local_copy_into_self, NULL);
       *copy_into_self |= local_copy_into_self;

       free (dst_name);
       free (src_name);

+      /* If we're copying into self, there's no point in continuing,
+        and in fact, that would even infloop, now that we record only
+        the first created directory per command line argument.  */
+      if (local_copy_into_self)
+       break;
+
       namep += strlen (namep) + 1;
     }
   free (name_space);
@@ -1125,6 +1136,7 @@ restore_default_fscreatecon_or_die (void)
    not known.  ANCESTORS points to a linked, null terminated list of
    devices and inodes of parent directories of SRC_NAME.  COMMAND_LINE_ARG
    is true iff SRC_NAME was specified on the command line.
+   FIRST_DIR_CREATED_PER_COMMAND_LINE_ARG is both input and output.
    Set *COPY_INTO_SELF if SRC_NAME is a parent of (or the
    same as) DST_NAME; otherwise, clear it.
    Return true if successful.  */
@@ -1135,6 +1147,7 @@ copy_internal (char const *src_name, char const *dst_name,
               struct dir_list *ancestors,
               const struct cp_options *x,
               bool command_line_arg,
+              bool *first_dir_created_per_command_line_arg,
               bool *copy_into_self,
               bool *rename_succeeded)
 {
@@ -1815,11 +1828,15 @@ copy_internal (char const *src_name, char const 
*dst_name,
                }
            }

-         /* Insert the created directory's inode and device
-             numbers into the search structure, so that we can
-             avoid copying it again.  */
-         if (!x->hard_link)
-           remember_copied (dst_name, dst_sb.st_ino, dst_sb.st_dev);
+         /* Record the created directory's inode and device numbers into
+            the search structure, so that we can avoid copying it again.
+            Do this only for the first directory that is created for each
+            source command line argument.  */
+         if (!*first_dir_created_per_command_line_arg)
+           {
+             remember_copied (dst_name, dst_sb.st_ino, dst_sb.st_dev);
+             *first_dir_created_per_command_line_arg = true;
+           }

          if (x->verbose)
            emit_verbose (src_name, dst_name, NULL);
@@ -1838,6 +1855,7 @@ copy_internal (char const *src_name, char const *dst_name,
             in a source directory would cause the containing destination
             directory not to have owner/perms set properly.  */
          delayed_ok = copy_dir (src_name, dst_name, new_dst, &src_sb, dir, x,
+                                first_dir_created_per_command_line_arg,
                                 copy_into_self);
        }
     }
@@ -2185,8 +2203,11 @@ copy (char const *src_name, char const *dst_name,
   top_level_src_name = src_name;
   top_level_dst_name = dst_name;

+  bool first_dir_created_per_command_line_arg = false;
   return copy_internal (src_name, dst_name, nonexistent_dst, 0, NULL,
-                       options, true, copy_into_self, rename_succeeded);
+                       options, true,
+                       &first_dir_created_per_command_line_arg,
+                       copy_into_self, rename_succeeded);
 }

 /* Set *X to the default options for a value of type struct cp_options.  */
diff --git a/tests/cp/into-self b/tests/cp/into-self
index ee3fcf5..cd87232 100755
--- a/tests/cp/into-self
+++ b/tests/cp/into-self
@@ -1,7 +1,7 @@
 #!/bin/sh
 # Confirm that copying a directory into itself gets a proper diagnostic.

-# Copyright (C) 2001, 2002, 2004, 2006-2008 Free Software Foundation, Inc.
+# Copyright (C) 2001, 2002, 2004, 2006-2009 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
@@ -28,15 +28,32 @@ fi

 . $srcdir/test-lib.sh

-mkdir dir || framework_failure
+mkdir a dir || framework_failure

 fail=0

 # This command should exit nonzero.
 cp -R dir dir 2> out && fail=1
+echo 1 >> out
+
+# This should, too.  However, with coreutils-7.1 it would infloop.
+cp -rl dir dir 2>> out && fail=1
+echo 2 >> out
+
+cp -rl a dir dir 2>> out && fail=1
+echo 3 >> out
+cp -rl a dir dir 2>> out && fail=1
+echo 4 >> out

 cat > exp <<\EOF
 cp: cannot copy a directory, `dir', into itself, `dir/dir'
+1
+cp: cannot copy a directory, `dir', into itself, `dir/dir'
+2
+cp: cannot copy a directory, `dir', into itself, `dir/dir'
+3
+cp: cannot copy a directory, `dir', into itself, `dir/dir'
+4
 EOF
 #'

--
1.6.2.rc1.285.gc5f54