bug-coreutils
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: rm: avoiding a race condition on non-glibc systems


From: Paul Eggert
Subject: Re: rm: avoiding a race condition on non-glibc systems
Date: Fri, 13 May 2005 15:55:20 -0700
User-agent: Gnus/5.1006 (Gnus v5.10.6) Emacs/21.4 (gnu/linux)

Jim Meyering <address@hidden> writes:

> So the current test of __GLIBC__ may be wrong if the
> underlying kernel is not Linux.

A few thoughts.

It'd be nice to factor out this "can unlink(2) remove directories"
business into a gnulib module.  Tar could use it, for example.

On traditional Unix, only the superuser can unlink directories.  I had
thought that "rm" used this to decide whether to use the slow method
(i.e., the method that worries about whether unlink() can remove
directories).  But now that I look at the code, I see that it's purely
a compile-time test.  It would be more efficient on traditional Unix
hosts if we used a run-time test, for the common case where "rm" is
not being run as root.  Also, I think the code would be a bit cleaner,
as it's one less #ifdef.

On Solaris 10, as I understand it, a process can give up its privilege
to unlink directories (if it has it).  That would be a win, no?  (I
don't have easy access to a Solaris 10 system to check this.)

FreeBSD (since 2.2) doesn't let unlink(2) remove directories.
At least, that's what the man pages say.

How about this patch?  It incorporates the above ideas.

2005-05-13  Paul Eggert  <address@hidden>

        * m4/prereqs.m4 (gl_PREREQ): Require gl_UNLINKDIR.
        * src/remove.c: Include unlinkdir.h.
        (UNLINK_CAN_UNLINK_DIRS): Remove.
        (remove_entry): Use cannot_unlink_dirs () rather than
        UNLINK_CAN_UNLINK_DIRS.
        * lib/unlinkdir.c, lib/unlinkdir.h: New files.
        * m4/unlinkdir.m4: New file.

Index: m4/prereq.m4
===================================================================
RCS file: /fetish/cu/m4/prereq.m4,v
retrieving revision 1.108
diff -p -u -r1.108 prereq.m4
--- m4/prereq.m4        29 Apr 2005 05:37:32 -0000      1.108
+++ m4/prereq.m4        13 May 2005 22:45:10 -0000
@@ -114,6 +114,7 @@ AC_DEFUN([gl_PREREQ],
   AC_REQUIRE([gl_TIMESPEC])
   AC_REQUIRE([gl_UNICODEIO])
   AC_REQUIRE([gl_UNISTD_SAFER])
+  AC_REQUIRE([gl_UNLINKDIR])
   AC_REQUIRE([gl_USERSPEC])
   AC_REQUIRE([gl_UTIMECMP])
   AC_REQUIRE([gl_UTIMENS])
Index: src/remove.c
===================================================================
RCS file: /fetish/cu/src/remove.c,v
retrieving revision 1.122
diff -p -u -r1.122 remove.c
--- src/remove.c        13 May 2005 08:42:35 -0000      1.122
+++ src/remove.c        13 May 2005 22:45:11 -0000
@@ -36,6 +36,7 @@
 #include "quote.h"
 #include "remove.h"
 #include "root-dev-ino.h"
+#include "unlinkdir.h"
 #include "yesno.h"
 
 /* Avoid shadowing warnings because these are functions declared
@@ -46,16 +47,6 @@
 #define obstack_chunk_alloc malloc
 #define obstack_chunk_free free
 
-/* If anyone knows of another system for which unlink can never
-   remove a directory, please report it to address@hidden
-   The code below is slightly more efficient if it *knows* that
-   unlink(2) cannot possibly unlink a directory.  */
-#ifdef __GLIBC__
-# define UNLINK_CAN_UNLINK_DIRS 0  /* Good!  */
-#else
-# define UNLINK_CAN_UNLINK_DIRS 1  /* Less efficient.  */
-#endif
-
 /* This is the maximum number of consecutive readdir/unlink calls that
    can be made (with no intervening rewinddir or closedir/opendir)
    before triggering a bug that makes readdir return NULL even though
@@ -729,111 +720,110 @@ remove_entry (Dirstack_state const *ds, 
   if (s != RM_OK)
     return s;
 
-  /* Why bother with the following #if/#else block?  Because on systems with
+  /* Why bother with the following if/else block?  Because on systems with
      an unlink function that *can* unlink directories, we must determine the
      type of each entry before removing it.  Otherwise, we'd risk unlinking
      an entire directory tree simply by unlinking a single directory;  then
      all the storage associated with that hierarchy would not be freed until
-     the next reboot.  Not nice.  To avoid that, on such slightly losing
+     the next fsck.  Not nice.  To avoid that, on such slightly losing
      systems, we need to call lstat to determine the type of each entry,
      and that represents extra overhead that -- it turns out -- we can
-     avoid on GNU-libc-based systems, since there, unlink will never remove
+     avoid on non-losing systems, since there, unlink will never remove
      a directory.  Also, on systems where unlink may unlink directories,
      we're forced to allow a race condition: we lstat a non-directory, then
      go to unlink it, but in the mean time, a malicious someone has replaced
      it with a directory.  */
 
-#if UNLINK_CAN_UNLINK_DIRS
-
-  /* If we don't already know whether FILENAME is a directory, find out now.
-     Then, if it's a non-directory, we can use unlink on it.  */
-  if (is_dir == T_UNKNOWN)
+  if (cannot_unlink_dir ())
     {
-# if HAVE_STRUCT_DIRENT_D_TYPE
-      if (dp && dp->d_type != DT_UNKNOWN)
-       is_dir = DT_IS_DIR (dp) ? T_YES : T_NO;
-      else
-# endif
+      if (is_dir == T_YES && ! x->recursive)
        {
-         struct stat sbuf;
-         if (lstat (filename, &sbuf))
-           {
-             if (errno == ENOENT && x->ignore_missing_files)
-               return RM_OK;
-
-             error (0, errno,
-                    _("cannot lstat %s"), quote (full_filename (filename)));
-             return RM_ERROR;
-           }
-
-         is_dir = S_ISDIR (sbuf.st_mode) ? T_YES : T_NO;
+         error (0, EISDIR, _("cannot remove directory %s"),
+                quote (full_filename (filename)));
+         return RM_ERROR;
        }
-    }
 
-  if (is_dir == T_NO)
-    {
-      /* At this point, barring race conditions, FILENAME is known
-        to be a non-directory, so it's ok to try to unlink it.  */
+      /* is_empty_directory is set iff it's ok to use rmdir.
+        Note that it's set only in interactive mode -- in which case it's
+        an optimization that arranges so that the user is asked just
+        once whether to remove the directory.  */
+      if (is_empty_directory == T_YES)
+       DO_RMDIR (filename, x);
+
+      /* If we happen to know that FILENAME is a directory, return now
+        and let the caller remove it -- this saves the overhead of a failed
+        unlink call.  If FILENAME is a command-line argument, then dp is NULL,
+        so we'll first try to unlink it.  Using unlink here is ok, because it
+        cannot remove a directory.  */
+      if ((dp && DT_IS_DIR (dp)) || is_dir == T_YES)
+       return RM_NONEMPTY_DIR;
+
       DO_UNLINK (filename, x);
 
-      /* unlink failed with some other error code.  report it.  */
-      error (0, errno, _("cannot remove %s"),
-            quote (full_filename (filename)));
-      return RM_ERROR;
+      if (! x->recursive
+         || errno == ENOENT || errno == ENOTDIR
+         || errno == ELOOP || errno == ENAMETOOLONG)
+       {
+         /* Either --recursive is not in effect, or the file cannot be a
+            directory.  Report the unlink problem and fail.  */
+         error (0, errno, _("cannot remove %s"),
+                quote (full_filename (filename)));
+         return RM_ERROR;
+       }
     }
-
-  if (! x->recursive)
+  else
     {
-      error (0, EISDIR, _("cannot remove directory %s"),
-            quote (full_filename (filename)));
-      return RM_ERROR;
-    }
+      /* If we don't already know whether FILENAME is a directory, find out 
now.
+        Then, if it's a non-directory, we can use unlink on it.  */
+      if (is_dir == T_UNKNOWN)
+       {
+# if HAVE_STRUCT_DIRENT_D_TYPE
+         if (dp && dp->d_type != DT_UNKNOWN)
+           is_dir = DT_IS_DIR (dp) ? T_YES : T_NO;
+         else
+# endif
+           {
+             struct stat sbuf;
+             if (lstat (filename, &sbuf))
+               {
+                 if (errno == ENOENT && x->ignore_missing_files)
+                   return RM_OK;
+
+                 error (0, errno, _("cannot lstat %s"),
+                        quote (full_filename (filename)));
+                 return RM_ERROR;
+               }
 
-  if (is_empty_directory == T_YES)
-    {
-      DO_RMDIR (filename, x);
-      /* Don't diagnose any failure here.
-        It'll be detected when the caller tries another way.  */
-    }
+             is_dir = S_ISDIR (sbuf.st_mode) ? T_YES : T_NO;
+           }
+       }
 
+      if (is_dir == T_NO)
+       {
+         /* At this point, barring race conditions, FILENAME is known
+            to be a non-directory, so it's ok to try to unlink it.  */
+         DO_UNLINK (filename, x);
 
-#else /* ! UNLINK_CAN_UNLINK_DIRS */
+         /* unlink failed with some other error code.  report it.  */
+         error (0, errno, _("cannot remove %s"),
+                quote (full_filename (filename)));
+         return RM_ERROR;
+       }
 
-  if (is_dir == T_YES && ! x->recursive)
-    {
-      error (0, EISDIR, _("cannot remove directory %s"),
-            quote (full_filename (filename)));
-      return RM_ERROR;
-    }
+      if (! x->recursive)
+       {
+         error (0, EISDIR, _("cannot remove directory %s"),
+                quote (full_filename (filename)));
+         return RM_ERROR;
+       }
 
-  /* is_empty_directory is set iff it's ok to use rmdir.
-     Note that it's set only in interactive mode -- in which case it's
-     an optimization that arranges so that the user is asked just
-     once whether to remove the directory.  */
-  if (is_empty_directory == T_YES)
-    DO_RMDIR (filename, x);
-
-  /* If we happen to know that FILENAME is a directory, return now
-     and let the caller remove it -- this saves the overhead of a failed
-     unlink call.  If FILENAME is a command-line argument, then dp is NULL,
-     so we'll first try to unlink it.  Using unlink here is ok, because it
-     cannot remove a directory.  */
-  if ((dp && DT_IS_DIR (dp)) || is_dir == T_YES)
-    return RM_NONEMPTY_DIR;
-
-  DO_UNLINK (filename, x);
-
-  if (! x->recursive
-      || errno == ENOENT || errno == ENOTDIR
-      || errno == ELOOP || errno == ENAMETOOLONG)
-    {
-      /* Either --recursive is not in effect, or the file cannot be a
-        directory.  Report the unlink problem and fail.  */
-      error (0, errno, _("cannot remove %s"),
-            quote (full_filename (filename)));
-      return RM_ERROR;
+      if (is_empty_directory == T_YES)
+       {
+         DO_RMDIR (filename, x);
+         /* Don't diagnose any failure here.
+            It'll be detected when the caller tries another way.  */
+       }
     }
-#endif
 
   return RM_NONEMPTY_DIR;
 }
--- /dev/null   2003-03-18 13:55:57 -0800
+++ lib/unlinkdir.h     2005-05-13 15:41:33 -0700
@@ -0,0 +1,27 @@
+/* unlinkdir.h - determine (and maybe change) whether we can unlink directories
+
+   Copyright (C) 2005 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 2, 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, write to the Free Software Foundation,
+   Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA.  */
+
+/* Written by Paul Eggert and Jim Meyering.  */
+
+#include <stdbool.h>
+
+#if UNLINK_CANNOT_UNLINK_DIR
+static bool cannot_unlink_dir (void) { return true; }
+#else
+bool cannot_unlink_dir (void);
+#endif
--- /dev/null   2003-03-18 13:55:57 -0800
+++ lib/unlinkdir.c     2005-05-13 15:41:26 -0700
@@ -0,0 +1,70 @@
+/* unlinkdir.c - determine (and maybe change) whether we can unlink directories
+
+   Copyright (C) 2005 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 2, 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, write to the Free Software Foundation,
+   Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA.  */
+
+/* Written by Paul Eggert and Jim Meyering.  */
+
+#include <config.h>
+
+#include "unlinkdir.h"
+
+#if HAVE_PRIV_H
+# include <priv.h>
+#endif
+#if HAVE_UNISTD_H
+# include <unistd.h>
+#endif
+
+#if ! UNLINK_CANNOT_UNLINK_DIR
+
+/* Return true if we cannot unlink directories, false if we might be
+   able to unlink directories.  If possible, tell the kernel we don't
+   want to be able to unlink directories, so that we can return true.  */
+
+bool
+cannot_unlink_dir (void)
+{
+  static bool initialized;
+  static bool cannot;
+
+  if (! initialized)
+    {
+#if defined PRIV_EFFECTIVE && defined PRIV_SYS_LINKDIR
+      /* We might be able to unlink directories if we cannot
+        determine our privileges, or if we have the
+        PRIV_SYS_LINKDIR privilege and cannot delete it.  */
+      priv_set_t *pset = priv_allocset ();
+      if (pset)
+       {
+         cannot =
+           (getppriv (PRIV_EFFECTIVE, pset) == 0
+            && (! priv_ismember (pset, PRIV_SYS_LINKDIR)
+                || (priv_delset (pset, PRIV_SYS_LINKDIR) == 0
+                    && setppriv (PRIV_SET, PRIV_EFFECTIVE, pset) == 0)));
+         priv_freeset (pset);
+       }
+#else
+      /* In traditional Unix, only root can unlink directories.  */
+      cannot = (getuid () != 0);
+#endif
+    }
+
+  initialized = true;
+  return cannot;
+}
+
+#endif
--- /dev/null   2003-03-18 13:55:57 -0800
+++ m4/unlinkdir.m4     2005-05-13 15:41:12 -0700
@@ -0,0 +1,30 @@
+#serial 1
+
+# Copyright (C) 2005 Free Software Foundation, Inc.
+#
+# This file is free software; the Free Software Foundation
+# gives unlimited permission to copy and/or distribute it,
+# with or without modifications, as long as this notice is preserved.
+
+# Written by Paul Eggert.
+
+AC_DEFUN([gl_UNLINKDIR],
+[
+  AC_REQUIRE([AC_CANONICAL_HOST])
+  AC_CHECK_HEADERS_ONCE(priv.h unistd.h)
+
+  # The Hurd, the Linux kernel, and the FreeBSD kernel version 2.2 and later
+  # never let anyone (even root) unlink directories.
+  # If anyone knows of another system for which unlink can never
+  # remove a directory, please report it to <address@hidden>.
+  # Unfortunately this is difficult to test for, since it requires root access
+  # and might create garbage in the file system,
+  # so the code below simply relies on the kernel name and version number.
+  case $host in
+  *-*-gnu[0-9]* | \
+  *-*-linux-* | *-*-linux | \
+  *-*-freebsd2.2* | *-*-freebsd[3-9]* | *-*-freebsd[1-9][0-9]*)
+    AC_DEFINE([UNLINK_CANNOT_UNLINK_DIR], 1,
+      [Define to 1 if unlink (dir) cannot possibly succeed.]);;
+  esac
+])




reply via email to

[Prev in Thread] Current Thread [Next in Thread]