From: Jim Meyering <jim@meyering.net>
Date: Sat, 22 Sep 2007 08:02:09 +0000 (+0200)
Subject: rm: give a sensible diagnostic when failing to remove a symlink
X-Git-Tag: v6.9.89~100
X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=a7ec8caffe1a48590f5e3da9400080ab8a6ec303;p=platform%2Fupstream%2Fcoreutils.git

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
--- 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
--- 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
--- 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
--- 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
--- 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