bug#12339: Bug: rm -fr . doesn't dir depth first deletion yet it is documented to do so.

[Jim Meyering]

Jim Meyering wrote:
...
> With the following patch, I see new behavior.
> It's an improvement, but we're still not there:
>
>     $ mkdir -p d/e/f; ln -s d s; rm -r s/
>     rm: cannot remove 's/': Not a directory
>     [Exit 1]
>     $ find
>     .
>     ./s
>     ./d
>
> Notice how it did traverse s/ into d/, and removed d/e and d/e/f.
> The only problem is that when it attempted to remove the command-line
> specified "s/", unlinkat (AT_FDCWD, "s/", AT_REMOVEDIR) failed:
>
>     unlinkat(4, "d", 0)                     = 0
>     unlinkat(5, "f", AT_REMOVEDIR)          = 0
>     unlinkat(4, "e", AT_REMOVEDIR)          = 0
>     unlinkat(AT_FDCWD, "s/", AT_REMOVEDIR)  = -1 ENOTDIR (Not a directory)
>     rm: cannot remove 's/': Not a directory
>     +++ exited with 1 +++
>
> Now, this looks like a problem with unlinkat.

That's probably a problem with unlinkat,
but does not explain what caused the invalid diagnostic.
BTW, while it was easy for me to discover that this bug
was introduced between 8.5 and 8.6, neither rm.c nor remove.c
had significant changes in that delta.

And bisecting would have been a chore, since bootstrapping those versions
is, um... challenging.  That made me want to make each package in the
volatile tool chain (m4, autoconf, automake and maybe even bison, too)
into a submodule of coreutils.  Currently it's trivial to build coreutils
from a tarball, but it's not at all easy to bootstrap from an arbitrary
version in git.

Anyway, I ended up comparing strace output from 8.5's rm to that of
the rm from 8.6.  Here's the key difference:

  -openat(AT_FDCWD, "s", O_RDONLY|O_NOCTTY|O_NONBLOCK|O_DIRECTORY) = 3
  +openat(AT_FDCWD, "s", O_RDONLY|O_NOCTTY|O_NONBLOCK|O_DIRECTORY|O_NOFOLLOW) = 
-1 ELOOP (Too many levels of symbolic links)
   ...
   unlinkat(AT_FDCWD, "s", AT_REMOVEDIR)   = -1 ENOTDIR (Not a directory)

The directory-opening code in gnulib's fts.c, which rm uses for
recursive traversal, was fixed to avoid a race condition:

commit 0971365ee6f6638cbeec77db08fbdfe5f3bab53d
Author: Paul Eggert <address@hidden>
Date:   Mon Sep 13 12:38:41 2010 -0700

    fts: use O_NOFOLLOW to avoid race condition when opening a directory

    * lib/fts.c (opendirat): New arg extra_flags.
    (__opendir2): Use it to avoid following symlinks when opening
    a directory, if symlinks are not supposed to be followed.  See
    <http://lists.gnu.org/archive/html/bug-gnulib/2010-09/msg00213.html>.

That was clearly a good change.
What it did however, was to trigger a problem in remove.c,
making it print this bogus diagnostic:

    rm: cannot remove 's': Too many levels of symbolic links

Here's the bad code:

  /* When failing to rmdir an unreadable directory, the typical
     errno value is EISDIR, but that is not as useful to the user
     as the errno value from the failed open (probably EPERM).
     Use the earlier, more descriptive errno value.  */
  if (ent->fts_info == FTS_DNR)
    errno = ent->fts_errno;
  error (0, errno, _("cannot remove %s"), quote (ent->fts_path));

The fts change affected this code by making the "if" condition true.
Before, the openat succeeded.  Now, it fails with ELOOP.
By telling openat not to follow symlinks, we've made fts set ->fts_info
to FTS_DNR (unreadable directory), and that made remove.c use the
prior errno value (the bogus ELOOP).

While not necessary to fix the problem at hand, since I made
it so the trailing slash is preserved down to the openat syscall,
this additional change might avoid a similar problem down the road:

-  /* When failing to rmdir an unreadable directory, the typical
-     errno value is EISDIR, but that is not as useful to the user
-     as the errno value from the failed open (probably EPERM).
-     Use the earlier, more descriptive errno value.  */
-  if (ent->fts_info == FTS_DNR)
+  /* When failing to rmdir an unreadable directory, the typical errno value
+     is EISDIR or ENOTDIR, but that would be meaningless in a diagnostic.
+     When that happens and the errno value from the failed open is EPERM
+     or EACCES, use the earlier, more descriptive errno value.  */
+  if (ent->fts_info == FTS_DNR
+      && (errno == ENOTEMPTY || errno == EISDIR || errno == ENOTDIR)
+      && (ent->fts_errno == EPERM || ent->fts_errno == EACCES))
     errno = ent->fts_errno;
   error (0, errno, _("cannot remove %s"), quote (ent->fts_path));
   mark_ancestor_dirs (ent);

I'll post this new pair of rm-related patches in a minute.