diff -r 20bd2bdc0a2a Include/fileobject.h --- a/Include/fileobject.h Thu Mar 27 14:34:59 2008 +0100 +++ b/Include/fileobject.h Sat Mar 29 01:29:57 2008 +0100 @@ -9,10 +9,10 @@ extern "C" { typedef struct { PyObject_HEAD - FILE *f_fp; + FILE *volatile f_fp; PyObject *f_name; PyObject *f_mode; - int (*f_close)(FILE *); + int (*volatile f_close)(FILE *); int f_softspace; /* Flag used by 'print' command */ int f_binary; /* Flag which indicates whether the file is open in binary (1) or text (0) mode */ diff -r 20bd2bdc0a2a Lib/test/test_file.py --- a/Lib/test/test_file.py Thu Mar 27 14:34:59 2008 +0100 +++ b/Lib/test/test_file.py Sat Mar 29 01:29:57 2008 +0100 @@ -1,6 +1,9 @@ import sys import sys import os import unittest +import tempfile +import time +import threading from array import array from weakref import proxy @@ -339,11 +342,91 @@ class FileSubclassTests(unittest.TestCas self.failUnless(f.subclass_closed) +class FileThreadingTests(unittest.TestCase): + # These tests check the ability to call various methods of file objects + # (including close()) concurrently without crashing the Python interpreter. + # See #815646, #595601 + + def setUp(self): + self.to_remove = set() + self.f = None + self.filename = tempfile.mktemp() + + def tearDown(self): + for filename in self.to_remove: + try: + os.remove(filename) + except (IOError, OSError): + pass + if self.f: + try: + self.f.close() + except (IOError, OSError, ValueError): + pass + + def _create_file(self): + filename = tempfile.mktemp() + self.f = open(filename, "w") + self.to_remove.add(filename) + + def _run_workers(self, func, nb_instances=2, duration=0.5): + self.do_continue = True + threads = [] + try: + for i in range(nb_instances): + t = threading.Thread(target=func) + t.start() + threads.append(t) + time.sleep(duration) + finally: + self.do_continue = False + for t in threads: + t.join(1.0) + + def _test_close_open_io(self, io_func, nb_workers=20): + def worker(): + while self.do_continue: + try: + self.f.close() + self._create_file() + io_func() + except (IOError, OSError, ValueError): + pass + self._create_file() + self._run_workers(worker, nb_workers) + + def test_close_open(self): + def io_func(): + pass + self._test_close_open_io(io_func) + + def test_close_open_read(self): + def io_func(): + self.f.read(0) + self._test_close_open_io(io_func) + + def test_close_open_seek(self): + def io_func(): + self.f.seek(0, 0) + self._test_close_open_io(io_func) + + def test_close_open_tell(self): + def io_func(): + self.f.tell() + self._test_close_open_io(io_func) + + def test_close_open_write(self): + def io_func(): + self.f.write('') + self._test_close_open_io(io_func) + + def test_main(): # Historically, these tests have been sloppy about removing TESTFN. # So get rid of it no matter what. try: - run_unittest(AutoFileTests, OtherFileTests, FileSubclassTests) + run_unittest(AutoFileTests, OtherFileTests, FileSubclassTests, + FileThreadingTests) finally: if os.path.exists(TESTFN): os.unlink(TESTFN) diff -r 20bd2bdc0a2a Objects/fileobject.c --- a/Objects/fileobject.c Thu Mar 27 14:34:59 2008 +0100 +++ b/Objects/fileobject.c Sat Mar 29 01:29:57 2008 +0100 @@ -48,9 +48,41 @@ #define NEWLINE_LF 2 /* \n newline seen */ #define NEWLINE_CRLF 4 /* \r\n newline seen */ +/* + These macros allow fetching the FILE pointer to a local variable before + releasing the GIL, so that it isn't modified in our back. + Allow, they avoid calling fclose() on that same pointer when it is being + used elsewhere. close() raises an IOError if that condition is detected. + See #815646, #595601. +*/ + +#define GET_FILE_BEGIN_ALLOW_THREADS(fobj) \ +{ \ + FILE *volatile local_fp = fobj->f_fp; \ + if (local_fp) {\ + int (*volatile _save_fclose)(FILE *) = fobj->f_close; \ + fobj->f_close = _save_fclose ? dummy_fclose : _save_fclose; \ + Py_BEGIN_ALLOW_THREADS + +#define GET_FILE_END_ALLOW_THREADS(fobj) \ + Py_END_ALLOW_THREADS \ + if (_save_fclose && _save_fclose != dummy_fclose) \ + fobj->f_close = _save_fclose;\ + }\ +} + + #ifdef __cplusplus extern "C" { #endif + +/* This function should never be called, it is used as a placeholder + to notify close_the_file() that the file isn't ready to be closed. */ +static int +dummy_fclose(FILE *fp) +{ + return EOF; +} FILE * PyFile_AsFile(PyObject *f) @@ -271,6 +303,39 @@ cleanup: return (PyObject *)f; } +static PyObject * +close_the_file(PyFileObject *f) +{ + int sts = 0; + int (*volatile local_close)(FILE *); + FILE *volatile local_fp = f->f_fp; + if (local_fp != NULL) { + /* NULL out the FILE pointer before releasing the GIL, because it + will not be valid anymore after the close() function is called. */ + f->f_fp = NULL; + local_close = f->f_close; + f->f_close = NULL; + if (local_close == dummy_fclose) { + PyErr_SetString(PyExc_IOError, + "close() called during concurrent operation on the same file " + "object."); + return NULL; + } + if (local_close != NULL) { + Py_BEGIN_ALLOW_THREADS + errno = 0; + sts = (*local_close)(local_fp); + Py_END_ALLOW_THREADS + if (sts == EOF) + return PyErr_SetFromErrno(PyExc_IOError); + if (sts != 0) + return PyInt_FromLong((long)sts); + } + } + Py_INCREF(Py_None); + return Py_None; +} + PyObject * PyFile_FromFile(FILE *fp, char *name, char *mode, int (*close)(FILE *)) { @@ -386,15 +451,16 @@ static void static void file_dealloc(PyFileObject *f) { - int sts = 0; + PyObject *ret; if (f->weakreflist != NULL) PyObject_ClearWeakRefs((PyObject *) f); - if (f->f_fp != NULL && f->f_close != NULL) { - Py_BEGIN_ALLOW_THREADS - sts = (*f->f_close)(f->f_fp); - Py_END_ALLOW_THREADS - if (sts == EOF) - PySys_WriteStderr("close failed: [Errno %d] %s\n", errno, strerror(errno)); + ret = close_the_file(f); + if (!ret) { + PySys_WriteStderr("close failed in file object destructor:\n"); + PyErr_Print(); + } + else { + Py_DECREF(ret); } PyMem_Free(f->f_setbuf); Py_XDECREF(f->f_name); @@ -432,24 +498,10 @@ static PyObject * static PyObject * file_close(PyFileObject *f) { - int sts = 0; - if (f->f_fp != NULL) { - if (f->f_close != NULL) { - Py_BEGIN_ALLOW_THREADS - errno = 0; - sts = (*f->f_close)(f->f_fp); - Py_END_ALLOW_THREADS - } - f->f_fp = NULL; - } + PyObject *sts = close_the_file(f); PyMem_Free(f->f_setbuf); f->f_setbuf = NULL; - if (sts == EOF) - return PyErr_SetFromErrno(PyExc_IOError); - if (sts != 0) - return PyInt_FromLong((long)sts); - Py_INCREF(Py_None); - return Py_None; + return sts; } @@ -534,7 +586,7 @@ file_seek(PyFileObject *f, PyObject *arg file_seek(PyFileObject *f, PyObject *args) { int whence; - int ret; + int ret = -1; Py_off_t offset; PyObject *offobj, *off_index; @@ -566,10 +618,12 @@ file_seek(PyFileObject *f, PyObject *arg if (PyErr_Occurred()) return NULL; - Py_BEGIN_ALLOW_THREADS + GET_FILE_BEGIN_ALLOW_THREADS(f) errno = 0; - ret = _portable_fseek(f->f_fp, offset, whence); - Py_END_ALLOW_THREADS + ret = _portable_fseek(local_fp, offset, whence); + GET_FILE_END_ALLOW_THREADS(f) + if (!f->f_fp) + return err_closed(); if (ret != 0) { PyErr_SetFromErrno(PyExc_IOError); @@ -696,14 +750,16 @@ static PyObject * static PyObject * file_tell(PyFileObject *f) { - Py_off_t pos; + Py_off_t pos = -1; if (f->f_fp == NULL) return err_closed(); - Py_BEGIN_ALLOW_THREADS + GET_FILE_BEGIN_ALLOW_THREADS(f) errno = 0; - pos = _portable_ftell(f->f_fp); - Py_END_ALLOW_THREADS + pos = _portable_ftell(local_fp); + GET_FILE_END_ALLOW_THREADS(f) + if (!f->f_fp) + return err_closed(); if (pos == -1) { PyErr_SetFromErrno(PyExc_IOError); clearerr(f->f_fp); @@ -861,11 +917,16 @@ file_read(PyFileObject *f, PyObject *arg return NULL; bytesread = 0; for (;;) { - Py_BEGIN_ALLOW_THREADS + chunksize = 0; + GET_FILE_BEGIN_ALLOW_THREADS(f) errno = 0; chunksize = Py_UniversalNewlineFread(BUF(v) + bytesread, - buffersize - bytesread, f->f_fp, (PyObject *)f); - Py_END_ALLOW_THREADS + buffersize - bytesread, local_fp, (PyObject *)f); + GET_FILE_END_ALLOW_THREADS(f) + if (!f->f_fp) { + Py_DECREF(v); + return err_closed(); + } if (chunksize == 0) { if (!ferror(f->f_fp)) break; @@ -917,11 +978,14 @@ file_readinto(PyFileObject *f, PyObject return NULL; ndone = 0; while (ntodo > 0) { - Py_BEGIN_ALLOW_THREADS + nnow = 0; + GET_FILE_BEGIN_ALLOW_THREADS(f) errno = 0; - nnow = Py_UniversalNewlineFread(ptr+ndone, ntodo, f->f_fp, + nnow = Py_UniversalNewlineFread(ptr+ndone, ntodo, local_fp, (PyObject *)f); - Py_END_ALLOW_THREADS + GET_FILE_END_ALLOW_THREADS(f) + if (!f->f_fp) + return err_closed(); if (nnow == 0) { if (!ferror(f->f_fp)) break; @@ -1381,7 +1445,7 @@ file_readlines(PyFileObject *f, PyObject file_readlines(PyFileObject *f, PyObject *args) { long sizehint = 0; - PyObject *list; + PyObject *list = NULL; PyObject *line; char small_buffer[SMALLCHUNK]; char *buffer = small_buffer; @@ -1406,14 +1470,17 @@ file_readlines(PyFileObject *f, PyObject if ((list = PyList_New(0)) == NULL) return NULL; for (;;) { - if (shortread) - nread = 0; - else { - Py_BEGIN_ALLOW_THREADS + nread = 0; + if (!shortread) { + GET_FILE_BEGIN_ALLOW_THREADS(f) errno = 0; nread = Py_UniversalNewlineFread(buffer+nfilled, - buffersize-nfilled, f->f_fp, (PyObject *)f); - Py_END_ALLOW_THREADS + buffersize-nfilled, local_fp, (PyObject *)f); + GET_FILE_END_ALLOW_THREADS(f) + if (!f->f_fp) { + err_closed(); + goto error; + } shortread = (nread < buffersize-nfilled); } if (nread == 0) { @@ -1422,10 +1489,6 @@ file_readlines(PyFileObject *f, PyObject break; PyErr_SetFromErrno(PyExc_IOError); clearerr(f->f_fp); - error: - Py_DECREF(list); - list = NULL; - goto cleanup; } totalread += nread; p = (char *)memchr(buffer+nfilled, '\n', nread); @@ -1499,25 +1562,32 @@ file_readlines(PyFileObject *f, PyObject if (err != 0) goto error; } - cleanup: + +cleanup: Py_XDECREF(big_buffer); return list; + +error: + Py_CLEAR(list); + goto cleanup; } static PyObject * file_write(PyFileObject *f, PyObject *args) { char *s; - Py_ssize_t n, n2; + Py_ssize_t n, n2 = -1; if (f->f_fp == NULL) return err_closed(); if (!PyArg_ParseTuple(args, f->f_binary ? "s#" : "t#", &s, &n)) return NULL; f->f_softspace = 0; - Py_BEGIN_ALLOW_THREADS + GET_FILE_BEGIN_ALLOW_THREADS(f) errno = 0; - n2 = fwrite(s, 1, n, f->f_fp); - Py_END_ALLOW_THREADS + n2 = fwrite(s, 1, n, local_fp); + GET_FILE_END_ALLOW_THREADS(f) + if (!f->f_fp) + return err_closed(); if (n2 != n) { PyErr_SetFromErrno(PyExc_IOError); clearerr(f->f_fp); @@ -1883,7 +1953,7 @@ static int static int readahead(PyFileObject *f, int bufsize) { - Py_ssize_t chunksize; + Py_ssize_t chunksize = 0; if (f->f_buf != NULL) { if( (f->f_bufend - f->f_bufptr) >= 1) @@ -1895,11 +1965,15 @@ readahead(PyFileObject *f, int bufsize) PyErr_NoMemory(); return -1; } - Py_BEGIN_ALLOW_THREADS + GET_FILE_BEGIN_ALLOW_THREADS(f) errno = 0; chunksize = Py_UniversalNewlineFread( - f->f_buf, bufsize, f->f_fp, (PyObject *)f); - Py_END_ALLOW_THREADS + f->f_buf, bufsize, local_fp, (PyObject *)f); + GET_FILE_END_ALLOW_THREADS(f) + if (!f->f_fp) { + err_closed(); + return -1; + } if (chunksize == 0) { if (ferror(f->f_fp)) { PyErr_SetFromErrno(PyExc_IOError);