Bug 980061 - mv: fails to overwrite directory on cross-filesystem copy with EISDIR
mv: fails to overwrite directory on cross-filesystem copy with EISDIR
Status: CLOSED ERRATA
Product: Red Hat Enterprise Linux 6
Classification: Red Hat
Component: coreutils (Show other bugs)
6.4
Unspecified Unspecified
unspecified Severity unspecified
: rc
: ---
Assigned To: Ondrej Vasik
Tomas Dolezal
:
Depends On:
Blocks: 1035224
  Show dependency treegraph
 
Reported: 2013-07-01 06:41 EDT by Ken Booth
Modified: 2013-12-18 19:17 EST (History)
6 users (show)

See Also:
Fixed In Version: coreutils-8.4-22.el6
Doc Type: Bug Fix
Doc Text:
Cause: When moving directories between two filesystems, mv fails to overwrite empty directory. Consequence: This is violation of the POSIX standard. Fix: Utility mv no longer fails to overwrite empty destination directory as specified by POSIX. Result: POSIX standard rules are obeyed.
Story Points: ---
Clone Of:
: 1035224 (view as bug list)
Environment:
Last Closed: 2013-11-21 15:56:23 EST
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Ken Booth 2013-07-01 06:41:27 EDT
Description of problem:
  When copying directories cross-filesystem with mv and the destination directory already exists the copy fails with EISDIR

Version-Release number of selected component (if applicable):
RHEL5 all versions
RHEL6 all versions
Fedora all versions

How reproducible:
100%

Steps to Reproduce:
1.

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


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

Additional info:
Works on Solaris 10 as follows

sol10 # mkdir /export/WWW/test
sol10 # mkdir /test                     
sol10 # cp /etc/passwd /export/WWW/test
sol10 # mv /export/WWW/test /
sol10 # ls /test
passwd
sol10 # ls /export/WWW
main         lost+found

Possible fix ...

$ diff copy.c copy.c.new
2176c2176
<       if (unlink (dst_name) != 0 && errno != ENOENT)
---
>       if (unlink (dst_name) != 0 && errno != ENOENT && errno != EISDIR)

Will compile and test effects on pre-existing directories and their contents, and compare with Solaris. To see if this suggested fix has the same effect, then we need to decide if the effect is desirable.
Comment 2 Ondrej Vasik 2013-07-01 07:58:33 EDT
Thanks for suggestion, I'll check if this is acceptable with upstream. Please do not set fast flags next time. Maintainer should do that himself.
Comment 3 Ken Booth 2013-07-01 08:03:53 EDT
The first draft of suggested code doesn't work, so here is a second draft

2176c2176
<       if (unlink (dst_name) != 0 && errno != ENOENT)
---
>       if (unlink (dst_name) != 0 && errno != ENOENT && errno != EISDIR)
2267,2269c2267,2274
<               error (0, errno, _("cannot create directory %s"),
<                      quote (dst_name));
<               goto un_backup;
---
>               if (errno == EEXIST)
>                 {
>                   //* some code here to chmod the directory *//
>                 } else {
>                   error (0, errno, _("cannot create directory %s"),
>                          quote (dst_name));
>                   goto un_backup;
>                 }

Needs a bit of work to flesh out chmod'ing the destination directory, but it achieves the same bahaviour as Solaris has.

When the destination directory exists and has contents the source directory merges the directories clobbering existing files.

E.g.
f18# cp /etc/group /test
f18# vi /test/passwd
f18# mkdir /home/test
f18# cp /etc/passwd /home/test
f18# ls -l /home/test /test
/home/test:
total 4
-rw-r--r--. 1 root root 2768 Jul  1 12:54 passwd

/test:
total 8
-rw-r--r--. 1 root root 1162 Jul  1 12:53 group
-rw-r--r--. 1 root root 2781 Jul  1 12:54 passwd
f18# /home/ken/git/fedora/coreutils/src/mv /home/test /
f18# ls -l /home/test /test
ls: cannot access /home/test: No such file or directory
/test:
total 8
-rw-r--r--. 1 root root 1162 Jul  1 12:53 group
-rw-r--r--. 1 root root 2768 Jul  1 12:54 passwd

Now, the question is. Do we want to accept this behaviour as correct for Linux?
Comment 4 Ondrej Vasik 2013-07-01 08:17:39 EDT
Not unlike with upstream - so this should be discussed at bug-coreutils@gnu.org or coreutils@gnu.org - if you want to bring it up there, feel free to do so, otherwise I will raise this question myself - hopefully later this week. Still, 

I think it is not something what should be changed in released RHEL, as it may bring in some nasty consequences for the people who rely on current behaviour.
Comment 5 Ken Booth 2013-07-01 16:31:13 EDT
Final draft for proposed changes is as follows:

src/copy.c
2176c2176
<       if (unlink (dst_name) != 0 && errno != ENOENT)
---
>       if (unlink (dst_name) != 0 && errno != ENOENT && errno != EISDIR)
2267,2269c2267,2278
<               error (0, errno, _("cannot create directory %s"),
<                      quote (dst_name));
<               goto un_backup;
---
>               if (errno == EEXIST)
>                 {
>                   if (lchmod (dst_name, dst_mode & ~omitted_permissions) != 0)
>                     {
>                       error (0, errno, _("setting permissions for %s"),
>                              quote (dst_name));
>                     }
>                 } else {
>                   error (0, errno, _("cannot create directory %s"),
>                          quote (dst_name));
>                   goto un_backup;
>                 }

Completed various tests and this appears to achieve the desired effect.
Comment 6 Eric Blake 2013-07-01 18:28:06 EDT
(In reply to Ken Booth from comment #5)
> Final draft for proposed changes is as follows:
> 
> src/copy.c
> 2176c2176
> <       if (unlink (dst_name) != 0 && errno != ENOENT)
> ---
> >       if (unlink (dst_name) != 0 && errno != ENOENT && errno != EISDIR)

Ed script patches are illegible.  Please repost as a context diff.
Comment 7 Eric Blake 2013-07-01 18:37:54 EDT
Wouldn't this be simpler?

diff --git i/src/copy.c w/src/copy.c
index c1c8273..599ed8b 100644
--- i/src/copy.c
+++ w/src/copy.c
@@ -2173,7 +2173,7 @@ copy_internal (char const *src_name, char const *dst_name,
       /* The rename attempt has failed.  Remove any existing destination
          file so that a cross-device 'mv' acts as if it were really using
          the rename syscall.  */
-      if (unlink (dst_name) != 0 && errno != ENOENT)
+      if (remove (dst_name) != 0 && errno != ENOENT)
         {
           error (0, errno,
              _("inter-device move failed: %s to %s; unable to remove target"),
Comment 8 Ken Booth 2013-07-01 20:36:56 EDT
After discussing with Eric Blake on bug-coreutils@gnu.org determined that the above patch is not atomic (using special handling of the unlink error is not recommended practice), and further reading of POSIX standards determines that Solaris is not POSIX compliant.

Therefore propsed a new patch as follows ...

From 8b6549f321c06ee81262f58c6d7bd7e9c9092b30 Mon Sep 17 00:00:00 2001
From: Ken Booth <ken@booths.org.uk>
Date: Tue, 2 Jul 2013 01:06:32 +0100
Subject: [PATCH] mv: if inter-device move target is directory use rmdir, not
 unlink

---
 src/copy.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/src/copy.c b/src/copy.c
index c1c8273..5137f27 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -2173,13 +2173,27 @@ copy_internal (char const *src_name, char const *dst_name,
       /* The rename attempt has failed.  Remove any existing destination
          file so that a cross-device 'mv' acts as if it were really using
          the rename syscall.  */
-      if (unlink (dst_name) != 0 && errno != ENOENT)
+      if (S_ISDIR (src_mode))
         {
-          error (0, errno,
-             _("inter-device move failed: %s to %s; unable to remove target"),
-                 quote_n (0, src_name), quote_n (1, dst_name));
-          forget_created (src_sb.st_ino, src_sb.st_dev);
-          return false;
+          if (rmdir (dst_name) != 0 && errno != ENOENT)
+            {
+              error (0, errno,
+                 _("inter-device move failed: %s to %s; unable to remove target directory"),
+                     quote_n (0, src_name), quote_n (1, dst_name));
+              forget_created (src_sb.st_ino, src_sb.st_dev);
+              return false;
+            }
+        }
+      else
+        {
+          if (unlink (dst_name) != 0 && errno != ENOENT)
+            {
+              error (0, errno,
+                 _("inter-device move failed: %s to %s; unable to remove target"),
+                     quote_n (0, src_name), quote_n (1, dst_name));
+              forget_created (src_sb.st_ino, src_sb.st_dev);
+              return false;
+            }
         }
 
       new_dst = true;
-- 
1.8.1.4
Comment 9 Ken Booth 2013-07-01 20:48:39 EDT
Hi Eric,

Oops, just noticed your update. I compiled and tested it and your solution is neater.

I used 20 lines, and you did the same thing in a single line.

Test results give the correct errors:

f18# mkdir /home/test
f18# cp /etc/passwd /home/test
f18# ls -la /home/test /test
/home/test:
total 12
drwxr-xr-x.  2 root root 4096 Jul  2 01:41 .
drwxr-xr-x. 14 root root 4096 Jul  2 01:41 ..
-rw-r--r--.  1 root root 2768 Jul  2 01:41 passwd

/test:
total 12
drwxr-xr-x.  2 root root 4096 Jul  2 00:58 .
dr-xr-xr-x. 25 root root 4096 Jul  2 00:59 ..
-rw-r--r--.  1 root root 2768 Jul  2 00:58 passwd
f18# /home/ken/git/fedora/coreutils/src/mv /home/test /
/home/ken/git/fedora/coreutils/src/mv: inter-device move failed: ‘/home/test’ to ‘/test’; unable to remove target: Directory not empty
f18# rm /test/*
rm: remove regular file ‘/test/passwd’? y
f18# /home/ken/git/fedora/coreutils/src/mv /home/test /
f18# ls -la /home/test /test
ls: cannot access /home/test: No such file or directory
/test:
total 12
drwxr-xr-x.  2 root root 4096 Jul  2 01:41 .
dr-xr-xr-x. 25 root root 4096 Jul  2 01:45 ..
-rw-r--r--.  1 root root 2768 Jul  2 01:41 passwd
Comment 10 Eric Blake 2013-07-01 23:02:14 EDT
(In reply to Ken Booth from comment #9)
> Hi Eric,
> 
> Oops, just noticed your update. I compiled and tested it and your solution
> is neater.
> 
> I used 20 lines, and you did the same thing in a single line.

Ah, but my solution (comment 7) is loosey-goosey when mixing a source file with destination directory, or source directory with destination file; your patch is closer to actual rename() semantics which is what POSIX requires.  At any rate, we'll see what upstream coreutils does about the issue.
Comment 11 Ondrej Vasik 2013-07-08 04:43:32 EDT
Pointer to upstream discussion: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=14763 ... atm. it seems only Ken and Eric discussed the item. As for the question about etiquette in Fedora - please wait for the upstream commit - I'll commit what is used by upstream - before the upstream commit I tend to say we should wait with any actions downstream.
Comment 14 Pádraig Brady 2013-07-25 11:56:07 EDT
Upstream fix committed:
http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commit;h=2bdb74ec1
Comment 22 errata-xmlrpc 2013-11-21 15:56:23 EST
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

http://rhn.redhat.com/errata/RHSA-2013-1652.html

Note You need to log in before you can comment on or make changes to this bug.