bug#14763: mv directory cross-filesystem where destination exists fails to remove dest with EISDIR

[Eric Blake]

On 07/01/2013 03:21 PM, Ken Booth wrote:
> Hi,
> 
> I have found a bug which affects RHEL 5.9, RHEL 6.4, Fedora 18, and the
> latest version of coreutils from the git repo this morning (1st July
> 2013) in exactly the same way, and yet does not affect Solaris 10's mv
> command.
> 
> Test case:
> 
> f18 # mkdir /test /home/test
> f18 # cp /etc/passwd /home/test
> f18 # mv /home/test /
> mv: overwrite ‘/test’? y
> mv: inter-device move failed: ‘/home/test’ to ‘/test’; unable to remove
> target: Is a directory

Let's read what POSIX has to say about this:
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/mv.html

We are using the second form (mv source_file... target_dir) with
source_file of /home/test and target_dir of /.  The destination path is
thus constructed to be "/test".

Step 1 says:

>>     If the destination path exists, the -f option is not specified, and 
>> either of the following conditions is true:
>> 
>>         The permissions of the destination path do not permit writing and 
>> the standard input is a terminal.
>> 
>>         The -i option is specified.
>> 
>>     the mv utility shall write a prompt to standard error and read a line 
>> from standard input. If the response is not affirmative, mv shall do nothing 
>> more with the current source_file and go on to any remaining source_files.

The destination "/test" exists, but permissions allow writing (well,
assuming your umask is sane when you did mkdir) and -i is not specified,
so this step does not stop us, and we go on to step 2.

>> If the source_file operand and destination path name the same existing 
>> file...

Not the same, so on to step 3.

>> The mv utility shall perform actions equivalent to the rename() function 
>> defined in the System Interfaces volume of POSIX.1-2008, called with the 
>> following arguments:
>> 
>>     The source_file operand is used as the old argument.
>> 
>>     The destination path is used as the new argument.
>> 
>> If this succeeds, mv shall do nothing more with the current source_file and 
>> go on to any remaining source_files. If this fails for any reasons other 
>> than those described for the errno [EXDEV] in the System Interfaces volume 
>> of POSIX.1-2008, mv shall write a diagnostic message to standard error, do 
>> nothing more with the current source_file, and go on to any remaining 
>> source_files.

so now we have to cross-reference to see what is required for
rename("/home/test", "/test"):
http://pubs.opengroup.org/onlinepubs/9699919799/functions/rename.html

>> If the old argument points to the pathname of a directory, the new argument 
>> shall not point to the pathname of a file that is not a directory. If the 
>> directory named by the new argument exists, it shall be removed and old 
>> renamed to new. In this case, a link named new shall exist throughout the 
>> renaming operation and shall refer either to the directory referred to by 
>> new or old before the operation began. If new names an existing directory, 
>> it shall be required to be an empty directory.

Well, we just proved by your above construction that /test is empty, and
therefore should silently be replaced by the (non-empty) directory that
used to be named /home/test.  Therefore, the rename() should succeed,
and mv should do nothing further for the /home/test argument; and the
result of the rename is that the former /home/test/passwd should now be
located at /test/passwd.  You are correct that GNU mv has a bug.

Hmm, I seem to recall reporting a similar bug in the past regarding the
behavior on symlinks - a bit of research turned up commit f1d1ee912 in
May 2006 regarding non-atomic rename() of symlinks; now we are dealing
with non-atomic rename() of empty destination directories.

> Actual results:
> mv: inter-device move failed: ‘/home/test’ to ‘/test’; unable to remove
> target: Is a directory
> 
> Expected results:
> /home/test/passwd is copied to /test/passwd

Concur that this is what POSIX requires.

> 
> I have determined that the problem can be "fixed" (made to behave the
> same as Solaris) by editting src/copy.c as follows:
> 
> 2176c2176
> < if (unlink (dst_name) != 0 && errno != ENOENT)
> ---
>>       if (unlink (dst_name) != 0 &&  errno != ENOENT && errno != EISDIR)

Ed script patches are illegible.  Please instead submit this as a
context diff.  These directions in HACKING may help:
http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=blob;f=HACKING;h=49f97381;hb=HEAD

As is, I don't think your ed script patch is correct - it is not atomic.
 In other words, I don't think we should be trying unlink() followed by
special-case handling based on certain errno values, but instead should
be trying rename() up front.  Since the results are required to be
atomic, the success or failure of rename() will tell us whether we the
source directory was correctly renamed atop a previously empty
destination directory.

> 
> This has the same effect as mv does on Solaris 10.
> 
> Where the destination directory exists on the new mount point, the
> permissions are modified and new files will overwrite existing files in
> the destination tree. However, unique files in the destination directory
> will not be removed.

Huh? Nothing in the POSIX wording talked about altering permissions of
the destination directory after a rename of source dir to empty target dir.

> PS: There is a Red Hat bugzilla for this
> https://bugzilla.redhat.com/show_bug.cgi?id=980061

Thanks for the pointer.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature