cpython icon indicating copy to clipboard operation
cpython copied to clipboard

gh-143378: Fix UAF when `BytesIO` is concurrently mutated during `write` operations

Open superboy-zjc opened this issue 1 month ago • 6 comments

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

superboy-zjc avatar Jan 04 '26 04:01 superboy-zjc

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.

bedevere-app[bot] avatar Jan 04 '26 17:01 bedevere-app[bot]

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

superboy-zjc avatar Jan 04 '26 18:01 superboy-zjc

I have made the requested changes; please review again.

superboy-zjc avatar Jan 04 '26 19:01 superboy-zjc

Thanks for making the requested changes!

@picnixz: please review the changes made to this pull request.

bedevere-app[bot] avatar Jan 04 '26 19:01 bedevere-app[bot]

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)

picnixz avatar Jan 04 '26 19:01 picnixz

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.

superboy-zjc avatar Jan 05 '26 00:01 superboy-zjc

Thanks @superboy-zjc for the PR, and @serhiy-storchaka for merging it šŸŒ®šŸŽ‰.. I'm working now to backport this PR to: 3.13. šŸšŸ’ā›šŸ¤–

miss-islington-app[bot] avatar Jan 09 '26 11:01 miss-islington-app[bot]

Thanks @superboy-zjc for the PR, and @serhiy-storchaka for merging it šŸŒ®šŸŽ‰.. I'm working now to backport this PR to: 3.14. šŸšŸ’ā›šŸ¤–

miss-islington-app[bot] avatar Jan 09 '26 11:01 miss-islington-app[bot]

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

miss-islington-app[bot] avatar Jan 09 '26 11:01 miss-islington-app[bot]

GH-143599 is a backport of this pull request to the 3.14 branch.

bedevere-app[bot] avatar Jan 09 '26 11:01 bedevere-app[bot]

GH-143600 is a backport of this pull request to the 3.13 branch.

bedevere-app[bot] avatar Jan 09 '26 11:01 bedevere-app[bot]