gh-143375: Fix a crash in BufferedWriter.seek when passing an object with a specially crafted __index__ - #143577
gh-143375: Fix a crash in BufferedWriter.seek when passing an object with a specially crafted __index__#143577tomasr8 wants to merge 15 commits into
BufferedWriter.seek when passing an object with a specially crafted __index__#143577Conversation
| @@ -0,0 +1,2 @@ | |||
| Fix a crash in the ``seek`` method of :class:`~io.BufferedWriter` when | |||
There was a problem hiding this comment.
Is it only BufferedWriter or other buffered streams?
There was a problem hiding this comment.
It's also BufferedReader and BufferedRandom since they share the same implementation. I updated the news entry and added tests!
| // PyNumber_AsOff_t calls user code via __index__, which | ||
| // could have closed the file. | ||
| CHECK_CLOSED(self, "seek of closed file") |
There was a problem hiding this comment.
I do not think this comment is needed. It is trivial if we do all right, and we do not what to add such comments for each second line.
Remove CHECK_CLOSED above, it is redundant. Also, I think it is better to move PyNumber_AsOff_t immediately after the whence check.
There was a problem hiding this comment.
Oh, and CHECK_INITIALIZED also should be after PyNumber_AsOff_t. Add a test for concurrent detach().
There was a problem hiding this comment.
I do not think this comment is needed.
Removed
Remove CHECK_CLOSED above, it is redundant.
Also removed
Also, I think it is better to move PyNumber_AsOff_t immediately after the whence check.
Done
Add a test for concurrent detach().
Added!
| @@ -0,0 +1,2 @@ | |||
| Fix a crash in the ``seek`` method of :class:`~io.BufferedWriter` when | |||
| passing an object with a specially crafted :meth:`~object.__index__`. | |||
There was a problem hiding this comment.
No, a specially crafted __index__() is not a cause, it is only a mean of reproducing the issue. The root issue is that the buffered stream can be closed (or detached?) concurrently with seek(). This can happen in other thread (or in the garbage collector), when the GIL is released in not so special custom __index__().
There was a problem hiding this comment.
True, I updated the news entry with the root cause. Let me know if it's ok!
|
And I think TextIOWrapper has the same issue. See #143007. |
| # Used to crash before gh-143689: | ||
| self.assertEqual(bufio.read1(1), b"h") | ||
|
|
||
| def test_concurrent_close(self): |
There was a problem hiding this comment.
I'm not sure whether it's better keep the tests duplicated for each stream type or to use CommonBufferedTests (but that also runs for the Python versions, so we'd need some extra logic to skip those) or even to create CommonCBufferedTests just for the C versions?
|
This PR is stale because it has been open for 30 days with no activity. |
github-actions
Bot
removed
the
stale
3 participants
Adds a check after the call to
PyNumber_AsOff_twhich could have closed the file as a side-effect.BufferedWriter.seekduring re-entrant close #143375