gh-143378: Fix UAF when `BytesIO` is concurrently mutated during `write` operations
PyObject_GetBuffer() can execute user code (e.g. via buffer), which may close or otherwise mutate a BytesIO object while write() or writelines() is in progress. This could invalidate the internal buffer and lead to a use-after-free.
Temporarily bump the exports counter while acquiring the input buffer to block re-entrant mutation, and add regression tests to ensure such cases raise BufferError instead of crashing.
- Issue: gh-143378
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.
Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
Can you check if we also have an exception for the pure Python implementation?
Hi I can confirm currently we don't have an exception catching the concurrent mutation during a write in Python implementation. Should we keep this behavior consistent between C and Python version?
I'd be happy to add this logic check and unit tests as well. A potential patch for python version would be something like:
diff --git a/Lib/_pyio.py b/Lib/_pyio.py
index 69a088df8f..5b684c8f46 100644
--- a/Lib/_pyio.py
+++ b/Lib/_pyio.py
@@ -955,6 +955,8 @@ def write(self, b):
raise TypeError("can't write str to binary stream")
with memoryview(b) as view:
n = view.nbytes # Size of any bytes-like object
+ if self.closed:
+ raise ValueError("write to closed file")
if n == 0:
return 0
I have made the requested changes; please review again.
Thanks for making the requested changes!
@picnixz: please review the changes made to this pull request.
Hi I can confirm currently we don't have an exception catching the concurrent mutation during a write in Python implementation. Should we keep this behavior consistent between C and Python version?
I think but let's ask @cmaloney and @serhiy-storchaka about this. Sometimes it's not a good idea to force an exception just for consistency and it may not be useful either (and we apply GIGO paradigm in this case)
If the stream is closed concurrently with writing, BufferError is not an expected exception. You should get a usual exception that you get when trying to write to already closed stream.
Please add also a test for
writelines().It would also be interesting to add a test when the stream buffer is exported concurrently with writing -- in that case BufferError can be an expected exception.
Sounds great! I've updated the code following the suggestion. It indeed improves consistency and matches the expected behavior.
Regarding the unit tests, I've added coverage for concurrent export and close for both write() and writelines. Currently they are implemented as separate test cases for clarity. I did consider subtests to reduce duplication, but this felt a bit clearer in this case. Iām happy to refactor them into subtests if you think that would be preferable.
Thanks @superboy-zjc for the PR, and @serhiy-storchaka for merging it š®š.. I'm working now to backport this PR to: 3.13. ššāš¤
Thanks @superboy-zjc for the PR, and @serhiy-storchaka for merging it š®š.. I'm working now to backport this PR to: 3.14. ššāš¤
Sorry, @superboy-zjc and @serhiy-storchaka, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 6d54b6ac7d5744e1f59d784c8e020d632d2959a3 3.13
GH-143599 is a backport of this pull request to the 3.14 branch.
GH-143600 is a backport of this pull request to the 3.13 branch.