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.

diff --git a/NEWS b/NEWS
index 08a58d56a68d5f8e183cc678cff4d8194ed291bb..aad05df6db629bd10c2c72ecfe52aaaba13849bd 100644 (file)
--- 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 dffe24bc74d48db4cd5fa71eb84abf730f76211c..3a9911ff06a54f079aaa555a403248a63ae8b372 100644 (file)
--- 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 a6d4145816e2ff94c57c66057c0e05a40bd46a52..27e31ec8e40391feee533e6cda1d9393c93fb443 100644 (file)
--- 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 (executable)
index 0000000..9118350
--- /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