rmdir: diagnose non following of symlinks with trailing slash

GNU/Linux is unusual here in that rmdir("symlink/") returns ENOTDIR,
whereas Solaris and FreeBSD at least, will follow the symlink
and remove the target directory.  We don't make the behavior
on Linux kernels consistent, but at least clarify
the confusing error message.

* src/rmdir (main): Output a specific error message for the above case.
(remove_parents): In the error message, don't assume intermediate paths
are directories, as they could be symlinks.
* tests/rmdir/symlink-errors.sh: Add a new test.
* tests/local.mk: Reference the new test.
* NEWS: Mention the improvement.

4 files changed, 119 insertions, 6 deletions

diff --git a/NEWS b/NEWS
index 08a58d56a..aad05df6d 100644
--- a/NEWS
+++ b/NEWS
@@ -75,6 +75,9 @@ GNU coreutils NEWS                                    -*- outline -*-
   df now recognizes these file systems as remote:
   acfs, coda, fhgfs, gpfs, ibrix, ocfs2, and vxfs.
 
+  rmdir now clarifies the error if a symlink_to_dir/ has not been traversed.
+  This is the case on GNU/Linux systems, where the trailing slash is ignored.
+
   stat and tail now know about the "devmem", "exfat", "vboxsf", and "zonefs"
   file system types.  stat -f -c%T now reports the file system type,
   and tail -f uses polling for "vboxsf" and inotify for the others.
diff --git a/src/rmdir.c b/src/rmdir.c
index dffe24bc7..3a9911ff0 100644
--- a/src/rmdir.c
+++ b/src/rmdir.c
@@ -144,9 +144,19 @@ remove_parents (char *dir)
             }
           else
             {
-              /* Barring race conditions, DIR is expected to be a directory.  */
-              error (0, rmdir_errno, _("failed to remove directory %s"),
-                     quoteaf (dir));
+              char const* error_msg;
+              if (rmdir_errno != ENOTDIR)
+                {
+                  /* Barring race conditions,
+                     DIR is expected to be a directory.  */
+                  error_msg = N_("failed to remove directory %s");
+                }
+              else
+                {
+                  /* A path component could be a symbolic link */
+                  error_msg = N_("failed to remove %s");
+                }
+              error (0, rmdir_errno, _(error_msg), quoteaf (dir));
             }
           break;
         }
@@ -238,9 +248,42 @@ main (int argc, char **argv)
           if (ignorable_failure (rmdir_errno, dir))
             continue;
 
-          /* Here, the diagnostic is less precise, since we have no idea
-             whether DIR is a directory.  */
-          error (0, rmdir_errno, _("failed to remove %s"), quoteaf (dir));
+          /* Distinguish the case for a symlink with trailing slash.
+             On Linux, rmdir(2) confusingly does not follow the symlink,
+             thus giving the errno ENOTDIR, while on other systems the symlink
+             is followed.  We don't provide consistent behavior here,
+             but at least we provide a more accurate error message.  */
+          bool custom_error = false;
+          if (rmdir_errno == ENOTDIR)
+            {
+              char const *last_unix_slash = strrchr (dir, '/');
+              if (last_unix_slash && (*(last_unix_slash + 1) == '\0'))
+                {
+                  struct stat st;
+                  int ret = stat (dir, &st);
+                  /* Some other issue following, or is actually a directory. */
+                  if ((ret != 0 && errno != ENOTDIR) || S_ISDIR (st.st_mode))
+                    {
+                      /* Ensure the last component was a symlink.  */
+                      char* dir_arg = xstrdup (dir);
+                      strip_trailing_slashes (dir);
+                      ret = lstat (dir, &st);
+                      if (ret == 0 && S_ISLNK (st.st_mode))
+                        {
+                          error (0, 0,
+                                 _("failed to remove %s:"
+                                   " Symbolic link not followed"),
+                                 quoteaf (dir_arg));
+                          custom_error = true;
+                        }
+                      free (dir_arg);
+                    }
+                }
+            }
+
+          if (! custom_error)
+            error (0, rmdir_errno, _("failed to remove %s"), quoteaf (dir));
+
           ok = false;
         }
       else if (remove_empty_parents)
diff --git a/tests/local.mk b/tests/local.mk
index a6d414581..27e31ec8e 100644
--- a/tests/local.mk
+++ b/tests/local.mk
@@ -695,6 +695,7 @@ all_tests =					\
   tests/readlink/rl-1.sh			\
   tests/rmdir/fail-perm.sh			\
   tests/rmdir/ignore.sh				\
+  tests/rmdir/symlink-errors.sh			\
   tests/rmdir/t-slash.sh			\
   tests/tail-2/assert-2.sh			\
   tests/tail-2/big-4gb.sh			\
diff --git a/tests/rmdir/symlink-errors.sh b/tests/rmdir/symlink-errors.sh
new file mode 100755
index 000000000..911835095
--- /dev/null
+++ b/tests/rmdir/symlink-errors.sh
@@ -0,0 +1,66 @@
+#!/bin/sh
+# make sure rmdir outputs clear errors in the presence of symlinks
+
+# Copyright (C) 2021 Free Software Foundation, Inc.
+
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <https://www.gnu.org/licenses/>.
+
+. "${srcdir=.}/tests/init.sh"; path_prepend_ ./src
+print_ver_ rmdir
+
+mkdir dir || framework_failure_
+ln -s dir sl || framework_failure_
+ln -s missing dl || framework_failure_
+touch file || framework_failure_
+ln -s file fl || framework_failure_
+
+# Ensure a we maintain ENOTDIR so that we provide
+# accurate errors on systems on which rmdir(2) does following the symlink/
+returns_ 1 rmdir fl/ 2> err || fail=1
+# Ensure we diagnose symlink behavior.
+printf '%s\n' "rmdir: failed to remove 'fl/': Not a directory" > exp
+compare exp err || fail=1
+
+# Also ensure accurate errors from rmdir -p when traversing symlinks
+# Up to and including 8.32 rmdir would fail like:
+#   rmdir: failed to remove directory 'sl': Not a directory
+mkdir dir/dir2 || framework_failure_
+returns_ 1 rmdir -p sl/dir2 2> err || fail=1
+# Ensure we diagnose symlink behavior.
+printf '%s\n' "rmdir: failed to remove 'sl': Not a directory" > exp
+compare exp err || fail=1
+
+# Only perform the following on systems that don't follow the symlink
+if ! rmdir sl/ 2>/dev/null; then
+  # Up to and including 8.32 rmdir would fail like:
+  #   rmdir: failed to remove 'sl/': Not a directory
+  # That's inconsistent though as rm sl/ gives:
+  #   rm: cannot remove 'sl/': Is a directory
+  # Also this is inconsistent with other systems
+  # which do follow the symlink and rmdir the target.
+
+  new_error="rmdir: failed to remove '%s': Symbolic link not followed\\n"
+
+  # Ensure we diagnose symlink behavior.
+  returns_ 1 rmdir sl/ 2> err || fail=1
+  printf "$new_error" 'sl/' > exp || framework_failure_
+  compare exp err || fail=1
+
+  # Ensure a consistent diagnosis for dangling symlinks etc.
+  returns_ 1 rmdir dl/ 2> err || fail=1
+  printf "$new_error" 'dl/' > exp || framework_failure_
+  compare exp err || fail=1
+fi
+
+Exit $fail