We recently found an assertion failure in the universal
newline support when running a multithreaded program
where two threads used the same Python file object. 
The assert(stream != NULL) test in 
Py_UniversalNewlineFread() fails once in a blue moon,
where stream is the stdio FILE * that the fileobject
wraps.  Further analysis suggests that there is a race
condition between checking FILE * and using FILE * that
exists in at least Python 2.1 and up.

I'll actually describe the problem as it exists in
Python 2.2, because it is simpler to avoid the
universal newline code.  That code isn't the source of
the problem, although it's assert() uncovers it in a
clear way.

In file_read() (rev 2.141.6.5), the first thing it does
is check if f_fp (the FILE *) is NULL.  If so it raises
an IOError -- operation on closed file object.  Later,
file_read() enters a for loop that calls fread() until
enough bytes have been read.

	for (;;) {
		Py_BEGIN_ALLOW_THREADS
		errno = 0;
		chunksize = fread(BUF(v) + bytesread, 1,
				  buffersize - bytesread, f->f_fp);
		Py_END_ALLOW_THREADS
		if (chunksize == 0) {
			if (!ferror(f->f_fp))
				break;
			PyErr_SetFromErrno(PyExc_IOError);
			clearerr(f->f_fp);
			Py_DECREF(v);
			return NULL;
		}

The problem is that fread() is called after the global
interpreter lock is released.  Since the lock is
released, another Python thread could run and modify
the file object, changing the value of f->f_fp.  Under
the current interpreter lock scheme, it isn't safe to
use f->f_fp without holding the interpreter lock.

The current file_read() code can fail in a variety of
ways.  It's possible for a second thread to close the
file, which will set f->f_fp to NULL.  Who knows what
fread() will do when NULL is passed.

The universal newline code is squirrels the FILE * in a
local variable, which is worse.  If it happens that
another thread closes the file, at best the local
points to a closed FILE *.  But that memory could get
recycled and then there's no way to know what it points to.

socket I/O has a similar problem with unsafe sharing of
the file descriptor.  However, this problem seems less
severe in general, because we'd just be passing a bogus
file descriptor to a system call.  We don't have to
worry about whether stdio will dump core when passed a
bogus pointer.  There is a chance the a socket will be
closed and its file descriptor used for a different
socket.  So a call to recv() with one socket ends up
using a different socket.  That will be a nightmare to
debug, but it won't cause a segfault.  (And, in
general, files and sockets shouldn't be shared between
application threads unless the application is going to
make sure its safe.)

The solution to this problem is to use a
per-file-object lock to guard access to f->f_fp.  No
thread should read or right f->f_fp without holding the
lock.  To make sure that other threads get a chance to
run when there is contention for the file, the
file-object lock should never be held when the GIL is held.

Logged In: YES 
user_id=45365

I'm not sure your solution of locking in the file object is feasible. Every call of PyFile_AsFile() is dangerous, if the  code proceeds to release the GIL before using the FILE* it's in danger. And this code could be in extension modules too (I know there's quite a few in my own img package).

When I did the universal newlines patch I basically ignored the problem (thinking that no-one in their right mind would close a file in one thread while it's still in use in another: how wrong can one be:-).

The only real solution may be to get rid of PyFile_AsFile altogether, and replace it with PyFile_AsFileLocked() and PyFile_Unlock() and make these calls somehow work even when the GIL is held (possibly by temporarily releasing the GIL while acquiring the file lock).

Logged In: YES 
user_id=31392

I think I'm less worried about PyFile_AsFile() than I am
about PyFile_WriteObject().  The latter is invoked by the
print statement.  It extracts the FILE* using
PyFile_AsFile() and proceeds to pass it to PyObject_Print().
 PyObject_Print() MUST hold the GIL, but since it has the
raw FILE* it MUST hold the file lock.  But you can't hold
both locks :-(.

Logged In: YES 
user_id=31435

To cheer you up, it's probably worse than that <wink>.  
While you're holding a file lock, in theory you dare not 
execute any code that may call back into Python, because 
the called code may try to do an operation on the file 
you've got locked, and then the whole system freezes. That 
is, independent of the GIL, it's not thoroughly safe to call 
PyObject_Print with a file lock held, because an object's 
__str__ may try to "do something" with the Python-level file 
object too.  The more depressing news is that a reentrant 
file lock would solve that one.

Logged In: YES 
user_id=45365

Uhm... Isn't it good enough if you don't hold the GIL while you acquire the file lock? When I looked at it I thought it would be (but then, I didn't look very hard:-).

And there may be another way around this problem too. If you're really only worried about closing (Python at the moment couldn't care less that when two threads write to the same file at the same time the output may be intermingled or other fun things may happen) then don't lock the file, but simply set a flag "keep your hands off the FILE*, I'm using it!" and in that case delay the close. You may want to protect the flag and the delayed close with a lock, I guess.

Or you could define this as a programmer error and raise an exception if one thread tries to close a file another thread is using. (The application will know much better how to handle the contention with locking or some other mechanism).

Logged In: YES 
user_id=12800

Jack, your last paragraph sounds like a good idea <waiting
for Tim to shoot holes in it>.  Isn't the whole point to
avoid core dumps, not to "make it work right" whatever that
means?  We should take the simplest approach to avoiding
crashes.

Logged In: YES 
user_id=6380

Wow. This is harder than I thought! :-(

We should be able to solve whatever problem there is with
'print'. The PyFile_AsFile() API may have to become
obsolete, *or* we may have to add a separate call to access
the lock. I'll have to think about it more.

Logged In: YES 
user_id=31392

Here's a checkpoint of current progress.  The patch applies
cleanly and even compiles.  It works most of the time, but
it causes a bunch of test failures.  I haven't had time to
debug the errors, two likely errors are incorrect
propagation of errors from across the release lock boundary.
 (The error checking goes on inside so that clearerr() can
be called while the file lock is held but
PyErr_SetFromErrno() can be called while the GIL is held.) 
The other source of errors is typos -- like calling LOCK
when you mean UNLOCK.

Logged In: YES 
user_id=7212

I'd like to add that this particular problem cost me about a
week of trying to figure out what the heck was going on, a
stack trace thrown from Python is MUCH better than
intermittent last-chance exceptions thrown from our binaries
in the field.  :)

http://aegisknight.livejournal.com/128191.html

The race condition issue should get more attention.

Given how long this has been sitting without action, it's ludicrous to
keep it at "high" priority.  I'm also taking it away from Jermy since
his interest must be minimal by now.  And I'm removing 3.0 since it has
a completely different I/O implementation (with much more severe
multi-threading issues, but nothing in common with the 2.x series).

I've posted a preliminary patch for the close()-should-raise-an-error
approach here: #815646

Fix committed to trunk in revision 62195 via Issue 815646