Return value of <ExceptionGroup class>.split has insufficient checks leading to a type confusion bug
Closed this issue · 18 comments
Crash report
What happened?
The following code has checks to make sure the return value is a tuple and of size 2, but only in asserts which means that these checks wont happen on a non-debug build.
Lines 2093 to 2101 in b92f101
So you can create an ExceptionGroup subclass with a custom split function that doesnt return a tuple, and it will try to interpret that object as a tuple.
PoC
class Evil(BaseExceptionGroup):
def split(self, *args):
return "NOT A TUPLE!"
print("Running...")
try:
raise Evil("wow!", [Exception()])
except* Exception:
pass
print("program should crash before reaching this")Output
Running...
Segmentation fault (core dumped)
CPython versions tested on:
3.11, 3.12, 3.13
Operating systems tested on:
Linux, Windows
Output from running 'python -VV' on the command line:
No response
Linked PRs
Nice find! Raising a TypeError seems more appropriate here, would you like to send a PR?
Sure
I agree we should do something here, but raising a new exception while the interpreter is handling an exception from the program is not ideal if it swallows the user's exception.
So we need to think that through.
With the change I implemented, the new output is
Running...
+ Exception Group Traceback (most recent call last):
| File "/tmp/test.py", line 7, in <module>
| raise Evil("wow!", [Exception()])
| Evil: wow! (1 sub-exception)
+-+---------------- 1 ----------------
| Exception
+------------------------------------
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/tmp/test.py", line 8, in <module>
except* Exception:
pass
TypeError: Evil.split must return a 2-tuple
Could you update the test to check that the original exception is chained?
hmm, if I update the test to the following, then it final print statement executes
class Evil(BaseExceptionGroup):
def split(self, *args):
return "NOT A TUPLE!"
print("Running...")
try:
try:
raise Evil("wow!", [Exception()])
except* Exception:
pass
except TypeError:
pass
print("done")But if the original exception is dependent on the ExceptionGroup split function executing correctly, how else can we go about it?
hmm, if I update the test to the following, then it final print statement executes
On your branch I would expect it to work. except *Exception will catch a TypeError.
But if the original exception is dependent on the ExceptionGroup split function executing correctly, how else can we go about it?
PyErr_FormatUnraisable is also an option.
To clarify, the alternative suggestion is to treat anything other than a 2-tuple as a no-match, and raise a TypeError via PyErr_FormatUnraisable.
Though, chaining would be consistent with this:
>>> def t(): return ValueError
...
>>> try:
... raise ValueError(42)
... except t() as e:
... print(e)
...
42
>>>
>>> def t(): raise TypeError
...
>>> try:
... raise ValueError(42)
... except t() as e:
... print(e)
...
Traceback (most recent call last):
File "<python-input-12>", line 2, in <module>
raise ValueError(42)
ValueError: 42
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "<python-input-12>", line 3, in <module>
except t() as e:
~^^
File "<python-input-11>", line 1, in t
def t(): raise TypeError
^^^^^^^^^^^^^^^
TypeError
>>>
For the test, you need to use a different type so that you're not catching everything. Something like this:
class Evil(BaseExceptionGroup):
def split(self, *args):
return "NOT A TUPLE!"
print("Running...")
try:
try:
raise Evil("wow!", [OSError(), ValueError()])
except* ValueError:
pass
except* OSError:
pass
except TypeError as e:
assert that e is what we expect
assert that e.__context__ is the raised exception with `ValueErrors` split out
print("done")
I've never worked with FormatUnraisable before, is this the correct way to use it?
/* ... */
if (_PyBaseExceptionGroup_Check(exc_value)) {
PyObject *pair = PyObject_CallMethod(exc_value, "split", "(O)",
match_type);
if (pair == NULL) {
return -1;
}
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);
PyErr_FormatUnraisable("Exception ignored on calling %s.split",
Py_TYPE(exc_value)->tp_name);
Py_DECREF(pair);
goto no_match;
}
*match = Py_NewRef(PyTuple_GET_ITEM(pair, 0));
*rest = Py_NewRef(PyTuple_GET_ITEM(pair, 1));
Py_DECREF(pair);
return 0;
}
no_match:
*match = Py_NewRef(Py_None);
*rest = Py_NewRef(exc_value);
return 0;
}If I don't put the PyErr_Format before it, I get an assertion failure of
python.exe: Python/errors.c:1600: format_unraisable_v: Assertion `exc_type != NULL' failed.
With the changes above, I get this output
Running...
Exception ignored on calling Evil.split:
Traceback (most recent call last):
File "/tmp/test.py", line 9, in <module>
except* Exception:
TypeError: Evil.split must return a 2-tuple
+ Exception Group Traceback (most recent call last):
| File "/tmp/test.py", line 8, in <module>
| raise Evil("wow!", [Exception()])
| ExceptionGroup: wow! (1 sub-exception)
+-+---------------- 1 ----------------
| Exception
+------------------------------------
I've never worked with FormatUnraisable before, is this the correct way to use it?
That looks right. But see my previous comment - I think it will be consistent with the other case to go with your first suggestion.
So just keep it how it is in the PR then?
Yes, but extend the test.
This is what I have right now for the new test case, would this be sufficient?
class Evil(BaseExceptionGroup):
def split(self, *args):
return "NOT A 2-TUPLE!"
with self.assertRaisesRegex(TypeError, r"split must return a 2-tuple") as m:
try:
raise Evil("wow!", [OSError(), ValueError()])
except* ValueError:
pass
except* OSError:
pass
self.assertIs(type(m.exception.__context__), Evil)This test belong in test_except_star.py rather than test_exception_group.py. Once you move it there you can use assertExceptionIsLike for the context exception.
Also, a test for the case where split returns a tuple of more than 2 (this currently works so it should continue working).
So should the code be updated to check if the tuple size < 2 instead of != 2? You answered that in your review
Thanks, this has been merged and backported. If you want, you can make another PR, which we won't backport, to deprecate tuples of lentgh more than 2 (issue a deprecation warning).