Re: rm -r gives up before going deeper upon encountering hindrances

[Paul Eggert]

Jim Meyering <address@hidden> writes:

> +  /* Accept either EISDIR, EPERM, or EACCES as an indication that FILENAME 
> may
> +     be a directory.  POSIX says that unlink must set errno to EPERM when it
> +     fails to remove a directory, while Linux-2.4.18 sets it to EISDIR.
> +     unlink fails with EACCES when the parent directory is write-protected.  
> */

Aren't other error numbers possible?  For example, suppose the
directory is in a read-only filesystem, but there's a writeable
filesystem mounted underneath it.  Then errno might equal EROFS even
though it's a directory and 'rm' should recurse underneath it.

Come to think of it, the current approach in remove.c seems backwards.
Instead of checking _against_ error numbers like EPERM that suggest
that the file might be a directory, the code should be checking _for_
error numbers like ENOENT and ENOTDIR that can be returned only if the
file cannot possibly be a directory.

Not that I'm an expert in this part of the code, but here's a proposed
patch.

2004-05-08  Paul Eggert  <address@hidden>

        Fix bug where "rm" gave up too easily, reported by Dan Jacobsen in
        <http://mail.gnu.org/archive/html/bug-coreutils/2004-05/msg00013.html>.

        * src/remove.c (remove_entry): Check for errno values like ENOENT
        that show the file cannot be directory, instead of for errno
        values like EPERM that show the file might be a directory.  This
        is necessary because, when a single unlink() call has multiple
        reasons to fail, it can set errno to any of those reasons; it's
        only the rare errno value like ENOENT that excludes all the other
        possible reasons to fail even when the file is a directory.
        (remove_cwd_entries): Don't attempt chdir if the file is known
        to not be a directory.
        (remove_dir): Use the same method that remove_cwd_entries uses
        (for some reason they differed).  Don't assert that saved_errno
        must be EPERM; it might be just about anything.

Index: src/remove.c
===================================================================
RCS file: /home/meyering/coreutils/cu/src/remove.c,v
retrieving revision 1.102
diff -p -u -r1.102 remove.c
--- src/remove.c        27 Apr 2004 15:00:32 -0000      1.102
+++ src/remove.c        8 May 2004 07:31:07 -0000
@@ -762,14 +762,12 @@ remove_entry (Dirstack_state const *ds, 
 
   DO_UNLINK (filename, x);
 
-  /* Accept either EISDIR or EPERM as an indication that FILENAME may be
-     a directory.  POSIX says that unlink must set errno to EPERM when it
-     fails to remove a directory, while Linux-2.4.18 sets it to EISDIR.  */
-  if ((errno != EISDIR && errno != EPERM) || ! x->recursive)
-    {
-      /* some other error code.  Report it and fail.
-        Likewise, if we're trying to remove a directory without
-        the --recursive option.  */
+  if (! x->recursive
+      || errno == ENOENT || errno == ENOTDIR
+      || errno == ELOOP || errno == ENAMETOOLONG)
+    {
+      /* Either --recursive is not in effect, or the file cannot be a
+        directory.  Report the unlink problem and fail.  */
       error (0, errno, _("cannot remove %s"),
             quote (full_filename (filename)));
       return RM_ERROR;
@@ -871,8 +869,7 @@ remove_cwd_entries (Dirstack_state *ds, 
        case RM_NONEMPTY_DIR:
          {
            /* Save a copy of errno, in case the preceding unlink (from
-              remove_entry's DO_UNLINK) of a non-directory failed due
-              to EPERM.  */
+              remove_entry's DO_UNLINK) of a non-directory failed.  */
            int saved_errno = errno;
 
            /* Record dev/ino of F so that we can compare
@@ -882,7 +879,8 @@ remove_cwd_entries (Dirstack_state *ds, 
              error (EXIT_FAILURE, errno, _("cannot lstat %s"),
                     quote (full_filename (f)));
 
-           if (chdir (f))
+           errno = ENOTDIR;
+           if (! S_ISDIR (subdir_sb->st_mode) || chdir (f) != 0)
              {
                /* It is much more common that we reach this point for an
                   inaccessible directory.  Hence the second diagnostic, below.
@@ -989,14 +987,11 @@ remove_dir (Dirstack_state *ds, char con
       return RM_ERROR;
     }
 
-  if (chdir (dir))
+  errno = ENOTDIR;
+  if (! S_ISDIR (dir_sb.st_mode) || chdir (dir) != 0)
     {
-      if (! S_ISDIR (dir_sb.st_mode))
+      if (errno == ENOTDIR)
        {
-         /* This happens on Linux-2.4.18 when a non-privileged user tries
-            to delete a file that is owned by another user in a directory
-            like /tmp that has the S_ISVTX flag set.  */
-         assert (saved_errno == EPERM);
          error (0, saved_errno,
                 _("cannot remove %s"), quote (full_filename (dir)));
        }