classification
Title: BufferedReader.peek() crashes if closed
Type: crash Stage: resolved
Components: IO Versions: Python 3.5, Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: berker.peksag Nosy List: berker.peksag, haypo, jdherg, martin.panter, python-dev, serhiy.storchaka
Priority: normal Keywords: easy, patch

Created on 2015-03-28 00:29 by martin.panter, last changed 2015-05-12 14:43 by jdherg. This issue is now closed.

Files
File name Uploaded Description Edit
bug.py haypo, 2015-03-30 10:03
buffered_reader_closed_peek.patch jdherg, 2015-03-30 21:45 review
23796_fix_with_tests.patch jdherg, 2015-04-01 17:49 review
Messages (17)
msg239445 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-03-28 00:29
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.
msg239450 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-03-28 02:37
Confirmed it does indeed affect the current 3.5 (default) branch.
msg239456 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-03-28 06:38
Do you want provide a patch Martin?
msg239482 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-03-29 10:07
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.
msg239592 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2015-03-30 10:03
I confirm the bug on Python 3.5.0a3+.

Is someone interested to fix buffered_peek() to add a check on the closed state?
msg239645 - (view) Author: John Hergenroeder (jdherg) * Date: 2015-03-30 21:45
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 :)
msg239647 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2015-03-30 22:55
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)
msg239831 - (view) Author: John Hergenroeder (jdherg) * Date: 2015-04-01 17:49
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!
msg239840 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2015-04-01 19:43
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.
msg239855 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-04-01 22:19
New patch with tests looks good to me. The BufferedReaderTest class is a sensible place for the test.
msg240306 - (view) Author: John Hergenroeder (jdherg) * Date: 2015-04-09 03:28
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?
msg242076 - (view) Author: John Hergenroeder (jdherg) * Date: 2015-04-26 20:12
It looks like my contributor form has gone through -- what should my next steps here be? Thanks!
msg242742 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2015-05-08 01:18
23796_fix_with_tests.patch LGTM. I'll apply it this weekend. Thanks for the patch, John.
msg242970 - (view) Author: Roundup Robot (python-dev) Date: 2015-05-12 14:00
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
msg242971 - (view) Author: Roundup Robot (python-dev) Date: 2015-05-12 14:15
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
msg242972 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2015-05-12 14:16
Fixed. Thanks for the patch, John.
msg242975 - (view) Author: John Hergenroeder (jdherg) * Date: 2015-05-12 14:43
Wonderful! Thanks for your help, Berker!
History
Date User Action Args
2015-05-12 14:43:54jdhergsetmessages: + msg242975
2015-05-12 14:16:35berker.peksagsetstatus: open -> closed
resolution: fixed
messages: + msg242972

stage: commit review -> resolved
2015-05-12 14:15:18python-devsetmessages: + msg242971
2015-05-12 14:00:55python-devsetnosy: + python-dev
messages: + msg242970
2015-05-08 01:18:35berker.peksagsetmessages: + msg242742
stage: patch review -> commit review
2015-04-26 20:12:54jdhergsetmessages: + msg242076
2015-04-09 03:28:39jdhergsetmessages: + msg240306
2015-04-01 22:19:13martin.pantersetmessages: + msg239855
2015-04-01 19:43:32hayposetmessages: + msg239840
2015-04-01 17:49:52jdhergsetfiles: + 23796_fix_with_tests.patch

messages: + msg239831
2015-03-31 05:04:40serhiy.storchakasetassignee: serhiy.storchaka -> berker.peksag
2015-03-30 22:55:46berker.peksagsetnosy: + berker.peksag

messages: + msg239647
stage: needs patch -> patch review
2015-03-30 21:45:50jdhergsetfiles: + buffered_reader_closed_peek.patch

nosy: + jdherg
messages: + msg239645

keywords: + patch
2015-03-30 10:03:07hayposetfiles: + bug.py
nosy: + haypo
messages: + msg239592

2015-03-29 10:27:43serhiy.storchakasetkeywords: + easy
2015-03-29 10:07:54martin.pantersetmessages: + msg239482
2015-03-28 06:38:13serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg239456

assignee: serhiy.storchaka
stage: needs patch
2015-03-28 02:37:59martin.pantersetmessages: + msg239450
2015-03-28 00:29:49martin.pantercreate