Re: 'rm -fr' mishandles unreadable empty directories

[Jim Meyering]

Paul Eggert <address@hidden> wrote:
> (I found this while testing a new version of 'install-sh'.)
>
> Here's a scenario illustrating the problem.
>
>    $ mkdir -m a-r -p a/b
>    $ rm -fr a
>    rm: cannot open directory `a/b': Permission denied
>    rm: cannot remove directory `a': Directory not empty
>    $ ls -ld a a/b
>    drwxr-xr-x 3 eggert eggert 4096 Jun 26 00:51 a
>    d-wx-wx-wx 2 eggert eggert 4096 Jun 26 00:51 a/b
>
> My reading of POSIX is that, though a diagnostic is allowed here and
> rm can exit with nonzero status because a/b is unreadable, "rm" is
> still required to rmdir both a/b and a here, so this is a
> POSIX-conformance issue.
>
> Solaris 10 'rm' gets it right: when it discovers that "a/b" is
> unreadable (openat fails) it tries rmdir("a/b"), and this works.  No
> diagnostics are issued.
>
> This problem is in both coreutils 5.97 and CVS trunk.

Thanks for the report and test case.
I've fixed it on the trunk with the patch below.
Will look at the branch later.

2006-06-26  Jim Meyering  <address@hidden>

        * NEWS: rm no longer fails to remove an empty, unreadable directory
        * src/remove.c (remove_cwd_entries): If we can't open a directory,
        and the failure is not being ignored, try to remove the directory
        with rmdir (aka unlinkat-with-AT_REMOVEDIR), in case it's empty.
        Problem report and test case from Paul Eggert in
        <http://article.gmane.org/gmane.comp.gnu.core-utils.bugs/7425>.
        * tests/rm/empty-inacc: New test, for the above.

Index: NEWS
===================================================================
RCS file: /fetish/cu/NEWS,v
retrieving revision 1.386
diff -u -p -r1.386 NEWS
--- NEWS        25 Jun 2006 18:26:09 -0000      1.386
+++ NEWS        26 Jun 2006 12:10:08 -0000
@@ -72,6 +72,8 @@ GNU coreutils NEWS
   rm --interactive now takes an optional argument, although the
   default of using no argument still acts like -i.
 
+  rm no longer fails to remove an empty, unreadable directory
+
   sort now reports incompatible options (e.g., -i and -n) rather than
   silently ignoring one of them.
 
Index: src/remove.c
===================================================================
RCS file: /fetish/cu/src/remove.c,v
retrieving revision 1.150
diff -u -p -r1.150 remove.c
--- src/remove.c        25 Jun 2006 19:27:19 -0000      1.150
+++ src/remove.c        26 Jun 2006 12:39:34 -0000
@@ -1125,13 +1125,28 @@ remove_cwd_entries (DIR **dirp,
                                              x, errno, subdir_sb, ds, NULL);
            if (subdir_dirp == NULL)
              {
+               status = RM_ERROR;
+
                /* CAUTION: this test and diagnostic are identical to
                   those following the other use of fd_to_subdirp.  */
-               if (errno != ENOENT || !x->ignore_missing_files)
-                 error (0, errno,
-                        _("cannot remove %s"), quote (full_filename (f)));
-               AD_mark_as_unremovable (ds, f);
-               status = RM_ERROR;
+               if (errno == ENOENT && x->ignore_missing_files)
+                 {
+                   /* With -f, don't report "file not found".  */
+                 }
+               else
+                 {
+                   /* Upon fd_to_subdirp failure, try to remove F directly,
+                      in case it's just an empty directory.  */
+                   int saved_errno = errno;
+                   if (unlinkat (dirfd (*dirp), f, AT_REMOVEDIR) == 0)
+                     status = RM_OK;
+                   else
+                     error (0, saved_errno,
+                            _("cannot remove %s"), quote (full_filename (f)));
+                 }
+
+               if (status == RM_ERROR)
+                 AD_mark_as_unremovable (ds, f);
                break;
              }
 
@@ -1214,6 +1229,9 @@ remove_dir (int fd_cwd, Dirstack_state *
        {
          /* If fd_to_subdirp fails due to permissions, then try to
             remove DIR via rmdir, in case it's just an empty directory.  */
+         /* This use of rmdir just works, at least in the sole test I
+            have that exercises this code, but it'll soon go, to be
+            replaced by a use of unlinkat-with-AT_REMOVEDIR.  */
          if (rmdir (dir) == 0)
            return RM_OK;
 
Index: tests/rm/empty-inacc
===================================================================
RCS file: /fetish/cu/tests/rm/empty-inacc,v
retrieving revision 1.1
diff -u -p -r1.1 empty-inacc
--- tests/rm/empty-inacc        11 Feb 2006 18:03:29 -0000      1.1
+++ tests/rm/empty-inacc        26 Jun 2006 12:12:19 -0000
@@ -18,6 +18,10 @@ mkdir -p $tmp || framework_failure=1
 cd $tmp || framework_failure=1
 mkdir -m0 inacc || framework_failure=1
 
+# Also exercise the different code path that's taken for a directory
+# that is empty (hence removable) and unreadable.
+mkdir -m a-r -p a/unreadable
+
 if test $framework_failure = 1; then
   echo "$0: failure in testing framework" 1>&2
   (exit 1); exit 1
@@ -29,4 +33,8 @@ fail=0
 rm -rf inacc || fail=1
 test -d inacc && fail=1
 
+# This would fail for e.g., coreutils-5.97.
+rm -rf a || fail=1
+test -d a && fail=1
+
 (exit $fail); exit $fail