From 32cfedeb1c378d6acf8e39308831f826f614a204 Mon Sep 17 00:00:00 2001 From: Antoine Pitrou Date: Wed, 11 Aug 2010 13:31:33 +0000 Subject: Issue #9550: a BufferedReader could issue an additional read when the original read request had been satisfied, which can block indefinitely when the underlying raw IO channel is e.g. a socket. Report and original patch by Jason V. Miller. --- Lib/test/test_io.py | 24 ++++++++++++++++++++++++ Misc/ACKS | 1 + Misc/NEWS | 5 +++++ Modules/_io/bufferedio.c | 5 ++++- 4 files changed, 34 insertions(+), 1 deletion(-) diff --git a/Lib/test/test_io.py b/Lib/test/test_io.py index aa1679b..c584b92 100644 --- a/Lib/test/test_io.py +++ b/Lib/test/test_io.py @@ -52,12 +52,14 @@ class MockRawIO: self._read_stack = list(read_stack) self._write_stack = [] self._reads = 0 + self._extraneous_reads = 0 def read(self, n=None): self._reads += 1 try: return self._read_stack.pop(0) except: + self._extraneous_reads += 1 return b"" def write(self, b): @@ -88,6 +90,7 @@ class MockRawIO: try: data = self._read_stack[0] except IndexError: + self._extraneous_reads += 1 return 0 if data is None: del self._read_stack[0] @@ -824,6 +827,27 @@ class BufferedReaderTest(unittest.TestCase, CommonBufferedTests): self.assertRaises(IOError, bufio.seek, 0) self.assertRaises(IOError, bufio.tell) + def test_no_extraneous_read(self): + # Issue #9550; when the raw IO object has satisfied the read request, + # we should not issue any additional reads, otherwise it may block + # (e.g. socket). + bufsize = 16 + for n in (2, bufsize - 1, bufsize, bufsize + 1, bufsize * 2): + rawio = self.MockRawIO([b"x" * n]) + bufio = self.tp(rawio, bufsize) + self.assertEqual(bufio.read(n), b"x" * n) + # Simple case: one raw read is enough to satisfy the request. + self.assertEqual(rawio._extraneous_reads, 0, + "failed for {}: {} != 0".format(n, rawio._extraneous_reads)) + # A more complex case where two raw reads are needed to satisfy + # the request. + rawio = self.MockRawIO([b"x" * (n - 1), b"x"]) + bufio = self.tp(rawio, bufsize) + self.assertEqual(bufio.read(n), b"x" * n) + self.assertEqual(rawio._extraneous_reads, 0, + "failed for {}: {} != 0".format(n, rawio._extraneous_reads)) + + class CBufferedReaderTest(BufferedReaderTest): tp = io.BufferedReader diff --git a/Misc/ACKS b/Misc/ACKS index 2650147..26feba3 100644 --- a/Misc/ACKS +++ b/Misc/ACKS @@ -548,6 +548,7 @@ Trent Mick Aristotelis Mikropoulos Damien Miller Chad Miller +Jason V. Miller Jay T. Miller Roman Milner Andrii V. Mishkovskyi diff --git a/Misc/NEWS b/Misc/NEWS index b89f89d..80d07f5 100644 --- a/Misc/NEWS +++ b/Misc/NEWS @@ -70,6 +70,11 @@ Extensions Library ------- +- Issue #9550: a BufferedReader could issue an additional read when the + original read request had been satisfied, which could block indefinitely + when the underlying raw IO channel was e.g. a socket. Report and original + patch by Jason V. Miller. + - Issue #3757: thread-local objects now support cyclic garbage collection. Thread-local objects involved in reference cycles will be deallocated timely by the cyclic GC, even if the underlying thread is still running. diff --git a/Modules/_io/bufferedio.c b/Modules/_io/bufferedio.c index 1dd1ec4..4678da9 100644 --- a/Modules/_io/bufferedio.c +++ b/Modules/_io/bufferedio.c @@ -1383,7 +1383,10 @@ _bufferedreader_read_generic(buffered *self, Py_ssize_t n) self->pos = 0; self->raw_pos = 0; self->read_end = 0; - while (self->read_end < self->buffer_size) { + /* NOTE: when the read is satisfied, we avoid issuing any additional + reads, which could block indefinitely (e.g. on a socket). + See issue #9550. */ + while (remaining > 0 && self->read_end < self->buffer_size) { Py_ssize_t r = _bufferedreader_fill_buffer(self); if (r == -1) goto error; -- cgit v0.12