-
-
Notifications
You must be signed in to change notification settings - Fork 33.7k
gh-143008: fix TextIOWrapper.truncate via re-entrant flush #143041
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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): | ||
| 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we make the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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" | ||
|
|
||
| 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. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
|
|
@@ -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; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why can't this be in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
|
||
|
|
@@ -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(); | ||
|
|
@@ -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; | ||
|
|
@@ -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; | ||
|
|
@@ -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); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. every loop iteration /
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
|
@@ -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; | ||
| } | ||
|
|
||
|
|
@@ -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; | ||
| } | ||
|
|
||
|
|
@@ -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; | ||
| } | ||
|
|
||
|
|
@@ -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; | ||
| } | ||
|
|
||
|
|
@@ -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(); | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
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.pyto make it so can reduce this to just the methods that matter. I thinkself.MockRawIOwill work hereThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks