BufferedReader.peek() crashes if closed

[vadmium]

BPO	23796
Nosy	@vstinner, @berkerpeksag, @vadmium, @serhiy-storchaka
Files	bug.py buffered_reader_closed_peek.patch 23796_fix_with_tests.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/berkerpeksag'
closed_at = <Date 2015-05-12.14:16:35.573>
created_at = <Date 2015-03-28.00:29:49.990>
labels = ['easy', 'expert-IO', 'type-crash']
title = 'BufferedReader.peek() crashes if closed'
updated_at = <Date 2015-05-12.14:43:54.990>
user = 'https://github.com/vadmium'

bugs.python.org fields:

activity = <Date 2015-05-12.14:43:54.990>
actor = 'jdherg'
assignee = 'berker.peksag'
closed = True
closed_date = <Date 2015-05-12.14:16:35.573>
closer = 'berker.peksag'
components = ['IO']
creation = <Date 2015-03-28.00:29:49.990>
creator = 'martin.panter'
dependencies = []
files = ['38735', '38746', '38787']
hgrepos = []
issue_num = 23796
keywords = ['patch', 'easy']
message_count = 17.0
messages = ['239445', '239450', '239456', '239482', '239592', '239645', '239647', '239831', '239840', '239855', '240306', '242076', '242742', '242970', '242971', '242972', '242975']
nosy_count = 6.0
nosy_names = ['vstinner', 'python-dev', 'berker.peksag', 'martin.panter', 'serhiy.storchaka', 'jdherg']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue23796'
versions = ['Python 3.4', 'Python 3.5']

[vadmium]

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.

[vadmium]

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

[serhiy-storchaka]

Do you want provide a patch Martin?

[vadmium]

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.

[vstinner]

I confirm the bug on Python 3.5.0a3+.

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

[jdherg]

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 :)

[berkerpeksag]

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)

[jdherg]

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!

[vstinner]

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.