[Python-checkins] bpo-31976: Fix race condition when flushing a file is slow. (#4331)

[Antoine Pitrou]

https://github.com/python/cpython/commit/9703f092abc0259926d88c7855afeae4a78afc7d
commit: 9703f092abc0259926d88c7855afeae4a78afc7d
branch: master
author: benfogle <benfogle at gmail.com>
committer: Antoine Pitrou <pitrou at free.fr>
date: 2017-11-10T22:03:40+01:00
summary:

bpo-31976: Fix race condition when flushing a file is slow. (#4331)

files:
A Misc/NEWS.d/next/Library/2017-11-09-21-36-32.bpo-31976.EOA7qY.rst
M Lib/_pyio.py
M Lib/test/test_io.py
M Modules/_io/bufferedio.c

diff --git a/Lib/_pyio.py b/Lib/_pyio.py
index 6833883dadb..adf5d0ecbf6 100644
--- a/Lib/_pyio.py
+++ b/Lib/_pyio.py
@@ -1188,11 +1188,11 @@ def writable(self):
         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 @@ def seek(self, pos, whence=0):
             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 3158729a7fa..2ac2e17a03e 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 @@ def bad_write(b):
         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 00000000000..1dd54e35b37
--- /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 edc4ba5a537..1ae7a70bbda 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;