Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions Lib/test/test_io/test_textio.py
Original file line number Diff line number Diff line change
Expand Up @@ -1560,6 +1560,54 @@ def closed(self):
wrapper = self.TextIOWrapper(raw)
wrapper.close() # should not crash

def test_reentrant_detach_during_flush(self):
# gh-143008: Reentrant detach() during flush should raise RuntimeError
# instead of crashing.
wrapper = None

class BadRaw(self.RawIOBase):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a number of mock helpers defined in test_io/utils.py to make it so can reduce this to just the methods that matter. I think self.MockRawIO will work here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks

def write(self, b): return len(b)
def read(self, n=-1): return b''
def readable(self): return True
def writable(self): return True
def seekable(self): return True
def seek(self, pos, whence=0): return 0
def tell(self): return 0

class EvilBuffer(self.BufferedRandom):
detach_on_write = False

def flush(self):
if wrapper is not None and not self.detach_on_write:
wrapper.detach()
return super().flush()

def write(self, b):
if wrapper is not None and self.detach_on_write:
wrapper.detach()
return len(b)

tests = [
('truncate', lambda: wrapper.truncate(0)),
('close', lambda: wrapper.close()),
('detach', lambda: wrapper.detach()),
('seek', lambda: wrapper.seek(0)),
('tell', lambda: wrapper.tell()),
('reconfigure', lambda: wrapper.reconfigure(line_buffering=True)),
]
for name, method in tests:
with self.subTest(name):
wrapper = self.TextIOWrapper(EvilBuffer(BadRaw()), encoding='utf-8')
self.assertRaisesRegex(RuntimeError, "reentrant", method)
wrapper = None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we make the gc / closing here more explicit? del wrapper?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks


with self.subTest('read via writeflush'):
EvilBuffer.detach_on_write = True
wrapper = self.TextIOWrapper(EvilBuffer(BadRaw()), encoding='utf-8')
wrapper.write('x')
self.assertRaisesRegex(RuntimeError, "reentrant", wrapper.read)
wrapper = None


class PyTextIOWrapperTest(TextIOWrapperTest, PyTestCase):
shutdown_error = "LookupError: unknown encoding: ascii"
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix crash in :class:`io.TextIOWrapper` when reentrant :meth:`io.TextIOBase.detach` called.
33 changes: 26 additions & 7 deletions Modules/_io/textio.c
Original file line number Diff line number Diff line change
Expand Up @@ -667,6 +667,7 @@ struct textio
PyObject_HEAD
int ok; /* initialized? */
int detached;
int flushing; /* prevent reentrant detach during flush */
Py_ssize_t chunk_size;
PyObject *buffer;
PyObject *encoding;
Expand Down Expand Up @@ -725,6 +726,16 @@ struct textio

#define textio_CAST(op) ((textio *)(op))

/* gh-143007 need to check for reentrant flush */
static inline int
_textiowrapper_flush(textio *self)
{
self->flushing = 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't this be in _io_TextIOWrapper_flush_impl?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it can, the design is cover it in _PyFile_Flush

int result = _PyFile_Flush((PyObject *)self);
self->flushing = 0;
return result;
}

static void
textiowrapper_set_decoded_chars(textio *self, PyObject *chars);

Expand Down Expand Up @@ -1108,6 +1119,7 @@ _io_TextIOWrapper___init___impl(textio *self, PyObject *buffer,

self->ok = 0;
self->detached = 0;
self->flushing = 0;

if (encoding == NULL) {
PyInterpreterState *interp = _PyInterpreterState_GET();
Expand Down Expand Up @@ -1422,7 +1434,7 @@ _io_TextIOWrapper_reconfigure_impl(textio *self, PyObject *encoding,
return NULL;
}

if (_PyFile_Flush((PyObject *)self) < 0) {
if (_textiowrapper_flush(self) < 0) {
return NULL;
}
self->b2cratio = 0;
Expand Down Expand Up @@ -1565,7 +1577,12 @@ _io_TextIOWrapper_detach_impl(textio *self)
{
PyObject *buffer;
CHECK_ATTACHED(self);
if (_PyFile_Flush((PyObject *)self) < 0) {
if (self->flushing) {
PyErr_SetString(PyExc_RuntimeError,
"reentrant call to detach() is not allowed");
return NULL;
}
if (_textiowrapper_flush(self) < 0) {
return NULL;
}
buffer = self->buffer;
Expand Down Expand Up @@ -1636,9 +1653,11 @@ _textiowrapper_writeflush(textio *self)
Py_DECREF(pending);

PyObject *ret;
self->flushing = 1;
do {
ret = PyObject_CallMethodOneArg(self->buffer, &_Py_ID(write), b);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

every loop iteration / write call could execute arbitrary interpreter code that could invalidate self->buffer so likely need to check on each loop iteration.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will check, but I think maybe not need

} while (ret == NULL && _PyIO_trap_eintr());
self->flushing = 0;
Py_DECREF(b);
// NOTE: We cleared buffer but we don't know how many bytes are actually written
// when an error occurred.
Expand Down Expand Up @@ -2583,7 +2602,7 @@ _io_TextIOWrapper_seek_impl(textio *self, PyObject *cookieObj, int whence)
goto fail;
}

if (_PyFile_Flush((PyObject *)self) < 0) {
if (_textiowrapper_flush(self) < 0) {
goto fail;
}

Expand Down Expand Up @@ -2630,7 +2649,7 @@ _io_TextIOWrapper_seek_impl(textio *self, PyObject *cookieObj, int whence)
goto fail;
}

if (_PyFile_Flush((PyObject *)self) < 0) {
if (_textiowrapper_flush(self) < 0) {
goto fail;
}

Expand Down Expand Up @@ -2757,7 +2776,7 @@ _io_TextIOWrapper_tell_impl(textio *self)

if (_textiowrapper_writeflush(self) < 0)
return NULL;
if (_PyFile_Flush((PyObject *)self) < 0) {
if (_textiowrapper_flush(self) < 0) {
goto fail;
}

Expand Down Expand Up @@ -2967,7 +2986,7 @@ _io_TextIOWrapper_truncate_impl(textio *self, PyObject *pos)
{
CHECK_ATTACHED(self)

if (_PyFile_Flush((PyObject *)self) < 0) {
if (_textiowrapper_flush(self) < 0) {
return NULL;
}

Expand Down Expand Up @@ -3165,7 +3184,7 @@ _io_TextIOWrapper_close_impl(textio *self)
PyErr_Clear();
}
}
if (_PyFile_Flush((PyObject *)self) < 0) {
if (_textiowrapper_flush(self) < 0) {
exc = PyErr_GetRaisedException();
}

Expand Down
Loading