rm: give a sensible diagnostic when failing to remove a symlink

On some systems (those with openat et al), when rm would fail to
remove a symlink, it would fail with the misleading diagnostic,
"Too many levels of symbolic links".
* NEWS: Mention the bug fix.
* src/remove.c (is_nondir_lstat): New function.
(remove_entry): Use it to catch failed-to-remove symlink (and any
other non-dir) here so that we don't fall through and try to treat
it as directory, which -- with a symlink -- would provoke the bogus
ELOOP failure.
* tests/rm/fail-eacces: Add a test for the above.
* src/c99-to-c89.diff: Adjust offsets.

diff --git a/ChangeLog b/ChangeLog
index 730ba22..d96f4aa 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,18 @@
 2007-09-22  Jim Meyering  <jim@meyering.net>
 
+       rm: give a sensible diagnostic when failing to remove a symlink
+       On some systems (those with openat et al), when rm would fail to
+       remove a symlink, it would fail with the misleading diagnostic,
+       "Too many levels of symbolic links".
+       * NEWS: Mention the bug fix.
+       * src/remove.c (is_nondir_lstat): New function.
+       (remove_entry): Use it to catch failed-to-remove symlink (and any
+       other non-dir) here so that we don't fall through and try to treat
+       it as directory, which -- with a symlink -- would provoke the bogus
+       ELOOP failure.
+       * tests/rm/fail-eacces: Add a test for the above.
+       * src/c99-to-c89.diff: Adjust offsets.
+
        rm: fix a tiny, nearly inconsequential bug.
        Don't perform a "."-relative lstat, when the file in question
        may well not be in ".".  Although this is a bug, a few attempts

diff --git a/NEWS b/NEWS
index 93a632c..e8e20e0 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -202,6 +202,10 @@ GNU coreutils NEWS                                    -*- outline -*-
 
   "rm --interactive=never F" no longer prompts for an unwritable F
 
+  "rm -rf D" would emit an misleading diagnostic when failing to
+  remove a symbolic link within the unwritable directory, D.
+  Introduced in coreutils-6.0.
+
 ** New features
 
   sort's new --compress-program=PROG option specifies a compression

diff --git a/src/c99-to-c89.diff b/src/c99-to-c89.diff
index 3e66bc4..01e213b 100644 (file)
--- a/src/c99-to-c89.diff
+++ b/src/c99-to-c89.diff
@@ -42,7 +42,7 @@ diff -upr src/remove.c src/remove.c
 
        if (!yesno ())
        return RM_USER_DECLINED;
-@@ -1515,6 +1519,7 @@ rm_1 (Dirstack_state *ds, char const *fi
+@@ -1533,6 +1537,7 @@ rm_1 (Dirstack_state *ds, char const *fi
        return RM_ERROR;
      }
 
@@ -50,7 +50,7 @@ diff -upr src/remove.c src/remove.c
    struct stat st;
    cache_stat_init (&st);
    cycle_check_init (&ds->cycle_check_state);
-@@ -1537,6 +1542,7 @@ rm_1 (Dirstack_state *ds, char const *fi
+@@ -1555,6 +1560,7 @@ rm_1 (Dirstack_state *ds, char const *fi
    AD_push_initial (ds);
    AD_INIT_OTHER_MEMBERS ();
 
@@ -58,7 +58,7 @@ diff -upr src/remove.c src/remove.c
    enum RM_status status = remove_entry (AT_FDCWD, ds, filename,
                                        DT_UNKNOWN, &st, x);
    if (status == RM_NONEMPTY_DIR)
-@@ -1555,6 +1561,8 @@ rm_1 (Dirstack_state *ds, char const *fi
+@@ -1573,6 +1579,8 @@ rm_1 (Dirstack_state *ds, char const *fi
    ds_clear (ds);
    return status;
  }

diff --git a/src/remove.c b/src/remove.c
index aae7a88..ac10109 100644 (file)
--- a/src/remove.c
+++ b/src/remove.c
@@ -937,6 +937,21 @@ is_dir_lstat (int fd_cwd, char const *filename, struct stat *st)
   return is_dir;
 }
 
+/* Return true if FILENAME is a non-directory.
+   Otherwise, including the case in which lstat fails, return false.
+   *ST is FILENAME's tstatus.
+   Do not modify errno.  */
+static inline bool
+is_nondir_lstat (int fd_cwd, char const *filename, struct stat *st)
+{
+  int saved_errno = errno;
+  bool is_non_dir =
+    (cache_fstatat (fd_cwd, filename, st, AT_SYMLINK_NOFOLLOW) == 0
+     && !S_ISDIR (st->st_mode));
+  errno = saved_errno;
+  return is_non_dir;
+}
+
 #define DO_UNLINK(Fd_cwd, Filename, X)                                 \
   do                                                                   \
     {                                                                  \
@@ -1066,7 +1081,9 @@ remove_entry (int fd_cwd, Dirstack_state const *ds, char const *filename,
        errno = EISDIR;
 
       if (! x->recursive
-         || (cache_stat_ok (st) && !S_ISDIR (st->st_mode)))
+         || (cache_stat_ok (st) && !S_ISDIR (st->st_mode))
+         || (errno == EACCES && is_nondir_lstat (fd_cwd, filename, st))
+         )
        {
          if (ignorable_missing (x, errno))
            return RM_OK;

diff --git a/tests/rm/fail-eacces b/tests/rm/fail-eacces
index b15c2f5..d49764c 100755 (executable)
--- a/tests/rm/fail-eacces
+++ b/tests/rm/fail-eacces
@@ -1,5 +1,8 @@
 #!/bin/sh
 # Ensure that rm -rf unremovable-non-dir gives a diagnostic.
+# Test both a regular file and a symlink -- it makes a difference to rm.
+# With the symlink, rm from coreutils-6.9 would fail with a misleading
+# ELOOP diagnostic.
 
 # Copyright (C) 2006-2007 Free Software Foundation, Inc.
 
@@ -25,7 +28,19 @@ fi
 . $srcdir/../test-lib.sh
 skip_if_root_
 
-mkdir d && touch d/f && chmod a-w d || framework_failure
+ok=0
+mkdir d           &&
+  touch d/f       &&
+  ln -s f d/slink &&
+  chmod a-w d     &&
+  ok=1
+test $ok = 1 || framework_failure
+
+mkdir e           &&
+  ln -s f e/slink &&
+  chmod a-w e     &&
+  ok=1
+test $ok = 1 || framework_failure
 
 fail=0
 
@@ -33,7 +48,13 @@ rm -rf d/f 2> out && fail=1
 cat <<\EOF > exp
 rm: cannot remove `d/f': Permission denied
 EOF
+compare out exp || fail=1
 
+# This used to fail with ELOOP.
+rm -rf e 2> out && fail=1
+cat <<\EOF > exp
+rm: cannot remove `e/slink': Permission denied
+EOF
 compare out exp || fail=1
 
 (exit $fail); exit $fail