This is a live mirror of the Perl 5 development currently hosted at
Change 33492 did not spread the protection wide enough. There were
authorNicholas Clark <>
Wed, 12 Mar 2008 19:09:21 +0000 (19:09 +0000)
committerNicholas Clark <>
Wed, 12 Mar 2008 19:09:21 +0000 (19:09 +0000)
still two more races to be lost.
1: The close() could still happen after the (premature) mutex release
   allowed another thread to dup() to that file descriptor.
2: The initial dup() could happen whilst another thread was in the
   mutex protected region, and had temporarily closed the file
Race conditions remain with any other thread that actually does I/O
during the execution of the mutex protected region (as noted in a
comment), and dup() failure is not handled gracefully (also noted).

p4raw-id: //depot/perl@33498


index a0f96cc..ed829e4 100644 (file)
--- a/perlio.c
+++ b/perlio.c
@@ -3157,19 +3157,30 @@ PerlIOStdio_close(pTHX_ PerlIO *f)
            saveerr = errno;
            invalidate = PerlIOStdio_invalidate_fileno(aTHX_ stdio);
            if (!invalidate) {
+               MUTEX_LOCK(&PL_perlio_mutex);
+               /* Right. We need a mutex here because for a brief while we will
+                  have the situation that fd is actually closed. Hence if a
+                  second thread were to get into this block, its dup() would
+                  likely return our fd as its dupfd. (after all, it is closed).
+                  Then if we get to the dup2() first, we blat the fd back
+                  (messing up its temporary as a side effect) only for it to
+                  then close its dupfd (== our fd) in its close(dupfd) */
+               /* There is, of course, a race condition, that any other thread
+                  trying to input/output/whatever on this fd will be stuffed
+                  for the duraction of this little manoeuver. Perhaps we should
+                  hold an IO mutex for the duration of every IO operation if
+                  we know that invalidate doesn't work on this platform, but
+                  that would suck, and could kill performance.
+                  Except that correctness trumps speed.
+                  Advice from klortho #11912. */
                dupfd = PerlLIO_dup(fd);
-               if (dupfd >= 0) {
-                   /* Right. We need a mutex here because for a brief while we
-                      will have the situation that fd is actually closed.
-                      Hence if a second thread were to get into this block,
-                      its dup() would likely return our fd as its dupfd.
-                      (after all, it is closed). Then if we get to the dup2()
-                      first, we blat the fd back (messing up its temporary as
-                      a side effect) only for it to then close its dupfd
-                      (== our fd) in its close(dupfd) */
-                   MUTEX_LOCK(&PL_perlio_mutex);
-               } else {
+               if (dupfd < 0) {
+                   MUTEX_UNLOCK(&PL_perlio_mutex);
                    /* Oh cXap. This isn't going to go well. Not sure if we can
                       recover from here, or if closing this particular FILE *
                       is a good idea now.  */
@@ -3191,10 +3202,10 @@ PerlIOStdio_close(pTHX_ PerlIO *f)
        if (dupfd >= 0) {
+           PerlLIO_close(dupfd);
-       MUTEX_UNLOCK(&PL_perlio_mutex);
+           MUTEX_UNLOCK(&PL_perlio_mutex);
-           PerlLIO_close(dupfd);
        return result;