RHEL Engineering is moving the tracking of its product development work on RHEL 6 through RHEL 9 to Red Hat Jira (issues.redhat.com). If you're a Red Hat customer, please continue to file support cases via the Red Hat customer portal. If you're not, please head to the "RHEL project" in Red Hat Jira and file new tickets here. Individual Bugzilla bugs in the statuses "NEW", "ASSIGNED", and "POST" are being migrated throughout September 2023. Bugs of Red Hat partners with an assigned Engineering Partner Manager (EPM) are migrated in late September as per pre-agreed dates. Bugs against components "kernel", "kernel-rt", and "kpatch" are only migrated if still in "NEW" or "ASSIGNED". If you cannot log in to RH Jira, please consult article #7032570. That failing, please send an e-mail to the RH Jira admins at rh-issues@redhat.com to troubleshoot your issue as a user management inquiry. The email creates a ServiceNow ticket with Red Hat. Individual Bugzilla bugs that are migrated will be moved to status "CLOSED", resolution "MIGRATED", and set with "MigratedToJIRA" in "Keywords". The link to the successor Jira issue will be found under "Links", have a little "two-footprint" icon next to it, and direct you to the "RHEL project" in Red Hat Jira (issue links are of type "https://issues.redhat.com/browse/RHEL-XXXX", where "X" is a digit). This same link will be available in a blue banner at the top of the page informing you that that bug has been migrated.
Bug 980061 - mv: fails to overwrite directory on cross-filesystem copy with EISDIR
Summary: mv: fails to overwrite directory on cross-filesystem copy with EISDIR
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Red Hat Enterprise Linux 6
Classification: Red Hat
Component: coreutils
Version: 6.4
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: rc
: ---
Assignee: Ondrej Vasik
QA Contact: Tomas Dolezal
URL:
Whiteboard:
Depends On:
Blocks: 1035224
TreeView+ depends on / blocked
 
Reported: 2013-07-01 10:41 UTC by Ken Booth
Modified: 2013-12-19 00:17 UTC (History)
6 users (show)

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.
Clone Of:
: 1035224 (view as bug list)
Environment:
Last Closed: 2013-11-21 20:56:23 UTC
Target Upstream Version:
Embargoed:


Attachments (Terms of Use)


Links
System ID Private Priority Status Summary Last Updated
Red Hat Product Errata RHSA-2013:1652 0 normal SHIPPED_LIVE Low: coreutils security, bug fix, and enhancement update 2013-11-20 21:53:21 UTC

Description Ken Booth 2013-07-01 10:41:27 UTC
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 11:58:33 UTC
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 12:03:53 UTC
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 12:17:39 UTC
Not unlike with upstream - so this should be discussed at bug-coreutils or coreutils - 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 20:31:13 UTC
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 22:28:06 UTC
(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 22:37:54 UTC
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-02 00:36:56 UTC
After discussing with Eric Blake on bug-coreutils 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.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-02 00:48:39 UTC
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-02 03:02:14 UTC
(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 08:43:32 UTC
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 15:56:07 UTC
Upstream fix committed:
http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commit;h=2bdb74ec1

Comment 22 errata-xmlrpc 2013-11-21 20:56:23 UTC
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.