develooper Front page | perl.perl5.porters | Postings from March 2008


Nicholas Clark
March 12, 2008 16:04
Message ID:
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

	[ 33501]
	[ 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
	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;
     	/* 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) {
+		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) {
+		    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.  */
+		}
+	    }
         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);
-	if (dupfd) {
+	if (dupfd >= 0) {
+	    MUTEX_UNLOCK(&PL_perlio_mutex);
 	return result;
     } Perl Programming lists via nntp and http.
Comments to Ask Bjørn Hansen at | Group listing | About