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

yihong0618 · 2025-12-21T10:02:20Z

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>

cmaloney

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.

cmaloney · 2025-12-21T21:22:42Z

Lib/test/test_io/test_textio.py

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


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

cmaloney · 2025-12-21T21:25:52Z

Lib/test/test_io/test_textio.py

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


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

Misc/NEWS.d/next/Library/2025-12-21-17-56-37.gh-issue-143008.aakErJ.rst

cmaloney · 2025-12-21T21:30:29Z

Modules/_io/textio.c

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


Why can't this be in _io_TextIOWrapper_flush_impl?

it can, the design is cover it in _PyFile_Flush

cmaloney · 2025-12-21T22:12:54Z

Modules/_io/textio.c

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


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

yihong0618 · 2025-12-21T23:13:15Z

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.

yihong0618 · 2025-12-21T23:18:55Z

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>

cmaloney · 2025-12-22T03:56:46Z

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.

yihong0618 · 2025-12-22T04:18:46Z

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)

serhiy-storchaka

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.

yihong0618 · 2025-12-24T00:27:17Z

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

yihong0618 · 2025-12-29T09:08:12Z

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>

yihong0618 · 2025-12-29T09:43:25Z

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

cmaloney · 2026-01-05T05:25:56Z

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)

yihong0618 · 2026-01-05T05:30:40Z

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>

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

Open

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

Open

picnixz removed a link to an issue Jan 1, 2026

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

Open

cmaloney mentioned this pull request Mar 15, 2026

gh-143008: Safer TextIOWrapper underlying stream access #145957

Open

Uh oh!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yihong0618 commented Dec 21, 2025 •

edited

Loading

yihong0618 commented Dec 29, 2025 •

edited

Loading