Skip to content

Conversation

@superboy-zjc
Copy link
Contributor

@superboy-zjc superboy-zjc commented Jan 4, 2026

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.

…er access

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.
picnixz
picnixz previously requested changes Jan 4, 2026
Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you check if we also have an exception for the pure Python implementation?

@bedevere-app
Copy link

bedevere-app bot commented Jan 4, 2026

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.

@picnixz picnixz changed the title gh-143378: Fix use-after-free BytesIO write via re-entrant buffer access gh-143378: Fix UAF when BytesIO is concurrently mutated during write operations Jan 4, 2026
@superboy-zjc
Copy link
Contributor Author

superboy-zjc commented Jan 4, 2026

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 and others added 6 commits January 4, 2026 11:00
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@superboy-zjc
Copy link
Contributor Author

I have made the requested changes; please review again.

@bedevere-app
Copy link

bedevere-app bot commented Jan 4, 2026

Thanks for making the requested changes!

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

@bedevere-app bedevere-app bot requested a review from picnixz January 4, 2026 19:27
@picnixz
Copy link
Member

picnixz commented Jan 4, 2026

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)

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that it would be better to call check_closed() and check_exports() after we get a buffer.

Correspondingly, in the Python code we should perform these checks inside the with memoryview(...) block.

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@superboy-zjc
Copy link
Contributor Author

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.

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. 👍

You can either merge tests for write() and writelines() since they share the same class (it only needs copying the last two lines). Or leave it as is. Either is fine.

Please add a NEWS entry.

There is other bug in the Python implementation. It is easy to fix, but you can prefer to open a separate issue for it, because the test will be more complicated (__buffer__() needs to return different values).

n = view.nbytes # Size of any bytes-like object
if self.closed:
raise ValueError("write to closed file")
if n == 0:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rest of the code should be moved in the with block.

Otherwise writing to BytesIO after seeking can not just overwrite existing bytes, but move bytes after the current position.

This is a separate bug, so you can open a separate issue and fix it in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest removing the first if self.closed: at the function entry point.

Copy link
Contributor Author

@superboy-zjc superboy-zjc Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest removing the first if self.closed: at the function entry point.

Thank you for pointing it out! I've updated the code. This is very helpful to keep the consistency.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #143602.

# Prevent crashes when memio.write() concurrently exports 'memio'.
# See: https://github.com/python/cpython/issues/143378.
class B:
buf = None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not needed if you never read it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think we should just have memio.getbuffer()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to save a reference to it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right, so _ = memio.getbuffer()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to keep a reference after returning from __buffer__().

Initially I though about a local variable in the outer scope (an initialization would be needed in that case), but an attribute of the written object works as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right. Sorry. Yes an attribute would be just the best :')

@picnixz picnixz dismissed their stale review January 5, 2026 19:16

concerns were addressed

Copy link
Contributor

@kumaraditya303 kumaraditya303 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bytesio.c changes LGTM

@serhiy-storchaka serhiy-storchaka merged commit 6d54b6a into python:main Jan 9, 2026
46 checks passed
@serhiy-storchaka serhiy-storchaka added needs backport to 3.13 bugs and security fixes needs backport to 3.14 bugs and security fixes labels Jan 9, 2026
@miss-islington-app
Copy link

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
Copy link

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
Copy link

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 pushed a commit to miss-islington/cpython that referenced this pull request Jan 9, 2026
…ted during write operations (pythonGH-143408)

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.

Ensure that PyObject_GetBuffer() is called before validation checks.
(cherry picked from commit 6d54b6a)

Co-authored-by: zhong <60600792+superboy-zjc@users.noreply.github.com>
@bedevere-app
Copy link

bedevere-app bot commented Jan 9, 2026

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

@bedevere-app bedevere-app bot removed the needs backport to 3.14 bugs and security fixes label Jan 9, 2026
serhiy-storchaka pushed a commit to serhiy-storchaka/cpython that referenced this pull request Jan 9, 2026
…ly mutated during write operations (pythonGH-143408)

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.

Ensure that PyObject_GetBuffer() is called before validation checks.
(cherry picked from commit 6d54b6a)

Co-authored-by: zhong <60600792+superboy-zjc@users.noreply.github.com>
@bedevere-app
Copy link

bedevere-app bot commented Jan 9, 2026

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

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Jan 9, 2026
serhiy-storchaka pushed a commit that referenced this pull request Jan 9, 2026
…ated during write operations (GH-143408) (GH-143599)

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.

Ensure that PyObject_GetBuffer() is called before validation checks.
(cherry picked from commit 6d54b6a)

Co-authored-by: zhong <60600792+superboy-zjc@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants