New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BufferedReader.peek() crashes if closed #67984
Comments
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.
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:
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': |
New changeset 7d722c9049ff by Berker Peksag in branch '3.4': New changeset be7636fd6438 by Berker Peksag in branch 'default': |
Fixed. Thanks for the patch, John. |
Wonderful! Thanks for your help, Berker! |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: