gh-143008: fix TextIOWrapper.truncate via re-entrant flush by yihong0618 · Pull Request #143041 · python/cpython · GitHub

This patch fix all re-entrant flush problem
by adding an attr flushing
and. refactor the file check to _textiowrapper_flush

Issue: Null pointer dereference in TextIOWrapper.truncate via re-entrant flush #143008

all crash problem in 143008 can be fix cc @jackfromeast can you help to check?
thank you very much!

Signed-off-by: yihong0618 <zouzou0208@gmail.com>

I don't like adding a new flushing variable here. To me it doesn't solves the core issue: Code assumes self->buffer is non-NULL because of checks which become invalid by the time it's actually dereferenced/used (TOCTOU general bug class). I think TextIOWrapper already has the information we need to be looking at / checking for (CHECK_ATTACHED macro / self->detached, self->buffer == NULL, and self->ok, ...). Those seem to be tracking very close to the same thing and should be usable to solve this group of issues.

I think it would be better here to use CHECK_ATTACHED just before self->buffer is used / passed to a call / returned; making sure there aren't any other calls to interpreter methods betwen the CHECK_ATTACHED + self->buffer usage / dereference.

I suspect BufferedIO will need a similar set of fixes: It also has a concept of an underlying stream that can be "detached" and which user code may detach during various operations.

Some comments on specifics inline, the test_textio ones I think are important in further changes, implementation details less so if not adding/using the flushing member.

There's a number of mock helpers defined in test_io/utils.py to make it so can reduce this to just the methods that matter. I think self.MockRawIO will work here

thanks

can we make the gc / closing here more explicit? del wrapper?

thanks

Why can't this be in _io_TextIOWrapper_flush_impl?

it can, the design is cover it in _PyFile_Flush

dropped

every loop iteration / write call could execute arbitrary interpreter code that could invalidate self->buffer so likely need to check on each loop iteration.

will check, but I think maybe not need

dropped

I don't like adding a new flushing variable here. To me it doesn't solves the core issue: Code assumes self->buffer is non-NULL because of checks which become invalid by the time it's actually dereferenced/used (TOCTOU general bug class). I think TextIOWrapper already has the information we need to be looking at / checking for (CHECK_ATTACHED macro / self->detached, self->buffer == NULL, and self->ok, ...). Those seem to be tracking very close to the same thing and should be usable to solve this group of issues.

I think it would be better here to use CHECK_ATTACHED just before self->buffer is used / passed to a call / returned; making sure there aren't any other calls to interpreter methods betwen the CHECK_ATTACHED + self->buffer usage / dereference.

I suspect BufferedIO will need a similar set of fixes: It also has a concept of an underlying stream that can be "detached" and which user code may detach during various operations.

Some comments on specifics inline, the test_textio ones I think are important in further changes, implementation details less so if not adding/using the flushing member.

I think you are right but CHECK_ATTACHED in the middle of code is not a good name and easy to forget add it.

And I can change it to my first solution or waiting for other discuss here?

…akErJ.rst Co-authored-by: Cody Maloney <cmaloney@users.noreply.github.com>

I agree CHECK_ATTACHED is easy to forget and I'd argue this bug is the result of some cases missing it.. It is a relatively small change though and matches the rest of the textio.c code style / convention. I have been working towards changing textio.c more generally, but that's too big for a likely back-ported bugfix.

Can you explain what your first solution is? I'm not understanding what you're referring to.

I agree CHECK_ATTACHED is easy to forget and I'd argue this bug is the result of some cases missing it.. It is a relatively small change though and matches the rest of the textio.c code style / convention. I have been working towards changing textio.c more generally, but that's too big for a likely back-ported bugfix.

Can you explain what your first solution is? I'm not understanding what you're referring to.

#143008 (comment)

This will break the case when flush() was intentionally overridden.

I am not also sure that it fixes the core issue, it just makes a particular reproducer no longer working.

This will break the case when flush() was intentionally overridden.

I am not also sure that it fixes the core issue, it just makes a particular reproducer no longer working.

will dig it, thank you

I agree CHECK_ATTACHED is easy to forget and I'd argue this bug is the result of some cases missing it.. It is a relatively small change though and matches the rest of the textio.c code style / convention. I have been working towards changing textio.c more generally, but that's too big for a likely back-ported bugfix.

Can you explain what your first solution is? I'm not understanding what you're referring to.

after some check and work I do not think CHECK_ATTACHED before every is a good design will dig more
maybe I can find some better solution to fix all of them, if I failed will close this waiting for some expert to finish
this, thank you very much for review

Signed-off-by: yihong0618 <zouzou0208@gmail.com>

There's a number of mock helpers defined in test_io/utils.py to make it so can reduce this to just the methods that matter. I think self.MockRawIO will work here

I made some change but not sure its better idea, can you help to check? Thank you very much.

if you have better way to fix all of them feel free to close it~ thanks again

I'm starting to think a small refactor of TextIOWrapper to add and use a helper that always checks self->ok + self->attached would be better here; wrote up more thoughts in #143008 (comment)

I'm starting to think a small refactor of TextIOWrapper to add and use a helper that always checks self->ok + self->attached would be better here; wrote up more thoughts in #143008 (comment)

will try to understand and learn it thank you

TextIOWrapper keeps its underlying stream in a member called `self->buffer`. That stream can be detached by user code, such as custom `.flush` implementations resulting in `self->buffer` being set to NULL. The implementation often checked at the start of functions if `self->buffer` is in a good state, but did not always recheck after other Python code was called which could modify `self->buffer`. The cases which need to be re-checked are hard to spot so rather than rely on reviewer effort create better safety by making all self->buffer access go through helper functions. Thank you yihong0618 for the test, NEWS and initial implementation in pythongh-143041. Co-authored-by: yihong0618 <zouzou0208@gmail.com>

This PR is stale because it has been open for 30 days with no activity.

…stream access (#145957) TextIOWrapper keeps its underlying stream in a member called `self->buffer`. That stream can be detached by user code, such as custom `.flush` implementations resulting in `self->buffer` being set to NULL. The implementation often checked at the start of functions if `self->buffer` is in a good state, but did not always recheck after other Python code was called which could modify `self->buffer`. The cases which need to be re-checked are hard to spot so rather than rely on reviewer effort create better safety by making all self->buffer access go through helper functions. Thank you yihong0618 for the test, NEWS and initial implementation in gh-143041. Co-authored-by: yihong0618 <zouzou0208@gmail.com> Co-authored-by: Victor Stinner <vstinner@python.org>

#145957 was merged instead. Thanks for your contribution anyway! The merged change is based on your work and you're credited in the change ;-)

…lying stream access (python#145957) TextIOWrapper keeps its underlying stream in a member called `self->buffer`. That stream can be detached by user code, such as custom `.flush` implementations resulting in `self->buffer` being set to NULL. The implementation often checked at the start of functions if `self->buffer` is in a good state, but did not always recheck after other Python code was called which could modify `self->buffer`. The cases which need to be re-checked are hard to spot so rather than rely on reviewer effort create better safety by making all self->buffer access go through helper functions. Thank you yihong0618 for the test, NEWS and initial implementation in pythongh-143041. Co-authored-by: yihong0618 <zouzou0208@gmail.com> Co-authored-by: Victor Stinner <vstinner@python.org>

…rant detach (#159591) * gh-143008: Fix Null pointer dereferences in TextIOWrapper underlying stream access (#145957) TextIOWrapper keeps its underlying stream in a member called `self->buffer`. That stream can be detached by user code, such as custom `.flush` implementations resulting in `self->buffer` being set to NULL. The implementation often checked at the start of functions if `self->buffer` is in a good state, but did not always recheck after other Python code was called which could modify `self->buffer`. The cases which need to be re-checked are hard to spot so rather than rely on reviewer effort create better safety by making all self->buffer access go through helper functions. Thank you yihong0618 for the test, NEWS and initial implementation in gh-143041. Co-authored-by: yihong0618 <zouzou0208@gmail.com> Co-authored-by: Victor Stinner <vstinner@python.org> (cherry picked from commit db4b194) * gh-154523: Fix data-race in `TextIOWrapper.detach()` (#154565) (cherry picked from commit 8440a6a) * gh-157364: Fix use-after-free in io.TextIOWrapper during reentrant detach (#157370) Co-authored-by: Victor Stinner <vstinner@python.org> (cherry picked from commit e19dc47) --------- Co-authored-by: Cody Maloney <cmaloney@users.noreply.github.com> Co-authored-by: yihong0618 <zouzou0208@gmail.com> Co-authored-by: Brij Kapadia <97006829+brijkapadia@users.noreply.github.com> Co-authored-by: Shamil <ashm.tech@proton.me>

…rant detach (#159600) [3.15] gh-157364: Fix use-after-free in io.TextIOWrapper during reentrant detach (#159591) * gh-143008: Fix Null pointer dereferences in TextIOWrapper underlying stream access (#145957) TextIOWrapper keeps its underlying stream in a member called `self->buffer`. That stream can be detached by user code, such as custom `.flush` implementations resulting in `self->buffer` being set to NULL. The implementation often checked at the start of functions if `self->buffer` is in a good state, but did not always recheck after other Python code was called which could modify `self->buffer`. The cases which need to be re-checked are hard to spot so rather than rely on reviewer effort create better safety by making all self->buffer access go through helper functions. Thank you yihong0618 for the test, NEWS and initial implementation in gh-143041. (cherry picked from commit db4b194) * gh-154523: Fix data-race in `TextIOWrapper.detach()` (#154565) (cherry picked from commit 8440a6a) * gh-157364: Fix use-after-free in io.TextIOWrapper during reentrant detach (#157370) (cherry picked from commit e19dc47) --------- (cherry picked from commit ea1312b) Co-authored-by: Cody Maloney <cmaloney@users.noreply.github.com> Co-authored-by: yihong0618 <zouzou0208@gmail.com> Co-authored-by: Brij Kapadia <97006829+brijkapadia@users.noreply.github.com> Co-authored-by: Shamil <ashm.tech@proton.me>

fix: TextIOWrapper.truncate via re-entrant flush

f7eca0d

Signed-off-by: yihong0618 <zouzou0208@gmail.com>

bedevere-app Bot mentioned this pull request Dec 21, 2025

Null pointer dereference in TextIOWrapper.truncate via re-entrant flush #143008

Closed

bedevere-app Bot added the awaiting review label Dec 21, 2025

cmaloney reviewed Dec 21, 2025

View reviewed changes

Update Misc/NEWS.d/next/Library/2025-12-21-17-56-37.gh-issue-143008.a…

a96757b

…akErJ.rst Co-authored-by: Cody Maloney <cmaloney@users.noreply.github.com>

serhiy-storchaka reviewed Dec 23, 2025

View reviewed changes

fix: apply some suggestion

65d7dbb

Signed-off-by: yihong0618 <zouzou0208@gmail.com>

picnixz linked an issue Jan 1, 2026 that may be closed by this pull request

Null pointer dereference in TextIOWrapper.truncate via re-entrant flush #143008

Closed

picnixz removed a link to an issue Jan 1, 2026

Null pointer dereference in TextIOWrapper.truncate via re-entrant flush #143008

Closed

cmaloney mentioned this pull request Mar 15, 2026

gh-143008: Fix Null pointer dereferences in TextIOWrapper underlying stream access #145957

Merged

github-actions Bot added the stale Stale PR or inactive for long period of time. label May 4, 2026

vstinner closed this Jun 9, 2026

vstinner mentioned this pull request Oct 16, 2026

[3.14] gh-157364: Fix use-after-free in io.TextIOWrapper during reentrant detach #159600

Merged

+                          with self.subTest(name):
+                              wrapper = self.TextIOWrapper(EvilBuffer(BadRaw()), encoding='utf-8')
+                              self.assertRaisesRegex(RuntimeError, "reentrant", method)
+                              wrapper = None

                   PyObject *ret;
+                  self->flushing = 1;
                   do {
                       ret = PyObject_CallMethodOneArg(self->buffer, &_Py_ID(write), b);

+                      # instead of crashing.
+                      wrapper = None
+                      class BadRaw(self.RawIOBase):

+              static inline int
+              _textiowrapper_flush(textio *self)
+              {
+                  self->flushing = 1;

gh-143008: fix TextIOWrapper.truncate via re-entrant flush by yihong0618 · Pull Request #143041 · python/cpython · GitHub

Repository navigation

gh-143008: fix TextIOWrapper.truncate via re-entrant flush - #143041

gh-143008: fix TextIOWrapper.truncate via re-entrant flush#143041yihong0618 wants to merge 3 commits intopython:mainpython/cpython:mainfrom yihong0618:hy/close_issue_143008yihong0618/cpython:hy/close_issue_143008Copy head branch name to clipboard

Conversation

yihong0618 commented Dec 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmaloney left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yihong0618 commented Dec 21, 2025

Uh oh!

yihong0618 commented Dec 21, 2025

Uh oh!

cmaloney commented Dec 22, 2025

Uh oh!

yihong0618 commented Dec 22, 2025

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

yihong0618 commented Dec 24, 2025

Uh oh!

yihong0618 commented Dec 29, 2025

Uh oh!

yihong0618 commented Dec 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmaloney commented Jan 5, 2026

Uh oh!

yihong0618 commented Jan 5, 2026

Uh oh!

github-actions Bot commented May 4, 2026

Uh oh!

vstinner commented Jun 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gh-143008: fix TextIOWrapper.truncate via re-entrant flush#143041
yihong0618 wants to merge 3 commits into
python:mainfrom
yihong0618:hy/close_issue_143008

yihong0618 commented Dec 21, 2025 •
edited
Loading

yihong0618 commented Dec 29, 2025 •
edited
Loading