]> Savannah Git Hosting - gnulib.git/commitdiff
fts: close parent dir FD before returning from post-traversal fts_read
authorJim Meyering <meyering@redhat.com>
Sun, 23 Oct 2011 20:42:25 +0000 (22:42 +0200)
committerJim Meyering <meyering@redhat.com>
Mon, 24 Oct 2011 08:09:50 +0000 (10:09 +0200)
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
lib/fts.c

index 93ee45e6fad423a3ccd9ddcafb7fc284be30dda2..a4ac818eb4483e93ef009853c946c4adbcdbe722 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,28 @@
+2011-10-23  Jim Meyering  <meyering@redhat.com>
+
+       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  <gary@gnu.org>
            Bruno Haible  <bruno@clisp.org>
            Jim Meyering  <jim@meyering.net>
index e3829f324845b39c3494db27db8d0fbd12f5aacf..f61a91e6f0c888f84d6831e83f19fdee518f8ea6 100644 (file)
--- 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);