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

[yihong0618]

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

• Issue: gh-143008

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

[yihong0618]

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]

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

[cmaloney]

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]

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.

https://github.com/python/cpython/issues/143008#issuecomment-3677398247

[yihong0618]

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]

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

[yihong0618]

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]

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 https://github.com/python/cpython/issues/143008#issuecomment-3708981863

[yihong0618]

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