GeezerVison

wow that is cool thanks

@colesbury could you take a look as you've done some recent work on TextIO safety around threading. These cases are generally single threaded but generally the pattern "Implementation usually checked state; then did an operation which could modify it and didn't recheck".

I'm a bit confused about the relationship here between _textiowrapper_buffer_safe and CHECK_ATTACHED. When can buffer be NULL and CHECK_ATTACHED not trigger?

Less important, but please match the CPython style for C code, e.g.:

• { on new lines for functions definitions
• when wrapping function calls, indent continuation lines to align with the call site

I'm a bit confused about the relationship here between _textiowrapper_buffer_safe and CHECK_ATTACHED. When can buffer be NULL and CHECK_ATTACHED not trigger?

CHECK_ATTACHED, by way of CHECK_INITIALIZED, checks self->ok first. The self->ok flag is set to 0 at start of __init__, 1 at the end, and cleared/reset to 0 during .clear() as well as at the start of the Py_tp_dealloc. During __init__ there are calls to check the file is in a good state and get some attributes (remember: self->buf is the underlying file-like object not an internal buffer).

The more complicated case is during deletion where there is an interaction with the base class. TextIOWrapper implements Py_tp_dealloc and its base class (TextIOBase which has a base class IOBase) implements a Py_tp_finalize with the function iobase_finalize. That function tries to ensure all data is written in the full I/O "stack" before any part of it is closed out of order. It tries to have Text I/O flush and close, then Buffered I/O (self->buf), then the Raw I/O. If there is an underlying file (self->buf != NULL) during those operations need to try and write data in the TextIOWrapper even though self->ok is 0.

Adding to the complexity here all the objects involved and the multiple operations (flush(), close(), .closed) can be overridden and could call TextIOWrapper.clear() or TextIOWrapper.detach() causing self->buf to become NULL.

Less important, but please match the CPython style for C code, e.g.:

Sorry for missing those bits, doing a review of that and will update for it shortly.

There are a couple cases I'm not sure how to PEP 7, in particular:

/* hits 81 cols */
        PyObject *bytes = _textiowrapper_buffer_callmethod_noargs(self,
                                                                  &_Py_ID(read));
/* hits 81 cols */
            res = _textiowrapper_buffer_callmethod_onearg(self,
                                                          &_Py_ID(_dealloc_warn),
                                                          (PyObject *)self);


PyObject *input_chunk = _textiowrapper_buffer_callmethod_onearg(self,
            &_Py_ID(read), bytes_to_feed);

A couple directions I looked at but don't know how to decide between:

1. Make the variable name shorter (increases diff)
2. Make the function name shorter (ex. _textiowrapper_buf_... or _textio_buffer_, _textiowrapper_buf_callmeth_noargs)
3. Whats the right indentation to just put on the next line?

I think it's fine to make the function names shorter, i.e., buffer_callmethod_onearg instead of _textiowrapper_buffer_callmethod_onearg. File-scope static functions don't need a prefix or underscore.

In general the preference is for wrapping function calls like the following (from PEP 7 example):

PyErr_Format(PyExc_TypeError,
             "cannot create '%.100s' instances",
             type->tp_name);

But if that's not practical due to overly long lines, than just try to fit the style of surround code. For example, from elsewhere in textio.c:

        PyObject *incrementalDecoder = PyObject_CallFunctionObjArgs(
            (PyObject *)state->PyIncrementalNewlineDecoder_Type,
            self->decoder, self->readtranslate ? Py_True : Py_False, NULL);

This should also resolve gh-143007

I think thish is ready for re-review @colesbury

_pyio uses None to indicate no buffer. Can definitely migrate _io.TextIOWrapper to do that but that's a substantial code delta for an edge case and doesn't simplify fixing the time of check time of use issue here. Moving to None would make it calling methods on None rather than a null pointer de-reference. Would still need most this change of "always check just before using".

re: Recursion most the tests don't rely on it. The one that does is when flush() calls detach itself which results in another call to flush(). gh-143041 resolved that by making a new "state" effectively of "are detaching, don't detach again". Can do a similar pattern here if want to eliminate the recursion potentially: In .detach() change self->detached and/or self->ok before calling _PyFile_Flush(self) and making sure .flush() handles that case appropriately. I'm hesitant to do that as it changes the set of states a TextIOWrapper can be in a bit and I don't have those well mapped out mentally. Limiting recursion means the test exits after some number of recursion instances which validates it no longer segfaults / NULL pointer dereference is resolved.

Merged, thanks @cmaloney for the fix.

No one should meet this bug in practice, since it requires a custom buffer object which detach() when the stream is not supposed to be detached. But well, it's good to fix the code to support this corner case.

@cmaloney: You can create a change for the 3.15 branch to backport the critical section if you want.

Critical section only backport: gh-151203