ext/threads/t/free.t

I hope that I've managed to identify and fix the bug that was causing
ext/threads/t/free.t (and I think sometimes blocks.t) to sometimes fail
with a SEGV (and as it turns out sometimes just missing test output)
which was tarnishing (at least) the OS X smoke results.

There is a section of PerlIOStdio_close() that performs tricks with dup()
and dup2() to keep a file descriptor alive whilst a stdio FILE* is demolished
round it. In effect it was a race condition, and needed mutex protection,
to avoid corruption. Corruption was certainly what we had been getting -
hopefully not any more.

Hopefully having 1 Damian and 1 MJD reference in the comments keeps the
universe in balance. Oh, and 2 typos.

Nicholas Clark


Change 33502 by nicholas@mouse-mill on 2008/03/12 22:03:57

	Integrate:
	[ 33501]
	Integrate:
	[ 33491]
	Correct logic error in PerlIOStdio_close() - 0 is an acceptable value
	from dup(), so it can't also be the "don't do anything later" value.
	
	[ 33492]
	We need mutex protection in PerlIOStdio_close() for the duration of
	holding our true love file handle open, to stop anything else
	temporarily using it for a quick dup() fling, and then closing the
	file handle underneath us.
	I suspect that the lack of this protection was the cause of the threads
	free.t and blocks.t failures on OS X on 5.8.x, where usefaststdio is
	the default, and PerlIO is unable to "invalidate" the FILE *.
	
	[ 33498]
	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).

Affected files ...

... //depot/maint-5.8/perl/perlio.c#118 integrate

Differences ...

==== //depot/maint-5.8/perl/perlio.c#118 (text) ====

@@ -3114,7 +3114,7 @@
 	int invalidate = 0;
 	IV result = 0;
 	int saveerr = 0;
-	int dupfd = 0;
+	int dupfd = -1;
 #ifdef SOCKS5_VERSION_NAME
     	/* Socks lib overrides close() but stdio isn't linked to
 	   that library (though we are) - so we must call close()
@@ -3140,8 +3140,37 @@
 	    result = PerlIO_flush(f);
 	    saveerr = errno;
 	    invalidate = PerlIOStdio_invalidate_fileno(aTHX_ stdio);
-	    if (!invalidate)
+	    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) {
+		    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.  */
+		}
+#endif
+	    }
 	}
         result = PerlSIO_fclose(stdio);
 	/* We treat error from stdio as success if we invalidated
@@ -3155,9 +3184,12 @@
 	/* in SOCKS' case, let close() determine return value */
 	result = close(fd);
 #endif
-	if (dupfd) {
+	if (dupfd >= 0) {
 	    PerlLIO_dup2(dupfd,fd);
 	    PerlLIO_close(dupfd);
+#ifdef USE_ITHREADS
+	    MUTEX_UNLOCK(&PL_perlio_mutex);
+#endif
 	}
 	return result;
     }

• ext/threads/t/free.t by Nicholas Clark