gh-119506: fix _io.TextIOWrapper.write() write during flush

[chgnrdv]

Fixes #119506

• check if call to _textiowrapper_writeflush() has left any data in self->pending_bytes. If so, store them in self->pending_bytes after b
• add test

• Issue: _io.TextIOWrapper.write: write during flush causes pending_bytes length mismatch #119506

[chgnrdv]

cc @methane @eryksun

[methane]

Thank you. The patch looks good to me.

[chgnrdv]

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

cpython/Modules/_io/textio.c

Line 1735 in bf5b646

PyObject *list = PyList_New(2);

cpython/Modules/_io/textio.c

Line 1745 in bf5b646

if (PyList_Append(self->pending_bytes, b) < 0) {

[methane]

why do you swap data here?

• self->pending can be list
• call from other thread might be returned already.
  • "first returned first write" seems correct than "first called first write".

[elistevens]

If you look at the test case, where flushing the stream triggers a write, it's not a threading thing, it's because of write being called recursively.

[methane]

Even single thread case, it is not possble to guarantee "first called, first written."
Simple implementation is more important.

[methane]

How about while instead of if?

[elistevens]

That would also need a check to make sure that bytes_len > self->chunk_size isn't true.

[elistevens]

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.

[elistevens]

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.

[elistevens]

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).

[methane]

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.

[elistevens]

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!

[miss-islington-app]

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

[bedevere-app]

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

[bedevere-app]

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

[elistevens]

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

[methane]

I am not sure about this patch will be backported to security branches.
If it will, this line needs additional fixes. So backport is not simple.

[bedevere-app]

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