GNU bug report logs - #9813
rm -rf calls rmdir() prior to close(), which can fail

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 9813 in the body.
You can then email your comments to 9813 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Eric Blake <eblake <at> redhat.com>
To: bug-coreutils <bug-coreutils <at> gnu.org>, bug-gnulib <bug-gnulib <at> gnu.org>
Subject: rm -rf calls rmdir() prior to close(), which can fail
Date: Thu, 20 Oct 2011 11:38:36 -0600

POSIX is clear that attempts to rmdir() a directory that still has open 
descriptors may fail.  Of course, on Linux, this (rather limiting) 
restriction is not present, so we don't notice it; but on Cygwin, there 
are certain file systems where this is a real problem, such as in this 
thread:
http://cygwin.com/ml/cygwin/2011-10/msg00365.html

Looking at an strace on Linux reveals the problem (abbreviated to show 
highlights here):

$ mkdir -p a/b
$ strace rm -f a
...
openat(AT_FDCWD, "a", 
O_RDONLY|O_NOCTTY|O_NONBLOCK|O_DIRECTORY|O_NOFOLLOW) = 3
...
fcntl(3, F_DUPFD, 3)                    = 4
...
close(3)                                = 0
...
openat(4, "b", O_RDONLY|O_NOCTTY|O_NONBLOCK|O_DIRECTORY|O_NOFOLLOW) = 3
...
fcntl(3, F_DUPFD, 3)                    = 5
...
close(3)                                = 0
close(5)                                = 0
unlinkat(4, "b", AT_REMOVEDIR)          = 0
unlinkat(AT_FDCWD, "a", AT_REMOVEDIR)   = 0
close(4)                                = 0

Notice that for subdirectories, we opened the directory, then used dup 
to have a handle for use in further *at calls, then do 
fdopendir/readdir/closedir on the DIR*, then close the duplicate fd, all 
before calling unlinkat (aka rmdir) on that subdirectory.  But for the 
top-level directory, the dup'd fd (4) is still open when we attempt the 
unlinkat.

I'm still trying to investigate whether the fix needs to be in gnulib or 
just coreutils, but something needs to be done to swap the order so that 
the last handle to the directory is closed prior to the rmdir attempt.

-- 
Eric Blake   eblake <at> redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

Message #8 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Eric Blake <eblake <at> redhat.com>
Cc: bug-gnulib <bug-gnulib <at> gnu.org>, bug-coreutils <bug-coreutils <at> gnu.org>
Subject: Re: rm -rf calls rmdir() prior to close(), which can fail
Date: Thu, 20 Oct 2011 12:47:28 -0700

On 10/20/11 10:38, Eric Blake wrote:
> POSIX is clear that attempts to rmdir() a directory that still has open descriptors may fail.

Hmm, that's news to me.  And on the contrary, the spec
<http://pubs.opengroup.org/onlinepubs/9699919799/functions/rmdir.html>
explicitly talks about what rmdir() does when there are open descriptors:

 "If one or more processes have the directory open when the last link is removed,
  the dot and dot-dot entries, if present, shall be removed before rmdir() returns
  and no new entries may be created in the directory, but the directory shall not
  be removed until all references to the directory are closed."

which very much sounds like rmdir() is supposed to succeed in this case.

Also, there's no entry for this situation under the "may fail" section
of ERRORS.  And there's longstanding Unix tradition that you can unlink
a file that you have an open file descriptor to, which suggests that
rmdir() should do likewise.

So, if this is a problem under Cygwin, it's probably better to handle it
in the rmdir() wrapper that deals with Cygwin and file descriptors.

Message #11 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Eric Blake <eblake <at> redhat.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: bug-gnulib <bug-gnulib <at> gnu.org>, bug-coreutils <bug-coreutils <at> gnu.org>
Subject: Re: rm -rf calls rmdir() prior to close(), which can fail
Date: Thu, 20 Oct 2011 13:57:20 -0600

On 10/20/2011 01:47 PM, Paul Eggert wrote:
> On 10/20/11 10:38, Eric Blake wrote:
>> POSIX is clear that attempts to rmdir() a directory that still has open descriptors may fail.
>
> Hmm, that's news to me.  And on the contrary, the spec
> <http://pubs.opengroup.org/onlinepubs/9699919799/functions/rmdir.html>
> explicitly talks about what rmdir() does when there are open descriptors:
>
>   "If one or more processes have the directory open when the last link is removed,
>    the dot and dot-dot entries, if present, shall be removed before rmdir() returns
>    and no new entries may be created in the directory, but the directory shall not
>    be removed until all references to the directory are closed."
>
> which very much sounds like rmdir() is supposed to succeed in this case.
>
> Also, there's no entry for this situation under the "may fail" section
> of ERRORS.  And there's longstanding Unix tradition that you can unlink
> a file that you have an open file descriptor to, which suggests that
> rmdir() should do likewise.

That's because it's a shall fail, not a may fail error:

[EBUSY]
The directory to be removed is currently in use by the system or some 
process and the implementation considers this to be an error.

>
> So, if this is a problem under Cygwin, it's probably better to handle it
> in the rmdir() wrapper that deals with Cygwin and file descriptors.

It's more than just cygwin.  And while cygwin _is_ working around this 
in many cases (cygwin is going to some rather extreme lengths for NTFS 
and NFS, for example), it only works on a per-filesystem basis (the 
latest bug is that a Novell device driver, exposing the NWFS file 
system, has bugs in its mapping to Windows system calls that are 
preventing cygwin's normal workarounds from working).  But even when 
cygwin can work around it, it is expensive (it involves reopening the 
handle multiple times, with varying level of permission requests, to see 
if the file is previously opened in sharing mode, and depending on that 
result, temporarily moving the file to the recycle bin so that it will 
disappear when the last handle closes); whereas fixing coreutils to do 
things in the correct order in the first place would make the overall rm 
process faster because it isn't wasting time on corner case file 
shuffling for a directory that is being deleted in the first place.

At any rate, this is a regression introduced by coreutils 8.0, when rm 
switched to fts().  Prior to that point, coreutils used the correct 
ordering, where rmdir() was not attempted until after the close(); and 
the Cygwin report demonstrated that coreutils 7.0 worked on NWFS where 
coreutils 8.x fails.

-- 
Eric Blake   eblake <at> redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

Message #14 received at 9813 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Eric Blake <eblake <at> redhat.com>
Cc: bug-gnulib <at> gnu.org, 9813 <at> debbugs.gnu.org
Subject: Re: bug#9813: rm -rf calls rmdir() prior to close(), which can fail
Date: Thu, 20 Oct 2011 16:46:05 -0700

On 10/20/11 12:57, Eric Blake wrote:
> That's because it's a shall fail, not a may fail error:
> 
> [EBUSY]
> The directory to be removed is currently in use by the system
> or some process and the implementation considers this to be an error.

But "in use by" does not mean "accessed by an open
file descriptor owned by".  It means that the directory
is mounted, or is the working directory of a process,
or is the root directory.  The interpretation of "in use by"
to mean "tied down by a file descriptor"
flies in the face of the plain meaning of the earlier part
of the text, which talks about what happens when one invokes
rmdir() on a directory that has an open file descriptor.

If we allow the phrase "in use by" to mean whatever the
operating system wants it to mean, then an operating system
where rmdir() always fails with errno==EBUSY would conform
to POSIX, because the O.S. could always say that the directory
is "in use by" the rmdir() call itself.  That's not what was
intended here.

> It's more than just cygwin.

So far, I've seen only Cygwin mentioned.
Where does it happen in a typical GNUish environment?

This isn't just a coreutils issue: I expect that it'll occur
many programs that do the equivalent of "rm -fr".

Message #17 received at 9813 <at> debbugs.gnu.org (full text, mbox):

From: Eric Blake <eblake <at> redhat.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: bug-gnulib <at> gnu.org, 9813 <at> debbugs.gnu.org
Subject: Re: bug#9813: rm -rf calls rmdir() prior to close(), which can fail
Date: Thu, 20 Oct 2011 18:06:03 -0600

On 10/20/2011 05:46 PM, Paul Eggert wrote:
>> It's more than just cygwin.
>
> So far, I've seen only Cygwin mentioned.
> Where does it happen in a typical GNUish environment?

Try the same exercise using NFSv2 or NFSv3 mounts (NFSv4 is getting 
closer to POSIX compliance, but I don't know if it will handle this any 
better).  I suspect that it would be possible to find a testcase under 
Linux and Solaris clients using a less-than-stellar remote NFS server 
that reproduces this issue, at least on any setup where you would also 
see a failure in unlink()ing a regular file with open fds (rmdir() a 
directory with open handles is conceptually no different than unlink()).

>
> This isn't just a coreutils issue: I expect that it'll occur
> many programs that do the equivalent of "rm -fr".

Many programs that do the equivalent of "rm -fr" are using more naive 
algorithms (like coreutils 7.0 rm did) that do not involve fts() and 
unlinkat(), and thus do not hit the problem in the first place, because 
they aren't leaving the directory fd open during the rmdir().  But you 
are right that an strace of 'find a -delete' shows that find suffers 
from the same issue, while 'oldfind a -delete' is immune; again a 
problem where the difference is the use of gnulib's fts().

Maybe we need to ask for clarification from the Austin Group on how much 
of Window's default behavior should affect POSIX compliance (that 
behavior being that a directory is busy if any process has it as a 
current working directory or if any fd is open on the directory).

-- 
Eric Blake   eblake <at> redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

Message #20 received at 9813 <at> debbugs.gnu.org (full text, mbox):

From: Eric Blake <eblake <at> redhat.com>
Cc: Paul Eggert <eggert <at> cs.ucla.edu>, bug-gnulib <at> gnu.org, 9813 <at> debbugs.gnu.org
Subject: Re: bug#9813: rm -rf calls rmdir() prior to close(), which can fail
Date: Thu, 20 Oct 2011 18:11:27 -0600

On 10/20/2011 06:06 PM, Eric Blake wrote:
> Try the same exercise using NFSv2 or NFSv3 mounts (NFSv4 is getting
> closer to POSIX compliance, but I don't know if it will handle this any
> better). I suspect that it would be possible to find a testcase under
> Linux and Solaris clients using a less-than-stellar remote NFS server
> that reproduces this issue, at least on any setup where you would also
> see a failure in unlink()ing a regular file with open fds (rmdir() a
> directory with open handles is conceptually no different than unlink()).

Also, notice that the POSIX requirement on unlink() is that ETXTBSY is a 
may fail (a file is currently being executed by some other process), but 
that EBUSY is a shall fail, with the same wording as for rmdir(), namely:

[EBUSY]
The file named by the path argument cannot be unlinked because it is 
being used by the system or another process and the implementation 
considers this an error.

This "shall fail" EBUSY is _how_ NFS justifies their behavior of failing 
to unlink files with open fds, even though I agree with you that it 
flies in the face of traditional Unix semantics, and explains why many 
GNU programs have already made efforts to ensure that all fds to a file 
are closed before attempting unlink/rmdir.

-- 
Eric Blake   eblake <at> redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

Message #23 received at 9813 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Eric Blake <eblake <at> redhat.com>
Cc: bug-gnulib <at> gnu.org, 9813 <at> debbugs.gnu.org
Subject: Re: bug#9813: rm -rf calls rmdir() prior to close(), which can fail
Date: Thu, 20 Oct 2011 22:30:52 -0700

On 10/20/11 17:06, Eric Blake wrote:
>> So far, I've seen only Cygwin mentioned.
>> Where does it happen in a typical GNUish environment?
> 
> Try the same exercise using NFSv2 or NFSv3 mounts

I don't see why NFSv3 or v3 would be different.
I just tried your test case with an NFSv3 mount
(RHEL 5.7 client, NetApp server) and it did not
exhibit the problem.

> I suspect that it would be possible to find a testcase under Linux

Possibly, but I'd like to see it before worrying about this.

> at least on any setup where you would also see a failure in unlink()ing a regular file with open fds

Yes, if regular files don't conform to POSIX, then maybe directories don't
either.  But we generally don't have workarounds for those regular-file
issues in gnulib, and we shouldn't really: the glitches are relatively
rare in practice and people who are used to NFS know about the glitches
and deal with them manually as they come up.  Surely directories should be
treated similarly.

Message #28 received at 9813-done <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Eric Blake <eblake <at> redhat.com>
Cc: cygwin <at> cygwin.com, bug-gnulib <bug-gnulib <at> gnu.org>,
	9813-done <at> debbugs.gnu.org
Subject: Re: rm -rf calls rmdir() prior to close(), which can fail
Date: Sun, 23 Oct 2011 22:53:34 +0200

Eric Blake wrote:
> POSIX is clear that attempts to rmdir() a directory that still has
> open descriptors may fail.  Of course, on Linux, this (rather
> limiting) restriction is not present, so we don't notice it; but on
> Cygwin, there are certain file systems where this is a real problem,
> such as in this thread:
> http://cygwin.com/ml/cygwin/2011-10/msg00365.html
>
> Looking at an strace on Linux reveals the problem (abbreviated to show
> highlights here):
>
> $ mkdir -p a/b
> $ strace rm -f a
> ...
> openat(AT_FDCWD, "a", O_RDONLY|O_NOCTTY|O_NONBLOCK|O_DIRECTORY|O_NOFOLLOW) = 3
> ...
> fcntl(3, F_DUPFD, 3)                    = 4
> ...
> close(3)                                = 0
> ...
> openat(4, "b", O_RDONLY|O_NOCTTY|O_NONBLOCK|O_DIRECTORY|O_NOFOLLOW) = 3
> ...
> fcntl(3, F_DUPFD, 3)                    = 5
> ...
> close(3)                                = 0
> close(5)                                = 0
> unlinkat(4, "b", AT_REMOVEDIR)          = 0
> unlinkat(AT_FDCWD, "a", AT_REMOVEDIR)   = 0
> close(4)                                = 0
>
> Notice that for subdirectories, we opened the directory, then used dup
> to have a handle for use in further *at calls, then do
> fdopendir/readdir/closedir on the DIR*, then close the duplicate fd,
> all before calling unlinkat (aka rmdir) on that subdirectory.  But for
> the top-level directory, the dup'd fd (4) is still open when we
> attempt the unlinkat.

Thanks for the analysis, Eric.
That was due to a rather subtle but easy/safe-to-fix bug.

While the rm from coreutils-8.14 worked as your strace above shows, the
fixed one does this: (note how the close(4) now precedes the removal of "a")

  $ mkdir -p a/b
  $ strace -e openat,close,unlinkat ./rm -rf a
  close(3)                                = 0
  close(3)                                = 0
  openat(AT_FDCWD, "a", O_RDONLY|O_NOCTTY|O_NONBLOCK|O_DIRECTORY|O_NOFOLLOW) = 3
  close(3)                                = 0
  openat(4, "b", O_RDONLY|O_NOCTTY|O_NONBLOCK|O_DIRECTORY|O_NOFOLLOW) = 3
  close(3)                                = 0
  close(5)                                = 0
  unlinkat(4, "b", AT_REMOVEDIR)          = 0
  close(4)                                = 0
  unlinkat(AT_FDCWD, "a", AT_REMOVEDIR)   = 0
  close(0)                                = 0
  close(1)                                = 0
  close(2)                                = 0

Here is the patch that I expect to push tomorrow:

From a11c49cd72a91c05a272e36ff5d3cd92675cbfb5 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering <at> redhat.com>
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 "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.
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..3a2d2cc 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,28 @@
+2011-10-23  Jim Meyering  <meyering <at> redhat.com>
+
+	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.
+	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 <at> gnu.org>
 	    Bruno Haible  <bruno <at> clisp.org>
 	    Jim Meyering  <jim <at> meyering.net>
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

Message #29 received at 9813-done <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Eric Blake <eblake <at> redhat.com>
Cc: cygwin <at> cygwin.com, bug-gnulib <bug-gnulib <at> gnu.org>,
	9813-done <at> debbugs.gnu.org
Subject: Re: rm -rf calls rmdir() prior to close(), which can fail
Date: Mon, 24 Oct 2011 10:14:26 +0200

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 <meyering <at> redhat.com>
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  <meyering <at> 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 <at> gnu.org>
 	    Bruno Haible  <bruno <at> clisp.org>
 	    Jim Meyering  <jim <at> meyering.net>
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

Message #32 received at 9813 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Eric Blake <eblake <at> redhat.com>
Cc: cygwin <at> cygwin.com, bug-gnulib <bug-gnulib <at> gnu.org>, 9813 <at> debbugs.gnu.org
Subject: Re: rm -rf calls rmdir() prior to close(), which can fail
Date: Mon, 24 Oct 2011 10:58:05 +0200

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 <meyering <at> redhat.com>
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

Message #35 received at 9813 <at> debbugs.gnu.org (full text, mbox):

From: Eric Blake <eblake <at> redhat.com>
To: Jim Meyering <jim <at> meyering.net>
Cc: cygwin <at> cygwin.com, bug-gnulib <bug-gnulib <at> gnu.org>, 9813 <at> debbugs.gnu.org
Subject: Re: rm -rf calls rmdir() prior to close(), which can fail
Date: Mon, 24 Oct 2011 07:57:57 -0600

On 10/24/2011 02:58 AM, Jim Meyering wrote:
>   ** 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]

rm didn't use fts() until coreutils 8.0 (the cygwin testing proved that 
coreutils 7.0 did not suffer from the problem).  See also the news for 
8.13 mentioning an rm regression introduced by fts() in 8.0.

-- 
Eric Blake   eblake <at> redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

Message #38 received at 9813 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Eric Blake <eblake <at> redhat.com>
Cc: 9813 <at> debbugs.gnu.org
Subject: Re: rm -rf calls rmdir() prior to close(), which can fail
Date: Mon, 24 Oct 2011 16:21:00 +0200

Eric Blake wrote:
> On 10/24/2011 02:58 AM, Jim Meyering wrote:
>>   ** 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]
>
> rm didn't use fts() until coreutils 8.0 (the cygwin testing proved
> that coreutils 7.0 did not suffer from the problem).  See also the
> news for 8.13 mentioning an rm regression introduced by fts() in 8.0.

Thanks.

From 5bb6316bd71f3a52990a57d94203d8855e4b6b90 Mon Sep 17 00:00:00 2001
From: Eric Blake <eblake <at> redhat.com>
Date: Mon, 24 Oct 2011 16:20:34 +0200
Subject: [PATCH] doc: NEWS: correct "bug introduced in ..." version number

* NEWS: s/7.0/8.0/
---
 NEWS |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/NEWS b/NEWS
index b73057a..081989d 100644
--- a/NEWS
+++ b/NEWS
@@ -6,7 +6,7 @@ GNU coreutils NEWS                                    -*- outline -*-

   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]
+  [bug introduced in coreutils-8.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]
--
1.7.7.419.g87009

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.