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

[Eric Blake]

On 07/01/2013 04:27 PM, Eric Blake wrote:

> 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,

Oops, I missed one point - rename() is allowed to fail cross-filesystem
with EXDEV, and that's part of your testcase (at least, I assume you
mean that /test and /home/test are different filesystems, and therefore
the atomic rename() fails and we have to plow on with the next steps in
POSIX).  So, because the failure is EXDEV, we go on to step 4:

>> If the destination path exists, and it is a file of type directory and 
>> source_file is not a file of type directory, or it is a file not of type 
>> directory and source_file is a file of type directory, 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. If the destination 
>> path exists and was created by a previous step, it is unspecified whether 
>> this will treated as an error or the destination path will be overwritten.

The destination exists, but we don't have mismatch between
directory/non-directory.  So we don't error out, but instead go on to
step 5:

>> If the destination path exists, mv shall attempt to remove it. If this fails 
>> for any reason, 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.

Aha - we are attempting to remove it with unlink().  But for empty
directories, we should be using rmdir(), or even better, we should be
using remove() (which subsumes both unlink() and rmdir() at once).  I
wonder if a simpler patch would be to just s/unlink/rmdir/ in the line
of code where you were adding special-casing on errno values.

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

signature.asc
Description: OpenPGP digital signature