diff options
| -rw-r--r-- | Lib/_pyio.py | 19 | ||||
| -rw-r--r-- | Lib/test/test_io.py | 31 | ||||
| -rw-r--r-- | Misc/NEWS.d/next/Library/2017-11-09-21-36-32.bpo-31976.EOA7qY.rst | 2 | ||||
| -rw-r--r-- | Modules/_io/bufferedio.c | 14 |
4 files changed, 58 insertions, 8 deletions
diff --git a/Lib/_pyio.py b/Lib/_pyio.py index 6833883dad..adf5d0ecbf 100644 --- a/Lib/_pyio.py +++ b/Lib/_pyio.py @@ -1188,11 +1188,11 @@ class BufferedWriter(_BufferedIOMixin): return self.raw.writable() def write(self, b): - if self.closed: - raise ValueError("write to closed file") if isinstance(b, str): raise TypeError("can't write str to binary stream") with self._write_lock: + if self.closed: + raise ValueError("write to closed file") # XXX we can implement some more tricks to try and avoid # partial writes if len(self._write_buf) > self.buffer_size: @@ -1253,6 +1253,21 @@ class BufferedWriter(_BufferedIOMixin): self._flush_unlocked() return _BufferedIOMixin.seek(self, pos, whence) + def close(self): + with self._write_lock: + if self.raw is None or self.closed: + return + # We have to release the lock and call self.flush() (which will + # probably just re-take the lock) in case flush has been overridden in + # a subclass or the user set self.flush to something. This is the same + # behavior as the C implementation. + try: + # may raise BlockingIOError or BrokenPipeError etc + self.flush() + finally: + with self._write_lock: + self.raw.close() + class BufferedRWPair(BufferedIOBase): diff --git a/Lib/test/test_io.py b/Lib/test/test_io.py index 3158729a7f..2ac2e17a03 100644 --- a/Lib/test/test_io.py +++ b/Lib/test/test_io.py @@ -168,6 +168,22 @@ class PyMisbehavedRawIO(MisbehavedRawIO, pyio.RawIOBase): pass +class SlowFlushRawIO(MockRawIO): + def __init__(self): + super().__init__() + self.in_flush = threading.Event() + + def flush(self): + self.in_flush.set() + time.sleep(0.25) + +class CSlowFlushRawIO(SlowFlushRawIO, io.RawIOBase): + pass + +class PySlowFlushRawIO(SlowFlushRawIO, pyio.RawIOBase): + pass + + class CloseFailureIO(MockRawIO): closed = 0 @@ -1726,6 +1742,18 @@ class BufferedWriterTest(unittest.TestCase, CommonBufferedTests): self.assertRaises(OSError, b.close) # exception not swallowed self.assertTrue(b.closed) + def test_slow_close_from_thread(self): + # Issue #31976 + rawio = self.SlowFlushRawIO() + bufio = self.tp(rawio, 8) + t = threading.Thread(target=bufio.close) + t.start() + rawio.in_flush.wait() + self.assertRaises(ValueError, bufio.write, b'spam') + self.assertTrue(bufio.closed) + t.join() + + class CBufferedWriterTest(BufferedWriterTest, SizeofTest): tp = io.BufferedWriter @@ -4085,7 +4113,8 @@ def load_tests(*args): # Put the namespaces of the IO module we are testing and some useful mock # classes in the __dict__ of each test. mocks = (MockRawIO, MisbehavedRawIO, MockFileIO, CloseFailureIO, - MockNonBlockWriterIO, MockUnseekableIO, MockRawIOWithoutRead) + MockNonBlockWriterIO, MockUnseekableIO, MockRawIOWithoutRead, + SlowFlushRawIO) all_members = io.__all__ + ["IncrementalNewlineDecoder"] c_io_ns = {name : getattr(io, name) for name in all_members} py_io_ns = {name : getattr(pyio, name) for name in all_members} diff --git a/Misc/NEWS.d/next/Library/2017-11-09-21-36-32.bpo-31976.EOA7qY.rst b/Misc/NEWS.d/next/Library/2017-11-09-21-36-32.bpo-31976.EOA7qY.rst new file mode 100644 index 0000000000..1dd54e35b3 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2017-11-09-21-36-32.bpo-31976.EOA7qY.rst @@ -0,0 +1,2 @@ +Fix race condition when flushing a file is slow, which can cause a +segfault if closing the file from another thread. diff --git a/Modules/_io/bufferedio.c b/Modules/_io/bufferedio.c index edc4ba5a53..1ae7a70bbd 100644 --- a/Modules/_io/bufferedio.c +++ b/Modules/_io/bufferedio.c @@ -347,9 +347,10 @@ _enter_buffered_busy(buffered *self) } #define IS_CLOSED(self) \ + (!self->buffer || \ (self->fast_closed_checks \ ? _PyFileIO_closed(self->raw) \ - : buffered_closed(self)) + : buffered_closed(self))) #define CHECK_CLOSED(self, error_msg) \ if (IS_CLOSED(self)) { \ @@ -1971,14 +1972,17 @@ _io_BufferedWriter_write_impl(buffered *self, Py_buffer *buffer) Py_off_t offset; CHECK_INITIALIZED(self) - if (IS_CLOSED(self)) { - PyErr_SetString(PyExc_ValueError, "write to closed file"); - return NULL; - } if (!ENTER_BUFFERED(self)) return NULL; + /* Issue #31976: Check for closed file after acquiring the lock. Another + thread could be holding the lock while closing the file. */ + if (IS_CLOSED(self)) { + PyErr_SetString(PyExc_ValueError, "write to closed file"); + goto error; + } + /* Fast path: the data to write can be fully buffered. */ if (!VALID_READ_BUFFER(self) && !VALID_WRITE_BUFFER(self)) { self->pos = 0; |