Change 33492 did not spread the protection wide enough. There were

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
   descriptor.
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

1 files changed, 24 insertions, 13 deletions

diff --git a/perlio.c b/perlio.c
index a0f96ccfb7..ed829e4608 100644
--- 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) {
+#ifdef USE_ITHREADS
+		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. */
+#endif
 		dupfd = PerlLIO_dup(fd);
 #ifdef USE_ITHREADS
-		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)
 #endif
 	if (dupfd >= 0) {
 	    PerlLIO_dup2(dupfd,fd);
+	    PerlLIO_close(dupfd);
 #ifdef USE_ITHREADS
-	MUTEX_UNLOCK(&PL_perlio_mutex);
+	    MUTEX_UNLOCK(&PL_perlio_mutex);
 #endif
-	    PerlLIO_close(dupfd);
 	}
 	return result;
     }