Subject: bug#18499: Possible mv race for hardlinks (rhbz #1141368 )

From: Boris Ranto <[email-masked]>
Date: Tue, 18 Nov 2014 17:29:26 +0100

On Mon, 2014-11-17 at 00:28 +0000, Pádraig Brady wrote:
> On 16/11/14 16:35, Paul Eggert wrote:
> > Pádraig Brady wrote:
> >> If we change this, it's much more likely that people will start complaining
> >> about their non overlapping mv instances failing.
> > 
> > I'd far rather deal with those complaints than deal with complaints about 'mv' silently discarding files.  Either the FreeBSD or the Solaris behavior would be a real improvement over what we're doing now.  Neither behavior is as good as having support in the kernel for doing the right thing, but one step at a time.
> 
> Fair enough. That's 3 votes for changing this.
> I'll work on a patch to fail in this case.
> 
> thanks,
> Pádraig.
> 

I've looked at the code and I was able to identify the part that deals
with the symlinks. I'm attaching the patch that makes mv fail in this
case.

btw: I noticed that cp on two distinct hard links already has this
behaviour, i.e.: 'cp a b' will already exit with cp: 'a' and 'b' are the
same file.

-Boris
From 44846b411e1841d208646563a250994c1976ef03 Mon Sep 17 00:00:00 2001
From: Boris Ranto <[email-masked]>
Date: Tue, 18 Nov 2014 17:15:50 +0100
Subject: [PATCH] copy: treat hard links to the same file as same file

* src/copy.c (same_file_ok) : We may run into a race condition if
we treat hard links to the same file as distinct files. If we do
'mv a b' and 'mv b a' in parallel, both a and b can disappear from
the file system. The reason is that in this case the unlink on src
is called and the system calls can end up being run in the order
where unlink(a) and unlink(b) are the last two system calls. This
patch makes the mv calls exit with an error code so that we can
avoid the potential data loss problem.
---
 src/copy.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/src/copy.c b/src/copy.c
index 446e72d..2f4e211 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -1558,27 +1558,21 @@ same_file_ok (char const *src_name, struct stat const *src_sb,
     return true;
 #endif
 
-  /* They may refer to the same file if we're in move mode and the
-     target is a symlink.  That is ok, since we remove any existing
-     destination file before opening it -- via 'rename' if they're on
-     the same file system, via 'unlink (DST_NAME)' otherwise.
-     It's also ok if they're distinct hard links to the same file.  */
   if (x->move_mode || x->unlink_dest_before_opening)
     {
+      /* They may refer to the same file if we're in move mode and the
+         target is a symlink.  That is ok, since we remove any existing
+         destination file before opening it -- via 'rename' if they're on
+         the same file system, via 'unlink (DST_NAME)' otherwise. */
       if (S_ISLNK (dst_sb_link->st_mode))
         return true;
 
+      /* It's not ok if they're distinct hard links to the same file as
+         this causes a race condition and we may lose data in this case. */
       if (same_link
           && 1 < dst_sb_link->st_nlink
           && ! same_name (src_name, dst_name))
-        {
-          if (x->move_mode)
-            {
-              *unlink_src = true;
-              *return_now = true;
-            }
-          return true;
-        }
+        return false;
     }
 
   /* If neither is a symlink, then it's ok as long as they aren't
-- 
1.9.3