[coreutils] mv /etc/passwd ~

Version:

coreutils-6.8 (fixed in 6.10)

How to reproduce the failure?

use the command  

coreutils-6.8/src/mv /etc/passwd ~

Symptom:

Incorrect results. The failure has a message printed.

In version 6.8:

$ mv /etc/passwd ~

$ mv: cannot remove `/etc/passwd': Not a directory

--- This is not a correct error message, instead it should print:

$ mv: cannot remove `/etc/passwd': Permission denied

Root cause:

When ‘mv’ is trying to remove the ‘/etc/passwd’ using ‘unlink’, ‘unlink’ failed with error return value because of permission denial. However, ‘mv’ did not handle this error properly and did not terminate the execution in time, causing the error to propagate.

Comments written by us are in blue color.

remove.c:

/* remove_entry: remove a single file entry. If it is a non-empty directory,

* then return RM_NONEMPTY_DIR to the caller rm_1 so rm_1 would use

* ‘remove_dir’ to remove the directory.

*/

/* In this case, where filename == /etc/passwd, remove_entry should have

* rejected

* the input by printing ‘permission denied’, and return RM_ERROR. However,

* in the incorrect execution, it somehow did not properly handle the

* invalid input and returned RM_NONEMPTY_DIR, causing the execution to

* continue to ‘remove_dir’ in rm_1. */

remove_entry () {

  … …

  /* DO_UNLINK is a macro to ‘unlink’ system call (defined below).

   * Here, the filename

   * is ‘/etc/passwd’. This unlink call actually failed, with ‘

   * errno == EACCES’, indicating

   * permission denied. But ‘mv’ did not check it properly. */

 DO_UNLINK (fd_cwd, filename, x);

/* In the failure execution we have:

 * cache_stat_ok (st) == false, ISDIR (st->st_mode) == false.

 * So the if decision is evaluated as false, not going into the error

 * handling code.

 *

 * The patch is to loosen

 * the condition so the correct execution would reach the true branch.

 */

if (cache_stat_ok (st) && !S_ISDIR (st->st_mode)

+  || ((errno == EACCES || errno == EPERM)

+        && is_nondir_lstat (fd_cwd, filename, st))

  ) {

    /* It should have reached here, print an error message:

     * Permission denied, and return

     * RM_ERROR, which will terminate the execution. However,

     * in the incorrect

     * execution, it simply did not reach here because the condition

     * was too strict. */

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

                 quote (full_filename (filename)));

          return RM_ERROR;

     }

   return RM_NONEMPTY_DIR;

}

------ This is the definition of DO_UNLINK -----------------

#define DO_UNLINK(Fd_cwd, Filename, X)                                  \

     if (unlinkat (Fd_cwd, Filename, 0) == 0)                          \

       {                                                               \

         if ((X)->verbose)                                             \

           printf (_("removed %s\n"), quote (full_filename (Filename))); \

         return RM_OK;                                                 \

       }    

    /* What we can do: */                                                          \

+    else                      \

+           elog();           \

                                                                       \

     if (ignorable_missing (X, errno))                                 \

       return RM_OK;

/* rm_1 first tries to remove the source as a single file (non-directory)

* with ‘remove_entry’, and if it found out the source is a non-empty

* directory, it further uses ‘remove_dir’

* to recursively remove the entire directory.

* In the buggy execution, remove_entry returned RM_NONEMPTY_DIR...

* Therefore the remove_dir was called. In the fixed version,

* remove_entry will return RM_ERROR and remove_dir is not called. */

rm_1 ( … …) {

   enum RM_status status =

           remove_entry(AT_FDCWD, ds, filename, &st, x, NULL);

   if (status == RM_NONEMPTY_DIR) {

           status = remove_dir (AT_FDCWD, ds, filename, &st, x, cwd_errno);

                    … …

   }

   return status;

}

/* ‘remove_dir’ is to remove a non-empty directory.

* In the incorrect execution, dir == /etc/passwd. */

remove_dir (int fd_cwd, Dirstack_state *ds, char const *dir, … ) {

               … ...

    /* Since here mv already thinks ‘dir’ is a directory, it uses

     * unlinkat system call, which is to remove directories,

     * to remove the entry /etc/passwd. */

       if (unlinkat (fd_cwd, dir, AT_REMOVEDIR) == 0)

              return RM_OK;

       /* unlinkat (/etc/passwd) failed because it was not a

        * directory, so the following

        * error message was printed. */

       error (0, saved_errno, _("cannot remove %s"), quote (full_filename (dir)));

 

       return RM_ERROR;

}

Is there Error Message?

Yes. (though this error message is wrong and misleading).

Can Errlog automatically insert the error message?

Yes. We can anticipate both error return value of system call: DO_UNLINK and ‘unlinkat’ in remove_dir and print error message. The printed message from DO_UNLINK, together with errno, would provide sufficient information to diagnose the failure.