Re: rm: avoiding a race condition on non-glibc systems
Eric Blake [EMAIL PROTECTED] writes:
This change broke cygwin. Cygwin does not have struct dirent.d_type, so
DT_IS_DIR is defined as do_not_use_this_macro. I think protecting this if
statement with HAVE_STRUCT_DIRENT_D_TYPE, and letting cygwin fall through
to the unlink, will fix the problem.
Thanks for reporting it. I'd rather avoid yet another #if inside
expressions, so I installed the following patch instead. The main
idea is to remove DT_IS_DIR (with its confusing semantics) and replace
it with DT_IS_KNOWN and DT_MUST_BE (with, I hope, less confusing
semantics :-). This lets us remove an #if rather than add one.
2005-05-16 Paul Eggert [EMAIL PROTECTED]
Fix Cygwin porting problem reported by Eric Blake.
* src/remove.c (DT_IS_DIR): Remove.
(DT_IS_KNOWN, DT_MUST_BE): New macros.
(remove_entry): Use them.
--- remove.c14 May 2005 08:05:35 - 1.125
+++ remove.c16 May 2005 20:28:53 - 1.126
@@ -664,10 +664,16 @@ prompt (Dirstack_state const *ds, char c
}
#if HAVE_STRUCT_DIRENT_D_TYPE
-# define DT_IS_DIR(D) ((D)-d_type == DT_DIR)
+
+/* True if the type of the directory entry D is known. */
+# define DT_IS_KNOWN(d) ((d)-d_type != DT_UNKNOWN)
+
+/* True if the type of the directory entry D must be T. */
+# define DT_MUST_BE(d, t) ((d)-d_type == (t))
+
#else
-/* Use this only if the member exists -- i.e., don't return 0. */
-# define DT_IS_DIR(D) do_not_use_this_macro
+# define DT_IS_KNOWN(d) false
+# define DT_MUST_BE(d, t) false
#endif
#define DO_UNLINK(Filename, X) \
@@ -755,7 +761,7 @@ remove_entry (Dirstack_state const *ds,
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)
+ if ((dp DT_MUST_BE (dp, DT_DIR)) || is_dir == T_YES)
return RM_NONEMPTY_DIR;
DO_UNLINK (filename, x);
@@ -777,11 +783,9 @@ remove_entry (Dirstack_state const *ds,
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;
+ if (dp DT_IS_KNOWN (dp))
+ is_dir = DT_MUST_BE (dp, DT_DIR) ? T_YES : T_NO;
else
-#endif
{
struct stat sbuf;
if (lstat (filename, sbuf))
___
Bug-coreutils mailing list
Bug-coreutils@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-coreutils
Re: rm: avoiding a race condition on non-glibc systems
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 According to Paul Eggert on 5/13/2005 4:55 PM: 2005-05-13 Paul Eggert [EMAIL PROTECTED] * 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. + /* 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; + This change broke cygwin. Cygwin does not have struct dirent.d_type, so DT_IS_DIR is defined as do_not_use_this_macro. I think protecting this if statement with HAVE_STRUCT_DIRENT_D_TYPE, and letting cygwin fall through to the unlink, will fix the problem. 2005-05-16 Eric Blake [EMAIL PROTECTED] * src/remove.c (remove_entry) [! HAVE_STRUCT_DIRENT_D_TYPE]: Fix breakage on Cygwin when checking for directory. Index: src/remove.c === RCS file: /cvsroot/coreutils/coreutils/src/remove.c,v retrieving revision 1.125 diff -u -p -r1.125 remove.c - --- src/remove.c14 May 2005 08:05:35 - 1.125 +++ src/remove.c16 May 2005 13:21:05 - @@ -755,7 +755,11 @@ remove_entry (Dirstack_state const *ds, 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) + if (is_dir == T_YES +#if HAVE_STRUCT_DIRENT_D_TYPE + || (dp DT_IS_DIR (dp)) +#endif + ) return RM_NONEMPTY_DIR; DO_UNLINK (filename, x); - -- Life is short - so eat dessert first! Eric Blake [EMAIL PROTECTED] -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.0 (Cygwin) Comment: Public key at home.comcast.net/~ericblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org iD8DBQFCiJ6m84KuGfSFAYARAoaSAJ9PwT7Nqgda/rvCCdC2BXCPMqqldACfTtL6 pkcs5bvT1FNKdT34REcWmLg= =EnkT -END PGP SIGNATURE- ___ Bug-coreutils mailing list Bug-coreutils@gnu.org http://lists.gnu.org/mailman/listinfo/bug-coreutils
Re: rm: avoiding a race condition on non-glibc systems
Paul Eggert [EMAIL PROTECTED] wrote: ... How about this patch? It incorporates the above ideas. 2005-05-13 Paul Eggert [EMAIL PROTECTED] * 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. That looks fine, and works fine here. Please commit it. I'll see if I can find someone to test the give-up-privilege code as root on Solaris 10. ___ Bug-coreutils mailing list Bug-coreutils@gnu.org http://lists.gnu.org/mailman/listinfo/bug-coreutils
Re: rm: avoiding a race condition on non-glibc systems
Jim Meyering [EMAIL PROTECTED] writes: That looks fine, and works fine here. Please commit it. OK, done. I also added Cygwin to the list of platforms that can't unlink directories, as Eric Blake suggested. ___ Bug-coreutils mailing list Bug-coreutils@gnu.org http://lists.gnu.org/mailman/listinfo/bug-coreutils
Re: rm: avoiding a race condition on non-glibc systems
Paul Eggert [EMAIL PROTECTED] wrote: Jim Meyering [EMAIL PROTECTED] writes: That looks fine, and works fine here. Please commit it. OK, done. I also added Cygwin to the list of platforms that can't unlink directories, as Eric Blake suggested. Thanks! I found that it needed a little change: 2005-05-14 Jim Meyering [EMAIL PROTECTED] * unlinkdir.m4: Register unlinkdir.c and unlinkdir.h via AC_LIBSOURCES. Add `AC_LIBOBJ([unlinkdir])'. Index: m4/unlinkdir.m4 === RCS file: /fetish/cu/m4/unlinkdir.m4,v retrieving revision 1.1 retrieving revision 1.2 diff -u -p -u -r1.1 -r1.2 --- m4/unlinkdir.m4 14 May 2005 08:01:17 - 1.1 +++ m4/unlinkdir.m4 14 May 2005 10:34:00 - 1.2 @@ -1,4 +1,4 @@ -#serial 1 +#serial 2 # Copyright (C) 2005 Free Software Foundation, Inc. # @@ -13,6 +13,9 @@ AC_DEFUN([gl_UNLINKDIR], AC_REQUIRE([AC_CANONICAL_HOST]) AC_CHECK_HEADERS_ONCE(priv.h unistd.h) + AC_LIBSOURCES([unlinkdir.c, unlinkdir.h]) + AC_LIBOBJ([unlinkdir]) + # The Hurd, the Linux kernel, the FreeBSD kernel version 2.2 and later, # and Cygwin never let anyone (even root) unlink directories. # If anyone knows of another system for which unlink can never ___ Bug-coreutils mailing list Bug-coreutils@gnu.org http://lists.gnu.org/mailman/listinfo/bug-coreutils
rm: avoiding a race condition on non-glibc systems
In reviewing parts of remove.c I noted (again) the race condition on
systems with an unlink that may remove directories, so added this comment:
+/* If anyone knows of another system for which unlink can never
+ remove a directory, please report it to [EMAIL PROTECTED]
+ The code below is slightly more efficient if it *knows* that
+ unlink(2) cannot possibly unlink a directory. */
On non-glibc systems that race condition means that there's a window
via which a privileged rm user might be tricked into unlinking a
nonempty directory.
I investigated for Solaris-10 and found that this privilege
can be tested at run-time. Here's a little code to test
whether its PRIV_SYS_LINKDIR is in the effective set:
/* This is Solaris-10-specific -- at least priv.h */
#include priv.h
#include stdlib.h
#include stdbool.h
static bool
can_unlink_directory (void)
{
#ifdef __GLIBC__
return false;
#elif defined PRIV_EFFECTIVE defined PRIV_SYS_LINKDIR
/* Solaris 10 */
priv_set_t *pset = priv_allocset ();
if (getppriv (PRIV_EFFECTIVE, pset) != 0)
return true;
bool can = priv_ismember (pset, PRIV_SYS_LINKDIR);
priv_freeset (pset);
return can;
#else
/* Be pessimistic: assume that unlink can remove a directory. */
return true;
#endif
}
int
main ()
{
exit (can_unlink_directory () ? 0 : 1);
}
That little test program works fine, but I'm not sure I want to
rearrange remove.c enough to add such a run-time test.
At best, the race condition is purely theoretical (barring a manually
suspended `rm -r') and we needn't worry. If anyone can give an idea
of how easy/hard it would be for a non-privileged user to trick a
running rm process into unlinking a directory, I'd appreciate it.
___
Bug-coreutils mailing list
Bug-coreutils@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-coreutils
Re: rm: avoiding a race condition on non-glibc systems
Jim Meyering [EMAIL PROTECTED] 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 [EMAIL PROTECTED]
* 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.m429 Apr 2005 05:37:32 - 1.108
+++ m4/prereq.m413 May 2005 22:45:10 -
@@ -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.c13 May 2005 08:42:35 - 1.122
+++ src/remove.c13 May 2005 22:45:11 -
@@ -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 [EMAIL PROTECTED]
- 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))
