Repository navigation
gh-143375: Fix a crash in BufferedWriter.seek when passing an object with a specially crafted __index__ - #143577
tomasr8 wants to merge 15 commits into
Conversation
Sorry, something went wrong.
| @@ -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?
Sorry, something went wrong.
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!
Sorry, something went wrong.
| // 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.
Sorry, something went wrong.
There was a problem hiding this comment.
Oh, and CHECK_INITIALIZED also should be after PyNumber_AsOff_t. Add a test for concurrent detach().
Sorry, something went wrong.
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!
Sorry, something went wrong.
| @@ -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__().
Sorry, something went wrong.
There was a problem hiding this comment.
True, I updated the news entry with the root cause. Let me know if it's ok!
Sorry, something went wrong.
|
And I think TextIOWrapper has the same issue. See #143007. |
Sorry, something went wrong.
| # 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?
Sorry, something went wrong.
|
This PR is stale because it has been open for 30 days with no activity. |
Sorry, something went wrong.
Adds a check after the call to PyNumber_AsOff_t which could have closed the file as a side-effect.