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.

[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]

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.

[vstinner]

Good: the test does crash without the fix. So it confirms that the test checks the modified C code.

$ ./python -m test -v test_io.test_textio -m test_reentrant_detach_during_read
(...)Warning -- Unraisable exception
Exception ignored while finalizing file <_io.BufferedReader>:
Traceback (most recent call last):
  File "/home/vstinner/python/main/Lib/test/test_io/test_textio.py", line 1651, in readinto
    wrapper.detach()
    ~~~~~~~~~~~~~~^^
RuntimeError: reentrant call inside <_io.BufferedReader>
Fatal Python error: Segmentation fault

<Cannot show all threads while the GIL is disabled>
Stack (most recent call first):
  File "/home/vstinner/python/main/Lib/test/test_io/test_textio.py", line 1661 in test_reentrant_detach_during_read

[vstinner]

LGTM. Returning a strong reference in buffer_access_safe() sounds like a safe move. I checked that buffer_access_safe() callers do Py_DECREF() on the buffer.

./python -m test -R 3:3 test_io doesn't report leaks. But it reports negative reference count which is surprising:

$ ./python -m test -R 3:3 test_io.test_textio
(...)
test_io.test_textio leaked [-1, -1, -1] references, sum=-3 (this is fine)

$ ./python -m test -R 3:3 test_io.test_bufferedio
(...)
test_io.test_bufferedio leaked [-3, -3, -3] references, sum=-9 (this is fine)

[vstinner]

$ ./python -m test -R 3:3 test_io.test_textio
test_io.test_textio leaked [-1, -1, -1] references, sum=-3 (this is fine)
$ ./python -m test -R 3:3 test_io.test_bufferedio
test_io.test_bufferedio leaked [-3, -3, -3] references, sum=-9 (this is fine)

Ah, I get the same result on the main branch. So it's not caused by this change.

[vstinner]

Thanks the updates. I enabled auto-merge, please don't touch the PR (your branch) anymore. This change is a bugfix which should be backported to 3.14 and 3.15 branches.

[cmaloney]

CI failed on test_profiling which looks independent, re-running.

[miss-islington-app]

Thanks @ashm-dev for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14, 3.15.
🐍🍒⛏🤖

[miss-islington-app]

Sorry, @ashm-dev and @vstinner, I could not cleanly backport this to 3.15 due to a conflict.

Please backport manually with cherry_picker, see the devguide for more information.

cherry_picker e19dc470f4c2afd7ac944896232a3a6ca0204399 3.15

[miss-islington-app]

Sorry, @ashm-dev and @vstinner, I could not cleanly backport this to 3.14 due to a conflict.

Please backport manually with cherry_picker, see the devguide for more information.

cherry_picker e19dc470f4c2afd7ac944896232a3a6ca0204399 3.14

[vstinner]

Aha, more conflicts. I will backport the change to 3.14 and 3.15 once the 3.15 branch will be unblocked (next week).

[vstinner]

PR merged, thanks @ashm-dev for your fix.