From fb869d428fb78b16c8ccda630a8bff5c59afb60d Mon Sep 17 00:00:00 2001 From: Nico-Posada Date: Wed, 18 Dec 2024 14:04:23 -0500 Subject: [PATCH 1/6] have checks for the return value of split be done outside of asserts --- Lib/test/test_exception_group.py | 14 ++++++++++++++ Python/ceval.c | 11 +++++++++-- 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_exception_group.py b/Lib/test/test_exception_group.py index 53212529c27e28..76faf4cd33663f 100644 --- a/Lib/test/test_exception_group.py +++ b/Lib/test/test_exception_group.py @@ -576,6 +576,20 @@ def test_iteration_full_tracebacks(self): expected_tbs[i]) +class CustomExceptionGroupSplitTest(ExceptionGroupTestBase): + # See https://github.com/python/cpython/issues/128049 + def test_invalid_split_return_value(self): + class Evil(BaseExceptionGroup): + def split(self, types): + return "NOT A TUPLE" + + with self.assertRaises(TypeError): + try: + raise Evil("message here", [Exception()]) + except* Exception: + pass + + class ExceptionGroupSplitTestBase(ExceptionGroupTestBase): def split_exception_group(self, eg, types): diff --git a/Python/ceval.c b/Python/ceval.c index fd891d7839151e..3646deec32f66a 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -2095,8 +2095,15 @@ _PyEval_ExceptionGroupMatch(PyObject* exc_value, PyObject *match_type, if (pair == NULL) { return -1; } - assert(PyTuple_CheckExact(pair)); - assert(PyTuple_GET_SIZE(pair) == 2); + + if (!PyTuple_CheckExact(pair) || PyTuple_Size(pair) != 2) { + PyErr_Format(PyExc_TypeError, + "%s.split must return a 2-tuple", + Py_TYPE(exc_value)->tp_name); + Py_DECREF(pair); + return -1; + } + *match = Py_NewRef(PyTuple_GET_ITEM(pair, 0)); *rest = Py_NewRef(PyTuple_GET_ITEM(pair, 1)); Py_DECREF(pair); From 7a482a40e706f90ebe02e02f5ca597d85fdb7c2a Mon Sep 17 00:00:00 2001 From: Nico-Posada Date: Wed, 18 Dec 2024 14:27:06 -0500 Subject: [PATCH 2/6] added NEWS entry and fixed linter error --- Lib/test/test_exception_group.py | 2 +- .../2024-12-18-14-22-48.gh-issue-128079.SUD5le.rst | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) create mode 100644 Misc/NEWS.d/next/Core_and_Builtins/2024-12-18-14-22-48.gh-issue-128079.SUD5le.rst diff --git a/Lib/test/test_exception_group.py b/Lib/test/test_exception_group.py index 76faf4cd33663f..9bdce9fccd365a 100644 --- a/Lib/test/test_exception_group.py +++ b/Lib/test/test_exception_group.py @@ -582,7 +582,7 @@ def test_invalid_split_return_value(self): class Evil(BaseExceptionGroup): def split(self, types): return "NOT A TUPLE" - + with self.assertRaises(TypeError): try: raise Evil("message here", [Exception()]) diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2024-12-18-14-22-48.gh-issue-128079.SUD5le.rst b/Misc/NEWS.d/next/Core_and_Builtins/2024-12-18-14-22-48.gh-issue-128079.SUD5le.rst new file mode 100644 index 00000000000000..342bda97f35486 --- /dev/null +++ b/Misc/NEWS.d/next/Core_and_Builtins/2024-12-18-14-22-48.gh-issue-128079.SUD5le.rst @@ -0,0 +1,3 @@ +Fixed a type confusion bug where the caller of an :class:`ExceptionGroup` +object's :meth:`split` function had improper checks implemented leading to +any return value being interpreted as a tuple. From 723a6a637ae5b102bc41ca2598235948b7a75780 Mon Sep 17 00:00:00 2001 From: Nico-Posada Date: Wed, 18 Dec 2024 14:40:42 -0500 Subject: [PATCH 3/6] Fix rst error --- .../2024-12-18-14-22-48.gh-issue-128079.SUD5le.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2024-12-18-14-22-48.gh-issue-128079.SUD5le.rst b/Misc/NEWS.d/next/Core_and_Builtins/2024-12-18-14-22-48.gh-issue-128079.SUD5le.rst index 342bda97f35486..da01ac5ad637c6 100644 --- a/Misc/NEWS.d/next/Core_and_Builtins/2024-12-18-14-22-48.gh-issue-128079.SUD5le.rst +++ b/Misc/NEWS.d/next/Core_and_Builtins/2024-12-18-14-22-48.gh-issue-128079.SUD5le.rst @@ -1,3 +1,3 @@ Fixed a type confusion bug where the caller of an :class:`ExceptionGroup` -object's :meth:`split` function had improper checks implemented leading to -any return value being interpreted as a tuple. +object's :meth:`~BaseExceptionGroup.split` function had improper checks +implemented leading to any return value being interpreted as a tuple. From a360e60db22dadea9fa9fb779c81a0693ff04f64 Mon Sep 17 00:00:00 2001 From: Nico-Posada Date: Fri, 20 Dec 2024 09:44:31 -0500 Subject: [PATCH 4/6] code review changes --- Lib/test/test_except_star.py | 47 +++++++++++++++++++ Lib/test/test_exception_group.py | 14 ------ ...-12-18-14-22-48.gh-issue-128079.SUD5le.rst | 8 ++-- Python/ceval.c | 19 ++++++-- 4 files changed, 68 insertions(+), 20 deletions(-) diff --git a/Lib/test/test_except_star.py b/Lib/test/test_except_star.py index c49c6008e08e8c..b7af9119a0a55d 100644 --- a/Lib/test/test_except_star.py +++ b/Lib/test/test_except_star.py @@ -951,6 +951,53 @@ def derive(self, excs): self.assertExceptionIsLike(exc, FalsyEG("eg", [TypeError(1)])) self.assertExceptionIsLike(tes, FalsyEG("eg", [TypeError(1)])) self.assertExceptionIsLike(ves, FalsyEG("eg", [ValueError(2)])) + + def test_bad_exception_group_subclass_split_func(self): + # See https://github.com/python/cpython/issues/128049 + # tuples that return less than 2 values should + # result in a type error with the original eg chained to it + class BadEG1(ExceptionGroup): + def split(self, *args): + return "NOT A 2-TUPLE!" + + class BadEG2(ExceptionGroup): + def split(self, *args): + return ("NOT A 2-TUPLE!",) + + eg_list = [ + (BadEG1("eg", [OSError(123), ValueError(456)]), + r"split must return a tuple, not str"), + (BadEG2("eg", [OSError(123), ValueError(456)]), + r"split must return a 2-tuple, got tuple of size 1") + ] + + for EG, MSG in eg_list: + with self.assertRaisesRegex(TypeError, MSG) as m: + try: + raise EG + except* ValueError: + pass + except* OSError: + pass + + self.assertExceptionIsLike(m.exception.__context__, EG) + + # although it isn't expected, still allow tuples of length > 2 + # all tuple items past the second one will be ignored + # this quirk may be deprecated in the future + class WeirdEG(ExceptionGroup): + def split(self, *args): + return super().split(*args) + ("anything", 123456, None) + + try: + raise WeirdEG("eg", [OSError(123), ValueError(456)]) + except* OSError as e: + oeg = e + except* ValueError as e: + veg = e + + self.assertExceptionIsLike(oeg, WeirdEG("eg", [OSError(123)])) + self.assertExceptionIsLike(veg, WeirdEG("eg", [ValueError(456)])) class TestExceptStarCleanup(ExceptStarTest): diff --git a/Lib/test/test_exception_group.py b/Lib/test/test_exception_group.py index 9bdce9fccd365a..53212529c27e28 100644 --- a/Lib/test/test_exception_group.py +++ b/Lib/test/test_exception_group.py @@ -576,20 +576,6 @@ def test_iteration_full_tracebacks(self): expected_tbs[i]) -class CustomExceptionGroupSplitTest(ExceptionGroupTestBase): - # See https://github.com/python/cpython/issues/128049 - def test_invalid_split_return_value(self): - class Evil(BaseExceptionGroup): - def split(self, types): - return "NOT A TUPLE" - - with self.assertRaises(TypeError): - try: - raise Evil("message here", [Exception()]) - except* Exception: - pass - - class ExceptionGroupSplitTestBase(ExceptionGroupTestBase): def split_exception_group(self, eg, types): diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2024-12-18-14-22-48.gh-issue-128079.SUD5le.rst b/Misc/NEWS.d/next/Core_and_Builtins/2024-12-18-14-22-48.gh-issue-128079.SUD5le.rst index da01ac5ad637c6..8da4e677f068a3 100644 --- a/Misc/NEWS.d/next/Core_and_Builtins/2024-12-18-14-22-48.gh-issue-128079.SUD5le.rst +++ b/Misc/NEWS.d/next/Core_and_Builtins/2024-12-18-14-22-48.gh-issue-128079.SUD5le.rst @@ -1,3 +1,5 @@ -Fixed a type confusion bug where the caller of an :class:`ExceptionGroup` -object's :meth:`~BaseExceptionGroup.split` function had improper checks -implemented leading to any return value being interpreted as a tuple. +Fix a bug where :keyword:`except* ` does not properly check the +return value of an :exc:`ExceptionGroup`'s :meth:`~BaseExceptionGroup.split` +function, leading to a crash in some cases. Now when :meth:`~BaseExceptionGroup.split` +returns an invalid object, :keyword:`except* ` raises a :exc:`TypeError` +with the original raised :exc:`ExceptionGroup` object chained to it. diff --git a/Python/ceval.c b/Python/ceval.c index 3646deec32f66a..dc51baed8de2f9 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -2096,10 +2096,23 @@ _PyEval_ExceptionGroupMatch(PyObject* exc_value, PyObject *match_type, return -1; } - if (!PyTuple_CheckExact(pair) || PyTuple_Size(pair) != 2) { + if (!PyTuple_CheckExact(pair)) + { + PyErr_Format(PyExc_TypeError, + "%.200s.split must return a tuple, not %.200s", + Py_TYPE(exc_value)->tp_name, Py_TYPE(pair)->tp_name); + Py_DECREF(pair); + return -1; + } + + // NOTE due to a previous bug which allowed tuples of length > 2 to + // work without problem, we are still allowing them to work even + // though the error says otherwise + if (PyTuple_GET_SIZE(pair) < 2) { PyErr_Format(PyExc_TypeError, - "%s.split must return a 2-tuple", - Py_TYPE(exc_value)->tp_name); + "%.200s.split must return a 2-tuple, " + "got tuple of size %zd", + Py_TYPE(exc_value)->tp_name, PyTuple_GET_SIZE(pair)); Py_DECREF(pair); return -1; } From 5444a91135aee49f55cc038e1a7129673fa5b442 Mon Sep 17 00:00:00 2001 From: Nico-Posada Date: Fri, 20 Dec 2024 09:47:20 -0500 Subject: [PATCH 5/6] Fix linter errors --- Lib/test/test_except_star.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/test/test_except_star.py b/Lib/test/test_except_star.py index b7af9119a0a55d..3737c3e93ae6e4 100644 --- a/Lib/test/test_except_star.py +++ b/Lib/test/test_except_star.py @@ -951,7 +951,7 @@ def derive(self, excs): self.assertExceptionIsLike(exc, FalsyEG("eg", [TypeError(1)])) self.assertExceptionIsLike(tes, FalsyEG("eg", [TypeError(1)])) self.assertExceptionIsLike(ves, FalsyEG("eg", [ValueError(2)])) - + def test_bad_exception_group_subclass_split_func(self): # See https://github.com/python/cpython/issues/128049 # tuples that return less than 2 values should From 05d809b2aa294c690e3cef5494aeb8c7f08ee6ec Mon Sep 17 00:00:00 2001 From: Nico-Posada Date: Fri, 20 Dec 2024 13:44:15 -0500 Subject: [PATCH 6/6] code review changes --- Lib/test/test_except_star.py | 18 +++++++----------- Python/ceval.c | 17 +++++++---------- 2 files changed, 14 insertions(+), 21 deletions(-) diff --git a/Lib/test/test_except_star.py b/Lib/test/test_except_star.py index 3737c3e93ae6e4..284907f61213f8 100644 --- a/Lib/test/test_except_star.py +++ b/Lib/test/test_except_star.py @@ -952,10 +952,8 @@ def derive(self, excs): self.assertExceptionIsLike(tes, FalsyEG("eg", [TypeError(1)])) self.assertExceptionIsLike(ves, FalsyEG("eg", [ValueError(2)])) - def test_bad_exception_group_subclass_split_func(self): - # See https://github.com/python/cpython/issues/128049 - # tuples that return less than 2 values should - # result in a type error with the original eg chained to it + def test_exception_group_subclass_with_bad_split_func(self): + # see gh-128049. class BadEG1(ExceptionGroup): def split(self, *args): return "NOT A 2-TUPLE!" @@ -971,20 +969,18 @@ def split(self, *args): r"split must return a 2-tuple, got tuple of size 1") ] - for EG, MSG in eg_list: - with self.assertRaisesRegex(TypeError, MSG) as m: + for eg_class, msg in eg_list: + with self.assertRaisesRegex(TypeError, msg) as m: try: - raise EG + raise eg_class except* ValueError: pass except* OSError: pass - self.assertExceptionIsLike(m.exception.__context__, EG) + self.assertExceptionIsLike(m.exception.__context__, eg_class) - # although it isn't expected, still allow tuples of length > 2 - # all tuple items past the second one will be ignored - # this quirk may be deprecated in the future + # we allow tuples of length > 2 for backwards compatibility class WeirdEG(ExceptionGroup): def split(self, *args): return super().split(*args) + ("anything", 123456, None) diff --git a/Python/ceval.c b/Python/ceval.c index dc51baed8de2f9..40ad30e280edb4 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -2096,23 +2096,20 @@ _PyEval_ExceptionGroupMatch(PyObject* exc_value, PyObject *match_type, return -1; } - if (!PyTuple_CheckExact(pair)) - { + if (!PyTuple_CheckExact(pair)) { PyErr_Format(PyExc_TypeError, - "%.200s.split must return a tuple, not %.200s", - Py_TYPE(exc_value)->tp_name, Py_TYPE(pair)->tp_name); + "%.200s.split must return a tuple, not %.200s", + Py_TYPE(exc_value)->tp_name, Py_TYPE(pair)->tp_name); Py_DECREF(pair); return -1; } - // NOTE due to a previous bug which allowed tuples of length > 2 to - // work without problem, we are still allowing them to work even - // though the error says otherwise + // allow tuples of length > 2 for backwards compatibility if (PyTuple_GET_SIZE(pair) < 2) { PyErr_Format(PyExc_TypeError, - "%.200s.split must return a 2-tuple, " - "got tuple of size %zd", - Py_TYPE(exc_value)->tp_name, PyTuple_GET_SIZE(pair)); + "%.200s.split must return a 2-tuple, " + "got tuple of size %zd", + Py_TYPE(exc_value)->tp_name, PyTuple_GET_SIZE(pair)); Py_DECREF(pair); return -1; }