gh-143689: Fix BufferedReader.read1 leaving object in reentrant state on error #143690
Conversation
BufferedReader.read1() could leave the buffered object in a reentrant (locked) state when an exception was raised while allocating the output buffer. This change ensures the internal buffered lock is always released on error, keeping the object in a consistent state after failures. Signed-off-by: Yongtao Huang <yongtaoh2022@gmail.com>
cmaloney
left a comment
cmaloney
left a comment
There was a problem hiding this comment.
Thanks for the patch! The fix looks correct. The NEWs entry and test don't quite match the preferred patterns in CPython, left some comments.
Lib/test/test_io/test_bufferedio.py
Outdated
| def test_read1_error_does_not_cause_reentrant_failure(self): | ||
| # 32-bit builds (e.g. win32) can raise OverflowError | ||
| # converting huge Python int to Py_ssize_t. | ||
| if sys.maxsize <= 2**32: |
There was a problem hiding this comment.
This would be better to do with a skip decorator decorator. Example to match:
Line 378 in d1282ef
There was a problem hiding this comment.
Thanks, that helps.
Lib/test/test_io/test_bufferedio.py
Outdated
| # Under TSan, the process may abort on huge allocation | ||
| # attempts (exit code 66). | ||
| if support.check_sanitizer(thread=True): | ||
| self.skipTest("TSan aborts on huge allocations") |
There was a problem hiding this comment.
This one can also be a decorator, something like:
@unittest.skipIf(check_sanitizer(thread=True),
'ThreadSanitizer aborts on huge allocations (exit code 66).')
Misc/NEWS.d/next/Library/2026-01-11-14-14-19.gh-issue-143689.fzHJ2W.rst
Outdated
Show resolved
Hide resolved
…zHJ2W.rst Co-authored-by: Cody Maloney <cmaloney@users.noreply.github.com>
Thanks for the review and feedback — I’ve made the requested changes. |
| @unittest.skipUnless(sys.maxsize > 2**32, 'requires 64bit platform') | ||
| @unittest.skipIf(check_sanitizer(thread=True), | ||
| 'ThreadSanitizer aborts on huge allocations (exit code 66).') | ||
| def test_read1_error_does_not_cause_reentrant_failure(self): |
There was a problem hiding this comment.
Does it need @bigmemtest decorator?
There was a problem hiding this comment.
Thank you. I don’t think @bigmemtest is needed here, since the goal is only to trigger the “request too large → failure” error path, not to actually allocate a large amount of memory.
| self.assertIsInstance(cm.exception.__cause__, TypeError) | ||
|
|
||
| @unittest.skipUnless(sys.maxsize > 2**32, 'requires 64bit platform') | ||
| @unittest.skipIf(check_sanitizer(thread=True), |
There was a problem hiding this comment.
Only the non-free-threaded TSan build failed; the UBSan and free-threaded TSan builds are both OK.
Please refer to https://github.com/python/cpython/actions/runs/20896527108/job/60035968975?pr=143690
Lib/test/test_io/test_bufferedio.py
Outdated
|
|
||
| with self.open(os_helper.TESTFN, "rb", buffering=0) as raw: | ||
| bufio = self.tp(raw, buffer_size=8) | ||
| huge = 10**18 |
There was a problem hiding this comment.
I would love to see a comment, why this number is used :)
What are the rules for it?
Co-authored-by: sobolevn <mail@sobolevn.me>
3 participants
BufferedReader.read1() could leave the buffered object in a
reentrant (locked) state when an exception was raised while
allocating the output buffer.
This change ensures the internal buffered lock is always released
on error, keeping the object in a consistent state after failures.