From 14add40dbd71085d0f24dfc5d32c55c8ed66c440 Mon Sep 17 00:00:00 2001 From: Oren Milman Date: Mon, 21 Aug 2017 16:44:10 +0300 Subject: [PATCH 1/6] init commit --- Lib/test/test_io.py | 24 ++++++++++++++++++++ Modules/_io/textio.c | 17 ++++++++++++--- Modules/_testcapimodule.c | 46 +++++++++++++++++++++++++-------------- 3 files changed, 68 insertions(+), 19 deletions(-) diff --git a/Lib/test/test_io.py b/Lib/test/test_io.py index ab0cbe163613ec..81d97f5520a0c9 100644 --- a/Lib/test/test_io.py +++ b/Lib/test/test_io.py @@ -3245,6 +3245,30 @@ def _make_illegal_wrapper(): t = _make_illegal_wrapper() self.assertRaises(TypeError, t.read) + # Issue 31243: The interpreter shouldn't crash or raise a SystemError + # in case the return value of decoder's getstate() is invalid. + def _make_very_illegal_wrapper(getstate_ret_val): + class BadDecoder(): + def getstate(self): + return getstate_ret_val + class BadIncrementalDecoder(): + def __call__(self, dummy): + return BadDecoder() + quopri = codecs.lookup("quopri") + quopri_decoder = quopri.incrementaldecoder + quopri.incrementaldecoder = BadIncrementalDecoder() + try: + t = _make_illegal_wrapper() + finally: + quopri.incrementaldecoder = quopri_decoder + return t + t = _make_very_illegal_wrapper(42) + self.assertRaises(TypeError, t.read, 42) + t = _make_very_illegal_wrapper(()) + self.assertRaises(TypeError, t.read, 42) + t = _make_very_illegal_wrapper((1, 2)) + self.assertRaises(TypeError, t.read, 42) + def _check_create_at_shutdown(self, **kwargs): # Issue #20037: creating a TextIOWrapper at shutdown # shouldn't crash the interpreter. diff --git a/Modules/_io/textio.c b/Modules/_io/textio.c index 57b66d66c33745..215d2c86eb708c 100644 --- a/Modules/_io/textio.c +++ b/Modules/_io/textio.c @@ -1498,15 +1498,26 @@ textiowrapper_read_chunk(textio *self, Py_ssize_t size_hint) /* Given this, we know there was a valid snapshot point * len(dec_buffer) bytes ago with decoder state (b'', dec_flags). */ - if (PyArg_ParseTuple(state, "OO", &dec_buffer, &dec_flags) < 0) { + if (!PyTuple_Check(state)) { + PyErr_Format(PyExc_TypeError, + "decoder getstate() should have returned a tuple, " + "not '%.200s'", + Py_TYPE(state)->tp_name); + Py_DECREF(state); + return -1; + } + if (!PyArg_ParseTuple(state, + "OO;decoder getstate() should have returned a " + "2-tuple", &dec_buffer, &dec_flags)) + { Py_DECREF(state); return -1; } if (!PyBytes_Check(dec_buffer)) { PyErr_Format(PyExc_TypeError, - "decoder getstate() should have returned a bytes " - "object, not '%.200s'", + "decoder getstate() should have returned a tuple " + "whose first item is a bytes object, not '%.200s'", Py_TYPE(dec_buffer)->tp_name); Py_DECREF(state); return -1; diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index 95c10184159808..2952317e76db2d 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -864,8 +864,9 @@ test_L_code(PyObject *self) PyTuple_SET_ITEM(tuple, 0, num); value = -1; - if (PyArg_ParseTuple(tuple, "L:test_L_code", &value) < 0) + if (!PyArg_ParseTuple(tuple, "L:test_L_code", &value)) { return NULL; + } if (value != 42) return raiseTestError("test_L_code", "L code returned wrong value for long 42"); @@ -878,8 +879,9 @@ test_L_code(PyObject *self) PyTuple_SET_ITEM(tuple, 0, num); value = -1; - if (PyArg_ParseTuple(tuple, "L:test_L_code", &value) < 0) + if (!PyArg_ParseTuple(tuple, "L:test_L_code", &value)) { return NULL; + } if (value != 42) return raiseTestError("test_L_code", "L code returned wrong value for int 42"); @@ -1195,8 +1197,9 @@ test_k_code(PyObject *self) PyTuple_SET_ITEM(tuple, 0, num); value = 0; - if (PyArg_ParseTuple(tuple, "k:test_k_code", &value) < 0) + if (!PyArg_ParseTuple(tuple, "k:test_k_code", &value)) { return NULL; + } if (value != ULONG_MAX) return raiseTestError("test_k_code", "k code returned wrong value for long 0xFFF...FFF"); @@ -1215,8 +1218,9 @@ test_k_code(PyObject *self) PyTuple_SET_ITEM(tuple, 0, num); value = 0; - if (PyArg_ParseTuple(tuple, "k:test_k_code", &value) < 0) + if (!PyArg_ParseTuple(tuple, "k:test_k_code", &value)) { return NULL; + } if (value != (unsigned long)-0x42) return raiseTestError("test_k_code", "k code returned wrong value for long -0xFFF..000042"); @@ -1549,11 +1553,13 @@ test_s_code(PyObject *self) /* These two blocks used to raise a TypeError: * "argument must be string without null bytes, not str" */ - if (PyArg_ParseTuple(tuple, "s:test_s_code1", &value) < 0) - return NULL; + if (!PyArg_ParseTuple(tuple, "s:test_s_code1", &value)) { + return NULL; + } - if (PyArg_ParseTuple(tuple, "z:test_s_code2", &value) < 0) - return NULL; + if (!PyArg_ParseTuple(tuple, "z:test_s_code2", &value)) { + return NULL; + } Py_DECREF(tuple); Py_RETURN_NONE; @@ -1655,14 +1661,16 @@ test_u_code(PyObject *self) PyTuple_SET_ITEM(tuple, 0, obj); value = 0; - if (PyArg_ParseTuple(tuple, "u:test_u_code", &value) < 0) + if (!PyArg_ParseTuple(tuple, "u:test_u_code", &value)) { return NULL; + } if (value != PyUnicode_AS_UNICODE(obj)) return raiseTestError("test_u_code", "u code returned wrong value for u'test'"); value = 0; - if (PyArg_ParseTuple(tuple, "u#:test_u_code", &value, &len) < 0) + if (!PyArg_ParseTuple(tuple, "u#:test_u_code", &value, &len)) { return NULL; + } if (value != PyUnicode_AS_UNICODE(obj) || len != PyUnicode_GET_SIZE(obj)) return raiseTestError("test_u_code", @@ -1694,8 +1702,9 @@ test_Z_code(PyObject *self) value2 = PyUnicode_AS_UNICODE(obj); /* Test Z for both values */ - if (PyArg_ParseTuple(tuple, "ZZ:test_Z_code", &value1, &value2) < 0) + if (!PyArg_ParseTuple(tuple, "ZZ:test_Z_code", &value1, &value2)) { return NULL; + } if (value1 != PyUnicode_AS_UNICODE(obj)) return raiseTestError("test_Z_code", "Z code returned wrong value for 'test'"); @@ -1709,9 +1718,11 @@ test_Z_code(PyObject *self) len2 = -1; /* Test Z# for both values */ - if (PyArg_ParseTuple(tuple, "Z#Z#:test_Z_code", &value1, &len1, - &value2, &len2) < 0) + if (!PyArg_ParseTuple(tuple, "Z#Z#:test_Z_code", &value1, &len1, + &value2, &len2)) + { return NULL; + } if (value1 != PyUnicode_AS_UNICODE(obj) || len1 != PyUnicode_GET_SIZE(obj)) return raiseTestError("test_Z_code", @@ -2033,8 +2044,9 @@ test_empty_argparse(PyObject *self) tuple = PyTuple_New(0); if (!tuple) return NULL; - if ((result = PyArg_ParseTuple(tuple, "|:test_empty_argparse")) < 0) + if (!(result = PyArg_ParseTuple(tuple, "|:test_empty_argparse"))) { goto done; + } dict = PyDict_New(); if (!dict) goto done; @@ -2042,8 +2054,9 @@ test_empty_argparse(PyObject *self) done: Py_DECREF(tuple); Py_XDECREF(dict); - if (result < 0) + if (!result) { return NULL; + } else { Py_RETURN_NONE; } @@ -3698,8 +3711,9 @@ test_raise_signal(PyObject* self, PyObject *args) { int signum, err; - if (PyArg_ParseTuple(args, "i:raise_signal", &signum) < 0) + if (!PyArg_ParseTuple(args, "i:raise_signal", &signum)) { return NULL; + } err = raise(signum); if (err) From d5575f613272d26821d93e1843903d855eda083f Mon Sep 17 00:00:00 2001 From: Oren Milman Date: Mon, 21 Aug 2017 21:38:24 +0300 Subject: [PATCH 2/6] improve a comment, and remove parenthese from class definition --- Lib/test/test_io.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/Lib/test/test_io.py b/Lib/test/test_io.py index 81d97f5520a0c9..ed10eee43350c4 100644 --- a/Lib/test/test_io.py +++ b/Lib/test/test_io.py @@ -3245,13 +3245,14 @@ def _make_illegal_wrapper(): t = _make_illegal_wrapper() self.assertRaises(TypeError, t.read) - # Issue 31243: The interpreter shouldn't crash or raise a SystemError - # in case the return value of decoder's getstate() is invalid. + # Issue 31243: calling read() while the return value of decoder's + # getstate() is invalid should neither crash the interpreter nor + # raise a SystemError. def _make_very_illegal_wrapper(getstate_ret_val): - class BadDecoder(): + class BadDecoder: def getstate(self): return getstate_ret_val - class BadIncrementalDecoder(): + class BadIncrementalDecoder: def __call__(self, dummy): return BadDecoder() quopri = codecs.lookup("quopri") From 99e8405ac2756f59e514a9557c5a3577f18d6b9a Mon Sep 17 00:00:00 2001 From: Oren Milman Date: Thu, 24 Aug 2017 11:50:11 +0300 Subject: [PATCH 3/6] shorten the error messages and improve the test code. --- Lib/test/test_io.py | 15 +++++---------- Modules/_io/textio.c | 17 +++++++---------- 2 files changed, 12 insertions(+), 20 deletions(-) diff --git a/Lib/test/test_io.py b/Lib/test/test_io.py index ed10eee43350c4..79d49ce3734659 100644 --- a/Lib/test/test_io.py +++ b/Lib/test/test_io.py @@ -3252,17 +3252,12 @@ def _make_very_illegal_wrapper(getstate_ret_val): class BadDecoder: def getstate(self): return getstate_ret_val - class BadIncrementalDecoder: - def __call__(self, dummy): - return BadDecoder() + def _get_bad_decoder(dummy): + return BadDecoder() quopri = codecs.lookup("quopri") - quopri_decoder = quopri.incrementaldecoder - quopri.incrementaldecoder = BadIncrementalDecoder() - try: - t = _make_illegal_wrapper() - finally: - quopri.incrementaldecoder = quopri_decoder - return t + with support.swap_attr(quopri, 'incrementaldecoder', + _get_bad_decoder): + return _make_illegal_wrapper() t = _make_very_illegal_wrapper(42) self.assertRaises(TypeError, t.read, 42) t = _make_very_illegal_wrapper(()) diff --git a/Modules/_io/textio.c b/Modules/_io/textio.c index 215d2c86eb708c..1542163bb8d841 100644 --- a/Modules/_io/textio.c +++ b/Modules/_io/textio.c @@ -1499,16 +1499,13 @@ textiowrapper_read_chunk(textio *self, Py_ssize_t size_hint) * len(dec_buffer) bytes ago with decoder state (b'', dec_flags). */ if (!PyTuple_Check(state)) { - PyErr_Format(PyExc_TypeError, - "decoder getstate() should have returned a tuple, " - "not '%.200s'", - Py_TYPE(state)->tp_name); + PyErr_SetString(PyExc_TypeError, + "illegal decoder state"); Py_DECREF(state); return -1; } if (!PyArg_ParseTuple(state, - "OO;decoder getstate() should have returned a " - "2-tuple", &dec_buffer, &dec_flags)) + "OO;illegal decoder state", &dec_buffer, &dec_flags)) { Py_DECREF(state); return -1; @@ -1516,8 +1513,8 @@ textiowrapper_read_chunk(textio *self, Py_ssize_t size_hint) if (!PyBytes_Check(dec_buffer)) { PyErr_Format(PyExc_TypeError, - "decoder getstate() should have returned a tuple " - "whose first item is a bytes object, not '%.200s'", + "illegal decoder state: the first item should be a " + "bytes object, not '%.200s'", Py_TYPE(dec_buffer)->tp_name); Py_DECREF(state); return -1; @@ -2395,8 +2392,8 @@ _io_TextIOWrapper_tell_impl(textio *self) } \ if (!PyBytes_Check(dec_buffer)) { \ PyErr_Format(PyExc_TypeError, \ - "decoder getstate() should have returned a bytes " \ - "object, not '%.200s'", \ + "illegal decoder state: the first item should be a " \ + "bytes object, not '%.200s'", \ Py_TYPE(dec_buffer)->tp_name); \ Py_DECREF(_state); \ goto fail; \ From c8a21bade24e4361261f9ddb780319ba4c351d3f Mon Sep 17 00:00:00 2001 From: Oren Milman Date: Thu, 24 Aug 2017 13:35:14 +0300 Subject: [PATCH 4/6] added NEWS.d --- .../Core and Builtins/2017-08-24-13-34-49.bpo-31243.dRJzqR.rst | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2017-08-24-13-34-49.bpo-31243.dRJzqR.rst diff --git a/Misc/NEWS.d/next/Core and Builtins/2017-08-24-13-34-49.bpo-31243.dRJzqR.rst b/Misc/NEWS.d/next/Core and Builtins/2017-08-24-13-34-49.bpo-31243.dRJzqR.rst new file mode 100644 index 00000000000000..0b15e1c3e712ee --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2017-08-24-13-34-49.bpo-31243.dRJzqR.rst @@ -0,0 +1,2 @@ +Fix a crash in some methods of `io.TextIOWrapper`, when the decoder's state +is not a tuple. Patch by Oren Milman and Serhiy Storchaka. From a3a121255613d8be3b5352d40b2613cb13de1455 Mon Sep 17 00:00:00 2001 From: Oren Milman Date: Thu, 24 Aug 2017 14:12:26 +0300 Subject: [PATCH 5/6] fix NEWS.d item --- .../Core and Builtins/2017-08-24-13-34-49.bpo-31243.dRJzqR.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Core and Builtins/2017-08-24-13-34-49.bpo-31243.dRJzqR.rst b/Misc/NEWS.d/next/Core and Builtins/2017-08-24-13-34-49.bpo-31243.dRJzqR.rst index 0b15e1c3e712ee..f34ca2d2ff9cc0 100644 --- a/Misc/NEWS.d/next/Core and Builtins/2017-08-24-13-34-49.bpo-31243.dRJzqR.rst +++ b/Misc/NEWS.d/next/Core and Builtins/2017-08-24-13-34-49.bpo-31243.dRJzqR.rst @@ -1,2 +1,2 @@ Fix a crash in some methods of `io.TextIOWrapper`, when the decoder's state -is not a tuple. Patch by Oren Milman and Serhiy Storchaka. +is invalid. Patch by Oren Milman and Serhiy Storchaka. From f7f38c745d72f8240df29c44acb894a117101941 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Mon, 28 Aug 2017 11:11:09 +0300 Subject: [PATCH 6/6] No need to mention my name specially. --- .../Core and Builtins/2017-08-24-13-34-49.bpo-31243.dRJzqR.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Core and Builtins/2017-08-24-13-34-49.bpo-31243.dRJzqR.rst b/Misc/NEWS.d/next/Core and Builtins/2017-08-24-13-34-49.bpo-31243.dRJzqR.rst index f34ca2d2ff9cc0..166458f2b785ab 100644 --- a/Misc/NEWS.d/next/Core and Builtins/2017-08-24-13-34-49.bpo-31243.dRJzqR.rst +++ b/Misc/NEWS.d/next/Core and Builtins/2017-08-24-13-34-49.bpo-31243.dRJzqR.rst @@ -1,2 +1,2 @@ Fix a crash in some methods of `io.TextIOWrapper`, when the decoder's state -is invalid. Patch by Oren Milman and Serhiy Storchaka. +is invalid. Patch by Oren Milman.