Issue 23796: BufferedReader.peek() crashes if closed

If the BufferedReader object has already been closed, calling peek() can cause a strange error message or a crash:

$ python3 -bWall
Python 3.4.2 (default, Oct  8 2014, 14:33:30) 
[GCC 4.9.1 20140903 (prerelease)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from io import *
>>> b = BufferedReader(BytesIO())
>>> b.close()
>>> b.peek()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: PyMemoryView_FromBuffer(): info->buf must not be NULL
>>> b = BufferedReader(BytesIO(b"12"))
>>> b.read(1)
b'1'
>>> b.close()
>>> b.peek()
Segmentation fault (core dumped)
[Exit 139]

I’m not able to check with 3.5 at the moment, but looking at the code, I suspect this also applies to 3.5 and there needs to be a CHECK_CLOSED() call added somewhere around Modules/_io/bufferedio.c:884.

Confirmed it does indeed affect the current 3.5 (default) branch.

Do you want provide a patch Martin?

Happy to when I get a chance. It should be easy to do, though it is not high on my priority list because I don’t think the scenario is very important in real world usage. I only encountered it when testing edge cases in another patch.

I confirm the bug on Python 3.5.0a3+.

Is someone interested to fix buffered_peek() to add a check on the closed state?

If no-one else wants it, I'd love to tackle this as my first Python (and OSS in general) contribution. Attached is a one-line patch that just does a CHECK_CLOSED call in buffered_peek and is modeled on the pattern in the buffered_flush function just above. I'm under the impression that the final patch will need to include a test that confirms the patch worked, but I wanted to claim the bug and start the feedback process early :)

Thanks for the patch, John.

> I'm under the impression that the final patch will need to include a test that confirms the patch worked,

Correct. You could convert the reproducers in msg239445 to a test case.

The patch looks good to me. I think you'll also need to add a similar check to buffered_read1():

>>> from io import *
>>> b = BufferedReader(BytesIO(b"12"))
>>> b.read(1)
b'1'
>>> b.close()
>>> b.peek()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: peek of closed file
>>> b.read1(1)
Segmentation fault (core dumped)

Thanks for the feedback, Berker!

I've added a test case that closes a buffered reader and then attempts to both peek and read1 it. I tacked it onto the end of the BufferedReaderTest class in test_io, but let me know if there's a better place to put it.

I've also added the check to read1 -- good catch. I opted to add it before the check that the argument to read1 is 0 -- my assumption is that a 0-length read on a closed BufferReader should fail, not return an empty bytestring.

Thanks again for the feedback!

John Hergenroeder added the comment:
> my assumption is that a 0-length read on a closed BufferReader should fail, not return an empty bytestring.

I agree.

New patch with tests looks good to me. The BufferedReaderTest class is a sensible place for the test.

Thanks!

I submitted my contributor agreement form last week -- is there anything I can do to improve this patch while I wait for that to process?

It looks like my contributor form has gone through -- what should my next steps here be? Thanks!

23796_fix_with_tests.patch LGTM. I'll apply it this weekend. Thanks for the patch, John.

New changeset 41e9d324f10d by Berker Peksag in branch 'default':
Issue #23796: peak and read1 methods of BufferedReader now raise ValueError
https://hg.python.org/cpython/rev/41e9d324f10d

New changeset 7d722c9049ff by Berker Peksag in branch '3.4':
Issue #23796: peak and read1 methods of BufferedReader now raise ValueError
https://hg.python.org/cpython/rev/7d722c9049ff

New changeset be7636fd6438 by Berker Peksag in branch 'default':
Issue #23796: Null merge.
https://hg.python.org/cpython/rev/be7636fd6438

Fixed. Thanks for the patch, John.

Wonderful! Thanks for your help, Berker!

stage: commit review -> resolved

messages: + msg239831

messages: + msg239647
stage: needs patch -> patch review

nosy: + jdherg
messages: + msg239645

keywords: + patch

assignee: serhiy.storchaka
stage: needs patch