gh-157364: Fix use-after-free in TextIOWrapper during reentrant detach

[ashm-dev]

• Issue: io.TextIOWrapper UAF triggered by readline() #157364

[vstinner]

LGTM. I just have a last request on the test.

@cmaloney: Would you mind to double check this change? You wrote the first iteration if I recall correctly.

[vstinner]

Can you check that wrapper is actually detached? Maybe get wrapper.buffer and expect PyExc_ValueError("underlying buffer has been detached")?

[vstinner]

@ ashm-dev: You didn't reply to my request.

[cmaloney]

I will need a couple more days to look at this, it seems like this is more invasive than necessary to me and makes a number of not needed for the core UAF bug report.

[cmaloney]

For gh-157364 I think the bug, and solution, is actually in Buffered I/O. This PR changes Text I/O to keep one more reference, which may also be needed. The issue though is that Buffered I/O has an internal allocation which it passes as an argument to the Raw I/O .readinto method. That Raw I/O call may store and use that reference for an arbitrary amount of time. As long as it is stored the Buffered I/O should not get deallocated.

That makes two pieces to fix here:

1. The Buffer Protocol object currently doesn't reference the Buffered I/O that allocates and deallocates the buffer. That means if the buffer is stored anywhere than used later you can get a use after free. No Text I/O needed.
   

   cpython/Modules/_io/bufferedio.c

   Line 1629 in fd0970c

   if (PyBuffer_FillInfo(&buf, NULL, start, len, 0, PyBUF_CONTIG) == -1)

2. The Buffered I/O while buffered.read() is executing gets de-allocated. Keeping an additional reference in the Text I/O for that case will prevent that but it also feels like "When in a method on an object the interpreter should keep that object alive".

[ashm-dev]

Thanks for looking into this, @cmaloney!

Regarding point 1: passing (PyObject *)self to PyBuffer_FillInfo at line 1629 unfortunately doesn't work. Immediately after, PyMemoryView_FromBuffer(&buf) explicitly clears master.obj (Objects/memoryobject.c:787):

mbuf->master = *info;
mbuf->master.obj = NULL;

Because of this, the resulting memoryview still does not hold a reference to self (b.obj remains None). Additionally, PyBuffer_FillInfo acquires a new reference via Py_XNewRef(obj), which PyMemoryView_FromBuffer drops without Py_DECREF, causing self to leak a reference on every single read. Also, for read1(), the underlying buffer isn't even self->buffer—it is transient memory from PyBytesWriter.

Regarding point 2: CPython method calls via PyObject_CallMethod* do not automatically keep self alive if the caller holds only a borrowed reference. Since calling read1() triggers arbitrary Python code (via the raw stream's readinto), reentrantly detaching or clearing the buffer from Python code drops the last reference. Keeping a strong reference across the call in TextIOWrapper is the standard pattern across CPython for guarding against reentrant deallocation here.

[cmaloney]

For 1: The memoryview can be stored so it needs to keep the BufferedIO whose allocation it is filling alive otherwise use after free is possible. That there is no reference is a bug. I agree the current set of calls don't give a good path there, likely other memoryview / Buffer Protocol methods need to be used. It looks like PyMemoryView_FromObjectAndFlags may be a good fit.

[ashm-dev]

@cmaloney Done in 9763043.

[ashm-dev]

@cmaloney Gentle ping — could you please check if the updated `memoryview` approach in 9763043 looks good to you?

@vstinner All CI checks are passing and the review comments have been addressed. Ready for merge whenever you have a moment. Thanks!

[vstinner]

I don't understand well how _bufferedreader_raw_read_getbuffer() works. How is it different from the current PyBuffer_FillInfo() + PyMemoryView_FromBuffer() code?

[ashm-dev]

The key difference is reference ownership:

1. Previous code (PyMemoryView_FromBuffer):
   PyMemoryView_FromBuffer(&buf) explicitly clears mbuf->master.obj = NULL (see Objects/memoryobject.c:787). Because of this, the returned memoryview does not hold a reference to self (b.obj is None). If raw.readinto(b) stores the memoryview and BufferedReader is deallocated, subsequent access to the stored buffer leads to a use-after-free.

2. New code (_PyMemoryView_FromBufferProc):
   _PyMemoryView_FromBufferProc calls _bufferedreader_raw_read_getbuffer directly on &mbuf->master. There, PyBuffer_FillInfo sets view->obj = Py_NewRef(op), giving the managed buffer a strong reference to self that is not cleared. As a result, the memoryview keeps BufferedReader (and its internal allocation) alive as long as the memoryview is stored. When the memoryview is freed, PyBuffer_Release automatically decrefs self.

We can't use standard PyMemoryView_FromObject((PyObject *)self) here because BufferedReader doesn't implement the buffer protocol on itself (tp_as_buffer is NULL) — it only needs to expose this transient window into its buffer for readinto().

[cmaloney]

I have been triaging some old I/O bugs and came across gh-60198 which is exactly the BufferedReader memoryview bug here. That could be pulled out as a separate smaller PR that is likely quicker to land with its own NEWS.

I think this should have two news entries: one the io.TexTIOWrapper keeping a reference to self->buffer when calling it, which refers to gh-157364 / this issue. A second for the BufferedReader needs to be referenced by the memoryview (gh-60198). Also update the test_bufferedio.py comment for gh-60918. It has a bit more detail about why historically this has been hard to solve.

I don't like adding the two new members to buffered. The start + len we already know and I don't see why storing them as extra members helps solve the particular issue.

In the TextIO the Incref and decref living in very different functions I'm not a big fan of... I would much rather keep buffer_access_safe not modifying the refcnt then have each fo the callsites which does need to keep a non-borrowed reference do a Py_INCREF / Py_NewRef + Py_DECREF pair.

[ashm-dev]

@vstinner @cmaloney I narrowed this PR to the TextIOWrapper fix for gh-157364. I removed the BufferedReader change and added the requested check that the wrapper is detached after read() and readline(). Could you both please re-review? @cmaloney, is the remaining reference-handling approach acceptable to you?

[vstinner]

Nitpick: why moving this line after setting self->detached = 1;? If there is not specific reason, please move back the buffer assignment one line above.

[vstinner]

@ ashm-dev: You didn't reply to my request.

[vstinner]

I removed the BufferedReader change and added the requested check that the wrapper is detached after read() and readline().

Just a general remark: you're making many large changes on this PR. It's not easy to follow these changes.