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

(0002534)
vapier
06-29-07 09:22

i wouldnt really call this a bug ... i'd say "dont do it"

(0002535)
kiltedknight
06-29-07 09:28

Yeah, but you never know what a script will attempt at some point.

(0002536)
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

(0002537)
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.

(0002538)
vapier
06-29-07 10:36

... which is still an error that needs to be cleaned up

(0002540)
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.

(0002651)
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.

(0002674)
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.

(0002676)
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.

(0002678)
kiltedknight
08-23-07 11:45

One of my co-workers said something about there being memory leaks in the old patch.

(0002679)
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.

(0002700)
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.

(0002701)
vda
08-27-07 10:05

Fixed in svn, thanks!

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

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

(0002536) 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

(0002537) 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.

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

(0002540) 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.

(0002651) 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.

(0002674) 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.

(0002676) 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.

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

(0002679) 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.

(0002700) 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.

(0002701) vda 08-27-07 10:05	Fixed in svn, thanks!

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
08-27-07 10:05	vda	Status	assigned => closed
08-27-07 10:05	vda	Note Added: 0002701
08-27-07 10:05	vda	Resolution	open => fixed

Relationships