GeezerVison

cc @methane @eryksun

Thank you. The patch looks good to me.

Now it crashes if any of these calls fails: length ofself->pending_bytes doesn't match self->pending_bytes_count on return.

This PR seems to be addressing a similar issue as #119107 but I don't think that this PR fixes the race condition present when multi-threaded writes happen.

After fixing my original test case, I think that this minimal patch addresses the issue in practical terms:

diff --git a/Modules/_io/textio.c b/Modules/_io/textio.c
index 3de4c06704..e084d04428 100644
--- a/Modules/_io/textio.c
+++ b/Modules/_io/textio.c
@@ -1701,16 +1701,16 @@ _io_TextIOWrapper_write_impl(textio *self, PyObject *text)
         bytes_len = PyBytes_GET_SIZE(b);
     }
 
-    if (self->pending_bytes == NULL) {
-        self->pending_bytes_count = 0;
-        self->pending_bytes = b;
-    }
-    else if (self->pending_bytes_count + bytes_len > self->chunk_size) {
+    if (self->pending_bytes_count + bytes_len > self->chunk_size) {
         // Prevent to concatenate more than chunk_size data.
         if (_textiowrapper_writeflush(self) < 0) {
             Py_DECREF(b);
             return NULL;
         }
+    }
+
+    if (self->pending_bytes == NULL) {
+        assert(self->pending_bytes_count == 0);
         self->pending_bytes = b;
     }
     else if (!PyList_CheckExact(self->pending_bytes)) {

there is still an issue where since there isn't a lock AFAIK, another thread could modify self->pending_bytes between check and assignment here:

    if (self->pending_bytes == NULL) {
        assert(self->pending_bytes_count == 0);
        self->pending_bytes = b;
    }

but that doesn't seem to be an issue in practice.

I should note that there's a similar race condition issue in _textiowrapper_writeflush where we don't ensure that the list hasn't been modified between for (Py_ssize_t i = 0; i < PyList_GET_SIZE(pending); i++) { and self->pending_bytes = NULL;.

That's a bit more scary, given that there's an entire list iteration between that PyList_GET_SIZE(pending) call and nuking the entire list. #119107 tries to address that, though there's still a potential race with that change as well (but I have yet to actually provoke that issue with or without the change to writeflush, so it's largely theoretical at this point).

I should note that there's a similar race condition issue in _textiowrapper_writeflush where we don't ensure that the list hasn't been modified between for (Py_ssize_t i = 0; i < PyList_GET_SIZE(pending); i++) { and self->pending_bytes = NULL;.

Note that default Python build with GIL doesn't have such race.

_textiowrapper_writeflush releases GIL when it calls buffer.write():

    self->pending_bytes_count = 0;
    self->pending_bytes = NULL;
    Py_DECREF(pending);

    PyObject *ret;
    do {
        ret = PyObject_CallMethodOneArg(self->buffer, &_Py_ID(write), b);
    } while (ret == NULL && _PyIO_trap_eintr());

Other thread may call TextIOWrapper.write(b) and it appends b to self->pending_bytes.
We can not assume self->pending_bytes == NULL and self->pending_bytes_count == 0 after _textiowrapper_writeflush. That was the bug.

Note that default Python build with GIL doesn't have such race.
_textiowrapper_writeflush releases GIL when it calls buffer.write()

Ah, that makes sense. Thank you for explaining!

Thanks @chgnrdv for the PR, and @methane for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

GH-119964 is a backport of this pull request to the 3.13 branch.

GH-119965 is a backport of this pull request to the 3.12 branch.

Thanks for bringing this home! Does this qualify as a security issue? I'm wondering if it will be backported to 3.10.

GH-120314 is a backport of this pull request to the 3.11 branch.