[BusyBox 0001412]: "cp -a" allows copy of a directory into itself

bugs at busybox.net bugs at busybox.net
Mon Aug 27 14:12:29 UTC 2007


A NOTE has been added to this issue. 
====================================================================== 
http://busybox.net/bugs/view.php?id=1412 
====================================================================== 
Reported By:                kiltedknight
Assigned To:                BusyBox
====================================================================== 
Project:                    BusyBox
Issue ID:                   1412
Category:                   Other
Reproducibility:            always
Severity:                   minor
Priority:                   normal
Status:                     assigned
====================================================================== 
Date Submitted:             06-29-2007 09:16 PDT
Last Modified:              08-27-2007 07:12 PDT
====================================================================== 
Summary:                    "cp -a" allows copy of a directory into itself
Description: 
execute the following series of commands on GNU linux:

mkdir /tmp/dir1
touch /tmp/dir1/stuff
cp -a /tmp/dir1 /tmp/dir1

The following error is returned: cp: cannot copy a directory, `/tmp/dir1',
into itself, `/tmp/dir1/dir1'

Now run these:

mkdir /tmp/dir1
touch /tmp/dir1/stuff
busybox cp -a /tmp/dir1 /tmp/dir1

Now you get the following:
cp: cannot stat
'/tmp/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1':
File name too long

====================================================================== 

---------------------------------------------------------------------- 
 vapier - 06-29-07 09:22  
---------------------------------------------------------------------- 
i wouldnt really call this a bug ... i'd say "dont do it" 

---------------------------------------------------------------------- 
 kiltedknight - 06-29-07 09:28  
---------------------------------------------------------------------- 
Yeah, but you never know what a script will attempt at some point. 

---------------------------------------------------------------------- 
 vapier - 06-29-07 09:36  
---------------------------------------------------------------------- 
regardless of what a script does, you're going to end up with errors that
someone needs to go in and clean/fix ... what exact form that error takes
i think is irrelevant 

---------------------------------------------------------------------- 
 kiltedknight - 06-29-07 09:46  
---------------------------------------------------------------------- 
Except that this isn't just an error.  It actually recursively copies the
directories.  GNU Linux cp does not.  It does it one time and exits with
the error. 

---------------------------------------------------------------------- 
 vapier - 06-29-07 10:36  
---------------------------------------------------------------------- 
... which is still an error that needs to be cleaned up 

---------------------------------------------------------------------- 
 vda - 06-30-07 10:21  
---------------------------------------------------------------------- 
I know about this behaviour. It's not strictly a bug, more like "user
definitely made a typo in dir name, warn him and stop instead of filling
entire filesystem with recursive copy".

I think it makes sense for busybox cp to have such code too, just make it
conditional on some CONFIG_FEATURE_CP_xxx. 

---------------------------------------------------------------------- 
 kiltedknight - 08-08-07 11:05  
---------------------------------------------------------------------- 
Why should it be conditional?  A recursive copy of a directory into itself
may have started as a typo, but it should never continue to execute until
hitting the longest path name allowed or a disk full error, whichever
comes first. 

---------------------------------------------------------------------- 
 sh4d0wstr1f3 - 08-21-07 04:58  
---------------------------------------------------------------------- 
coreutils cp handles: copy of dir to dir, copy of dir into dir/sub1/ ...
subN/dir, and copy of dir into dir/ ... /link->dir ... the same code is
also used for move, so it detects a move of dir into
dir/sub1/.../subN/dir.

This is actually a battle that y'all fought long ago in a code base far
far away...

http://www.busybox.net/cgi-bin/viewcvs.cgi?rev=8516&view=rev
http://www.busybox.net/cgi-bin/viewcvs.cgi?rev=8517&view=rev
http://www.busybox.net/cgi-bin/viewcvs.cgi?rev=8547&view=rev

What situation the code in 8517 didn't handle that led Erik to revert it?
Maybe it wasn't pretty -- but it was miles cleaner than coreutils cp. 

---------------------------------------------------------------------- 
 kiltedknight - 08-22-07 12:15  
---------------------------------------------------------------------- 
Trying this against 1.4.2 produces two identical error messages:

cp: cannot copy a directory, 'stuff', into itself, 'stuff/stuff'

However, it does not behave the same way as coreutils because with
coreutils, I would find any file within "stuff" also present when doing
"ls stuff/stuff"... with this patch, stuff/stuff is empty. 

---------------------------------------------------------------------- 
 kiltedknight - 08-23-07 11:45  
---------------------------------------------------------------------- 
One of my co-workers said something about there being memory leaks in the
old patch. 

---------------------------------------------------------------------- 
 sh4d0wstr1f3 - 08-23-07 14:23  
---------------------------------------------------------------------- 
Ok, so there are problems with r8517; for a directory "sub/dir", doing 'cp
-R sub dir' is detected as a recursive copy. Probably not so helpful. It
can also use unitialized values of dest_stat and throws away the pointer
to new_source (which should probably be free()'d on the early return).

I think that the coreutils behavior is helpful (despite the fact that it
leaves residual directories around); accidental trees of the same
directory is not so nice. 

I'm going to throw up a small patch. This will detect a recursive copy in
the following cases:

cp sub sub
cp sub sub/dir
cp sub link_to_sub/dir

In each case, it detects the recursion an iteration later than coreutils,
leading to one level deeper residual than coreutils; but I think the
behavior as better than not doing anything and better than r8517.

You're not obligated to agree -- and if you have a better way I'm
certainly happy to hear about it.

The patch works by recording the source directories which are visited;
because no parent of dest can occur in source. This will only happen with
directories; and since there can't be hardlinks to directories, this
should never cause problems with the other code that uses the
ino_dev_hashtable.

The patch is against 1.4.2; if constructive feedback is provided I'm happy
to port the same for svn. 

---------------------------------------------------------------------- 
 sh4d0wstr1f3 - 08-27-07 07:12  
---------------------------------------------------------------------- 
vda proposed a patch that worked in most cases, but did not handle the
following scenario:

% mkdir -p sub/dir dir
% ln -s sub/dir dir/ldir
% busybox_vda cp -R dir sub sub [XXX]
cp: skipping 'dir/lsub/lsub' - recursive copying detected
cp: skipping 'sub/dir/lsub' - recursive copying detected

(long delay while sub/sub/sub/sub/sub/sub ... etc. is created)

sub/sub/sub/sub/sub/sub/.../sub/sub/sub: No such file or directory

Attached here is an updated version of my patch for svn trunk which is
resilient against the above scenario. 

Issue History 
Date Modified   Username       Field                    Change               
====================================================================== 
06-29-07 09:16  kiltedknight   New Issue                                    
06-29-07 09:16  kiltedknight   Status                   new => assigned     
06-29-07 09:16  kiltedknight   Assigned To               => BusyBox         
06-29-07 09:22  vapier         Note Added: 0002534                          
06-29-07 09:28  kiltedknight   Note Added: 0002535                          
06-29-07 09:28  kiltedknight   Issue Monitored: kiltedknight                    
06-29-07 09:36  vapier         Note Added: 0002536                          
06-29-07 09:46  kiltedknight   Note Added: 0002537                          
06-29-07 10:36  vapier         Note Added: 0002538                          
06-30-07 10:21  vda            Note Added: 0002540                          
08-08-07 11:05  kiltedknight   Note Added: 0002651                          
08-21-07 04:58  sh4d0wstr1f3   Note Added: 0002674                          
08-21-07 05:00  sh4d0wstr1f3   Issue Monitored: sh4d0wstr1f3                    
08-22-07 12:15  kiltedknight   Note Added: 0002676                          
08-23-07 11:45  kiltedknight   Note Added: 0002678                          
08-23-07 14:23  sh4d0wstr1f3   Note Added: 0002679                          
08-23-07 14:23  sh4d0wstr1f3   File Added: busybox.quintessence.r2446M.2        
           
08-27-07 07:12  sh4d0wstr1f3   File Added: busybox.quintessence.r19704M.2       
            
08-27-07 07:12  sh4d0wstr1f3   Note Added: 0002700                          
======================================================================




More information about the busybox-cvs mailing list