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