Subject: Re: rm -rf calls rmdir() prior to close(), which can fail
From: Jim Meyering <[email-masked]>
To: Eric Blake <[email-masked]>
Cc: [email-masked], bug-gnulib <[email-masked]>, [email-masked]
Date: Mon, 24 Oct 2011 10:58:05 +0200
List-Id: <cygwin.cygwin.com>

Jim Meyering wrote:
>> Here is the patch that I expect to push tomorrow:
...
> I've fixed/improved the ChangeLog/commit-log:
>
> Subject: [PATCH] fts: close parent dir FD before returning from
>  post-traversal fts_read
>
> The problem: the fts-using "mkdir -p A/B; rm -rf A" would attempt to
> unlink A, even though an FD open on A remained.  This is suboptimal
> (holding a file descriptor open longer than needed), but otherwise not
> a problem on Unix-like kernels.  However, on Cygwin with certain Novell
> file systems, (see http://cygwin.com/ml/cygwin/2011-10/msg00365.html),
> that represents a real problem: it causes the removal of A to fail
> with e.g., "rm: cannot remove `A': Device or resource busy"
>
> fts visits each directory twice and keeps a cache (fts_fd_ring) of
> directory file descriptors.  After completing the final, FTS_DP,
> visit of a directory, RESTORE_INITIAL_CWD intended to clear the FD
> cache, but then proceeded to add a new FD to it via the subsequent
> FCHDIR (which calls cwd_advance_fd and i_ring_push).  Before, the
> final file descriptor would be closed only via fts_close's call to
> fd_ring_clear.  Now, it is usually closed earlier, via the final
> FTS_DP-returning fts_read call.
> * lib/fts.c (restore_initial_cwd): New function, converted from
> the macro.  Call fd_ring_clear *after* FCHDIR, not before it.
> Update callers.
> Reported by Franz Sirl via the above URL, with analysis by Eric Blake
> in http://thread.gmane.org/gmane.comp.lib.gnulib.bugs/28739

I pushed that, along with the following in coreutils.
The gnulib update induced a new (coreutils-specific) syntax-check failure:

    src/system.h:# define ENODATA (-1)
    make[3]: *** [sc_prohibit_always-defined_macros] Error 1

because gnulib now defines that symbol, so I have also removed
that definition from coreutils:

From f8ae6440eb8f943fd1f040d039753851824512d3 Mon Sep 17 00:00:00 2001
From: Jim Meyering <[email-masked]>
Date: Mon, 24 Oct 2011 10:27:22 +0200
Subject: [PATCH] rm: update gnulib to get an fts fix for Cygwin+NWFS/NcFsd
 file systems

* NEWS (Bug fixes): Mention it.
As far as we know, this fix affects only Cygwin with NWFS or NcFsd
file systems.  See these:
http://git.sv.gnu.org/cgit/gnulib.git/commit/?id=71f13422f3e634
http://thread.gmane.org/gmane.comp.lib.gnulib.bugs/28739
http://cygwin.com/ml/cygwin/2011-10/msg00365.html
* src/system.h (ENODATA): Remove fall-back definition, now that
gnulib provides one.  Caught by the sc_prohibit_always-defined_macros
syntax-check rule.
Also remove now-irrelevant "Don't use bcopy..." comment.
---
 NEWS         |    4 ++++
 gnulib       |    2 +-
 src/system.h |   11 -----------
 3 files changed, 5 insertions(+), 12 deletions(-)

diff --git a/NEWS b/NEWS
index 4d210b5..b73057a 100644
--- a/NEWS
+++ b/NEWS
@@ -4,6 +4,10 @@ GNU coreutils NEWS                                    -*- outline -*-

 ** Bug fixes

+  rm -rf DIR would fail with "Device or resource busy" on Cygwin with NWFS
+  and NcFsd file systems.  This did not affect Unix/Linux-based kernels.
+  [bug introduced in coreutils-7.0, when rm began using fts]
+
   tac no longer fails to handle two or more non-seekable inputs
   [bug introduced in coreutils-5.3.0]

diff --git a/gnulib b/gnulib
index 6a4c64c..71f1342 160000
--- a/gnulib
+++ b/gnulib
@@ -1 +1 @@
-Subproject commit 6a4c64ce4a59bd9589e63fb5ee480765d356f8c7
+Subproject commit 71f13422f3e6345933513607255f1f7a7526e937
diff --git a/src/system.h b/src/system.h
index 18ac0cc..19421a9 100644
--- a/src/system.h
+++ b/src/system.h
@@ -74,19 +74,8 @@ you must include <sys/types.h> before including this file
 # define makedev(maj, min)  mkdev (maj, min)
 #endif

-/* Don't use bcopy!  Use memmove if source and destination may overlap,
-   memcpy otherwise.  */
-
 #include <string.h>
-
 #include <errno.h>
-
-/* Some systems don't define this; POSIX mentions it but says it is
-   obsolete, so gnulib does not provide it either.  */
-#ifndef ENODATA
-# define ENODATA (-1)
-#endif
-
 #include <stdbool.h>
 #include <stdlib.h>
 #include "version.h"
--
1.7.7.419.g87009

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple