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

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 <[email protected]>

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 <[email protected]>

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

fix: TextIOWrapper.truncate via re-entrant flush

f7eca0d

Signed-off-by: yihong0618 <[email protected]>

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 <[email protected]>

serhiy-storchaka reviewed Dec 23, 2025

View reviewed changes

Uh oh!

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

Are you sure you want to change the base?

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

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yihong0618 commented Dec 21, 2025 •

edited

Loading