Change 33498 by [EMAIL PROTECTED] on 2008/03/12 19:09:21
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/perl/perlio.c#384 edit
Differences ...
==== //depot/perl/perlio.c#384 (text) ====
Index: perl/perlio.c
--- perl/perlio.c#383~33492~ 2008-03-12 05:23:55.000000000 -0700
+++ perl/perlio.c 2008-03-12 12:09:21.000000000 -0700
@@ -3157,19 +3157,30 @@
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 @@
#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;
}
End of Patch.
