[coreutils] mv mistakenly unlink the destination before rename

Version:

coreutils-6.10 (fixed in 6.11)

How to reproduce the failure?

We cannot fully reproduce the failure. However, we can partially observe the bug manifestation using gdb with the following setting:

ln ./afile ./hardlink <- this is to make the nlink count of hardlink > 1

Then we use gdb  on mv:

gdb /coreutils-6.10/src/mv

(gdb) b copy.c:1349

(gdb) b copy.c:1514

(gdb) run some_cross_device_readonly_file hardlink

Symptom:

Wrong result.

 “mv would mistakenly unlink a destination file before calling rename,

 when the destination had two or more hard links.  “ --- developer’s bug fix summary.

We can see that at line 1349, it will first unlink the destination file, before reaching line 1514 (the rename). According to the developers, this behavior is not desirable. However, we are not sure what is the consequence of this failure. We surmise that under some specific execution scenarios this can be troublesome.

Root cause explained:

The condition of when to remove the destination file was incorrectly evaluated.

During the buggy execution, the flow is as following:

-------------------- copy.c (comments written by us are in blue) -------------------------------

copy_internal (src_name = “./my_fifo”, dst_name = “./my_fifo_dest” ...)

bool copy_internal (char const *src_name, char const *dst_name,  bool new_dst ... )

{

/* copy_internal is an internal library that is used by many unitilities

* such as ‘cp’, ‘mv’, etc.

*

* The original logic below is messed up. When the condition:

* (x->preserve_links && 1<dst_sb.st_nlink ) succeeds, it would call

* the following unlink to remove the destination, despite the fact that

* x->move_mode is set to true!

*  

* However, the correct logic is that unlink should never be called in ‘mv’,

* where x->move_mode == true.

* So the fix is to call unlink only when x->move_mode == false!

*/

    else if (! S_ISDIR (dst_sb.st_mode)

+      && !x->move_mode

      && (x->unlink_dest_before_opening

          || (x->preserve_links && 1 < dst_sb.st_nlink)

-          ||(!x->move_mode

-              && x->dereference==DEREF_NEVER

-              && S_ISLNK(src_sb.st_mode))))

+          ||(x->dereference == DEREF_NEVER

+              && !S_ISREG (src_sb.st_mode))))

{

1349:           if (unlink (dst_name) != 0 && errno != ENOENT)

       /* unlink the destination! This unlink should never be called by mv*/

        {

                  error (0, errno, _("cannot remove %s"), quote (dst_name));

                  return false;

            }

               … ...

} // else if (! S_ISDIR) ...

   /* Later in the same function: */

   if (x->move_mode)   {

1514: if (rename (src_name, dst_name) == 0)

        {

         /* here is the actual mv by using rename. If this rename succeeds

            (mv successful), then it will return. However, in the buggy

            execution, since we cannot rename from src to dst, the rename

            will return non-zero. */

            ...

         return true; /

     }

1578  if (errno != EXDEV)

/* If the rename failed because the src file is on a mounted device, rename

   would have failed with errno =EXDEV, bypassing the basic block below  */

1579   {

1590           error (0, errno,

1591                  _("cannot move %s to %s"),

1592                  quote_n (0, src_name), quote_n (1, dst_name));

1593           forget_created (src_sb.st_ino, src_sb.st_dev);

1594           return false;

1595   }

1596

1597   /* The rename attempt has failed.  Remove any existing destination

1598      file so that a cross-device `mv' acts as if it were really using

1599      the rename syscall.  */

 /* Here, they try to unlink the dst_name again. So the unlink would fail, with errno == ENOENT. However, they simply ignored the ENOENT error. So what we propose is as long as it failed, just log! */

1600  if (unlink (dst_name) != 0 && errno != ENOENT)

1601   {

1602     error (0, errno,

1603        _("inter-device move failed: %s to %s; unable to remove target"),

1604                  quote_n (0, src_name), quote_n (1, dst_name));

1605     forget_created (src_sb.st_ino, src_sb.st_dev);

1606     return false;

1607   }

     

}

Is there Error Message?

Not in all cases.

How can Errlog insert an error message?

Yes. It is function return value pattern!

In fact, there are two opportunities for Errlog to insert error message:

the error return of rename@line 1514 where errno==EXDEV, and unlink@line1600 where errno == ENOENT,

In particular, the 2nd msg, printed from the error return of unlink, will clearly indicate that the destination no longer exists! This provide strong hint to the developer that the file might be removed earlier in the execution!

Discussions:

The bug is yet another example that “very complicated if conditions are vulnerable”. The root cause is a wrong if condition:

1341           else if (! S_ISDIR (dst_sb.st_mode)

1342                    && (x->unlink_dest_before_opening

1343                        || (x->preserve_links && 1 < dst_sb.st_nlink)

1344                        || (!x->move_mode

1345                            && x->dereference == DEREF_NEVER

1346                            && S_ISLNK (src_sb.st_mode))

1347                        ))

Such complicated conditions should be more thoroughly tested.