Re: rm -rf calls rmdir() prior to close(), which can fail

[Jim Meyering]

Jim Meyering wrote:
...
> Here is the patch that I expect to push tomorrow:
>
> Subject: [PATCH] fts: close parent dir FD before returning from
>  post-traversal fts_read
>
> The problem: the fts-using "rm -rf A/B/" would attempt to unlink A,
> while a file descriptor open on A remained.  This is suboptimal
> (holding a file descriptor open longer than needed) on Linux, but
> otherwise not a problem.  However, on Cygwin with certain file system
> types, (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.

I've fixed/improved the ChangeLog/commit-log:

>From 71f13422f3e6345933513607255f1f7a7526e937 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Sun, 23 Oct 2011 22:42:25 +0200
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
---
 ChangeLog |   25 +++++++++++++++++++++++++
 lib/fts.c |   23 +++++++++++++++--------
 2 files changed, 40 insertions(+), 8 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 93ee45e..a4ac818 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,28 @@
+2011-10-23  Jim Meyering  <address@hidden>
+
+       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
+
 2011-10-23  Gary V. Vaughan  <address@hidden>
            Bruno Haible  <address@hidden>
            Jim Meyering  <address@hidden>
diff --git a/lib/fts.c b/lib/fts.c
index e3829f3..f61a91e 100644
--- a/lib/fts.c
+++ b/lib/fts.c
@@ -229,11 +229,6 @@ static int      fts_safe_changedir (FTS *, FTSENT *, int, 
const char *)
 #define ISSET(opt)      (sp->fts_options & (opt))
 #define SET(opt)        (sp->fts_options |= (opt))

-/* FIXME: make this a function */
-#define RESTORE_INITIAL_CWD(sp)                 \
-  (fd_ring_clear (&((sp)->fts_fd_ring)),        \
-   FCHDIR ((sp), (ISSET (FTS_CWDFD) ? AT_FDCWD : (sp)->fts_rfd)))
-
 /* FIXME: FTS_NOCHDIR is now misnamed.
    Call it FTS_USE_FULL_RELATIVE_FILE_NAMES instead. */
 #define FCHDIR(sp, fd)                                  \
@@ -349,6 +344,18 @@ cwd_advance_fd (FTS *sp, int fd, bool chdir_down_one)
   sp->fts_cwd_fd = fd;
 }

+/* Restore the initial, pre-traversal, "working directory".
+   In FTS_CWDFD mode, we merely call cwd_advance_fd, otherwise,
+   we may actually change the working directory.
+   Return 0 upon success. Upon failure, set errno and return nonzero.  */
+static int
+restore_initial_cwd (FTS *sp)
+{
+  int fail = FCHDIR (sp, ISSET (FTS_CWDFD) ? AT_FDCWD : sp->fts_rfd);
+  fd_ring_clear (&(sp->fts_fd_ring));
+  return fail;
+}
+
 /* Open the directory DIR if possible, and return a file
    descriptor.  Return -1 and set errno on failure.  It doesn't matter
    whether the file descriptor has read or write access.  */
@@ -948,7 +955,7 @@ next:   tmp = p;
                  * root.
                  */
                 if (p->fts_level == FTS_ROOTLEVEL) {
-                        if (RESTORE_INITIAL_CWD(sp)) {
+                        if (restore_initial_cwd(sp)) {
                                 SET(FTS_STOP);
                                 return (NULL);
                         }
@@ -1055,7 +1062,7 @@ cd_dot_dot:
          * one level, via "..".
          */
         if (p->fts_level == FTS_ROOTLEVEL) {
-                if (RESTORE_INITIAL_CWD(sp)) {
+                if (restore_initial_cwd(sp)) {
                         p->fts_errno = errno;
                         SET(FTS_STOP);
                 }
@@ -1579,7 +1586,7 @@ mem1:                           saved_errno = errno;
          */
         if (!continue_readdir && descend && (type == BCHILD || !nitems) &&
             (cur->fts_level == FTS_ROOTLEVEL
-             ? RESTORE_INITIAL_CWD(sp)
+             ? restore_initial_cwd(sp)
              : fts_safe_changedir(sp, cur->fts_parent, -1, ".."))) {
                 cur->fts_info = FTS_ERR;
                 SET(FTS_STOP);
--
1.7.7.419.g87009