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

From: Boris Ranto <[email-masked]>
Date: Tue, 18 Nov 2014 20:28:03 +0100

On Tue, 2014-11-18 at 16:46 +0000, Pádraig Brady wrote:
> On 18/11/14 16:29, Boris Ranto wrote:
> > 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.
> 
> I'm not sure symlinks should be treated differently here.
> I.E. it may be best to remove the whole unlink_src logic.
> I'll look later.
> 
> thanks,
> Pádraig.

You were right, the only other place that used the unlink_src logic was
the case handling mv for symlinks that were hard links to the same file
(this case was handled separetely from the normal files) -- i.e. the
case where you do this:
touch a; ln -s a b; ln b c; mv b c

I'm attaching the revised patch that removes the whole unlink_src logic
altogether.

-Boris
From d55f18a50e2e7298ae72106fa53ffeb7f7303f5f Mon Sep 17 00:00:00 2001
From: Boris Ranto <[email-masked]>
Date: Tue, 18 Nov 2014 20:20: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. The patch also removes the
unlink_src option to same_file_ok which is no longer necessary.
*src/copy.c (copy_internal): We need to patch up this function to
not use the unlink_src option which we removed from same_file_ok.
---
 src/copy.c | 50 ++++++++++++--------------------------------------
 1 file changed, 12 insertions(+), 38 deletions(-)

diff --git a/src/copy.c b/src/copy.c
index 446e72d..09cbba7 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -1407,20 +1407,12 @@ close_src_desc:
    copy a regular file onto a symlink that points to it.
    Try to minimize the cost of this function in the common case.
    Set *RETURN_NOW if we've determined that the caller has no more
-   work to do and should return successfully, right away.
-
-   Set *UNLINK_SRC if we've determined that the caller wants to do
-   'rename (a, b)' where 'a' and 'b' are distinct hard links to the same
-   file. In that case, the caller should try to unlink 'a' and then return
-   successfully.  Ideally, we wouldn't have to do that, and we'd be
-   able to rely on rename to remove the source file.  However, POSIX
-   mistakenly requires that such a rename call do *nothing* and return
-   successfully.  */
+   work to do and should return successfully, right away. */
 
 static bool
 same_file_ok (char const *src_name, struct stat const *src_sb,
               char const *dst_name, struct stat const *dst_sb,
-              const struct cp_options *x, bool *return_now, bool *unlink_src)
+              const struct cp_options *x, bool *return_now)
 {
   const struct stat *src_sb_link;
   const struct stat *dst_sb_link;
@@ -1431,7 +1423,6 @@ same_file_ok (char const *src_name, struct stat const *src_sb,
   bool same = SAME_INODE (*src_sb, *dst_sb);
 
   *return_now = false;
-  *unlink_src = false;
 
   /* FIXME: this should (at the very least) be moved into the following
      if-block.  More likely, it should be removed, because it inhibits
@@ -1468,9 +1459,7 @@ same_file_ok (char const *src_name, struct stat const *src_sb,
                  caller that all it must do is unlink A and return.  */
               if (same_link)
                 {
-                  *unlink_src = true;
-                  *return_now = true;
-                  return true;
+                  return false;
                 }
             }
 
@@ -1558,27 +1547,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
@@ -1939,11 +1922,10 @@ copy_internal (char const *src_name, char const *dst_name,
         { /* Here, we know that dst_name exists, at least to the point
              that it is stat'able or lstat'able.  */
           bool return_now;
-          bool unlink_src;
 
           have_dst_lstat = !use_stat;
           if (! same_file_ok (src_name, &src_sb, dst_name, &dst_sb,
-                              x, &return_now, &unlink_src))
+                              x, &return_now))
             {
               error (0, 0, _("%s and %s are the same file"),
                      quote_n (0, src_name), quote_n (1, dst_name));
@@ -2002,22 +1984,14 @@ copy_internal (char const *src_name, char const *dst_name,
              cp and mv treat -i and -f differently.  */
           if (x->move_mode)
             {
-              if (abandon_move (x, dst_name, &dst_sb)
-                  || (unlink_src && unlink (src_name) == 0))
+              if (abandon_move (x, dst_name, &dst_sb))
                 {
                   /* Pretend the rename succeeded, so the caller (mv)
                      doesn't end up removing the source file.  */
                   if (rename_succeeded)
                     *rename_succeeded = true;
-                  if (unlink_src && x->verbose)
-                    printf (_("removed %s\n"), quote (src_name));
                   return true;
                 }
-              if (unlink_src)
-                {
-                  error (0, errno, _("cannot remove %s"), quote (src_name));
-                  return false;
-                }
             }
           else
             {
-- 
1.9.3