From 1adba3193a188c35a6765aef9e816f785eab0651 Mon Sep 17 00:00:00 2001 From: Zane Bitter Date: Fri, 13 Oct 2017 16:47:13 -0400 Subject: [PATCH 1/3] bpo-28603: Fix tracebacks for unhashable exceptions In the traceback module, TracebackException checks for loops between exceptions to prevent an infinite traceback. It does this by putting the already-seen exception into a set. This means that unhashable exception objects will cause an error - an error that itself can likely not be printed because of the presence of the unhashable exception in the chain. In this case, we don't actually care about equality of the objects as defined by the class designer; we want to check that we don't encounter the self-same exception object, from a chain that is necessarily all in memory at the same time. We can trivially do so by comparing identities instead of equality. --- Lib/test/test_traceback.py | 19 +++++++++++++++++++ Lib/traceback.py | 6 +++--- Misc/ACKS | 1 + .../2017-10-17-12-29-18.bpo-28603.tGuX2C.rst | 3 +++ 4 files changed, 26 insertions(+), 3 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2017-10-17-12-29-18.bpo-28603.tGuX2C.rst diff --git a/Lib/test/test_traceback.py b/Lib/test/test_traceback.py index e4833535890d15..51e21b88cdcac8 100644 --- a/Lib/test/test_traceback.py +++ b/Lib/test/test_traceback.py @@ -994,6 +994,25 @@ def test_context(self): self.assertEqual(exc_info[0], exc.exc_type) self.assertEqual(str(exc_info[1]), str(exc)) + def test_unhashable(self): + class UnhashableException(Exception): + def __eq__(self, other): + return True + + ex1 = UnhashableException('ex1') + ex2 = UnhashableException('ex2') + try: + raise ex2 from ex1 + except UnhashableException: + try: + raise ex1 + except UnhashableException: + exc_info = sys.exc_info() + exc = traceback.TracebackException(*exc_info) + formatted = list(exc.format()) + self.assertIn('UnhashableException: ex2\n', formatted[2]) + self.assertIn('UnhashableException: ex1\n', formatted[6]) + def test_limit(self): def recurse(n): if n: diff --git a/Lib/traceback.py b/Lib/traceback.py index fb3bce12a131f7..ee8c73914032e1 100644 --- a/Lib/traceback.py +++ b/Lib/traceback.py @@ -458,11 +458,11 @@ def __init__(self, exc_type, exc_value, exc_traceback, *, limit=None, # Handle loops in __cause__ or __context__. if _seen is None: _seen = set() - _seen.add(exc_value) + _seen.add(id(exc_value)) # Gracefully handle (the way Python 2.4 and earlier did) the case of # being called with no type or value (None, None, None). if (exc_value and exc_value.__cause__ is not None - and exc_value.__cause__ not in _seen): + and id(exc_value.__cause__) not in _seen): cause = TracebackException( type(exc_value.__cause__), exc_value.__cause__, @@ -474,7 +474,7 @@ def __init__(self, exc_type, exc_value, exc_traceback, *, limit=None, else: cause = None if (exc_value and exc_value.__context__ is not None - and exc_value.__context__ not in _seen): + and id(exc_value.__context__) not in _seen): context = TracebackException( type(exc_value.__context__), exc_value.__context__, diff --git a/Misc/ACKS b/Misc/ACKS index 38f68aca214611..0c80080ee99f2f 100644 --- a/Misc/ACKS +++ b/Misc/ACKS @@ -147,6 +147,7 @@ Dominic Binks Philippe Biondi Michael Birtwell Stuart Bishop +Zane Bitter Roy Bixler Daniel Black Jonathan Black diff --git a/Misc/NEWS.d/next/Library/2017-10-17-12-29-18.bpo-28603.tGuX2C.rst b/Misc/NEWS.d/next/Library/2017-10-17-12-29-18.bpo-28603.tGuX2C.rst new file mode 100644 index 00000000000000..1a5fb0aa67b46c --- /dev/null +++ b/Misc/NEWS.d/next/Library/2017-10-17-12-29-18.bpo-28603.tGuX2C.rst @@ -0,0 +1,3 @@ +traceback: Fix a TypeError that occurred during printing of exception +tracebacks when either the current exception or an exception in its +context/cause chain is unhashable. Patch by Zane Bitter. From c4977dad155f27cb9e37bae168db3c924ea0b1b8 Mon Sep 17 00:00:00 2001 From: Zane Bitter Date: Tue, 17 Oct 2017 11:24:45 -0400 Subject: [PATCH 2/3] bpo-28603 Fix unhashable exception handling in IDLE IDLE effectively has its own implementation of traceback.print_exception() to allow it to clean up the tracebacks. Unhashable exceptions caused a TypeError in the exception printer, which caused the shell to restart. This change eliminates the problem by comparing exceptions by identity rather than equality, analagous to how the traceback module now does it. --- Lib/idlelib/idle_test/test_run.py | 35 +++++++++++++++++++ Lib/idlelib/run.py | 6 ++-- .../2017-10-17-13-26-13.bpo-28603.TMEQfp.rst | 2 ++ 3 files changed, 40 insertions(+), 3 deletions(-) create mode 100644 Lib/idlelib/idle_test/test_run.py create mode 100644 Misc/NEWS.d/next/IDLE/2017-10-17-13-26-13.bpo-28603.TMEQfp.rst diff --git a/Lib/idlelib/idle_test/test_run.py b/Lib/idlelib/idle_test/test_run.py new file mode 100644 index 00000000000000..d7e627d23d3841 --- /dev/null +++ b/Lib/idlelib/idle_test/test_run.py @@ -0,0 +1,35 @@ +import unittest +from unittest import mock + +from test.support import captured_stderr +import idlelib.run as idlerun + + +class RunTest(unittest.TestCase): + def test_print_exception_unhashable(self): + class UnhashableException(Exception): + def __eq__(self, other): + return True + + ex1 = UnhashableException('ex1') + ex2 = UnhashableException('ex2') + try: + raise ex2 from ex1 + except UnhashableException: + try: + raise ex1 + except UnhashableException: + with captured_stderr() as output: + with mock.patch.object(idlerun, + 'cleanup_traceback') as ct: + ct.side_effect = lambda t, e: t + idlerun.print_exception() + + tb = output.getvalue().strip().splitlines() + self.assertEqual(11, len(tb)) + self.assertIn('UnhashableException: ex2', tb[3]) + self.assertIn('UnhashableException: ex1', tb[10]) + + +if __name__ == '__main__': + unittest.main(verbosity=2) diff --git a/Lib/idlelib/run.py b/Lib/idlelib/run.py index 39e0c116f9bc41..6440e673bf5163 100644 --- a/Lib/idlelib/run.py +++ b/Lib/idlelib/run.py @@ -203,16 +203,16 @@ def print_exception(): seen = set() def print_exc(typ, exc, tb): - seen.add(exc) + seen.add(id(exc)) context = exc.__context__ cause = exc.__cause__ - if cause is not None and cause not in seen: + if cause is not None and id(cause) not in seen: print_exc(type(cause), cause, cause.__traceback__) print("\nThe above exception was the direct cause " "of the following exception:\n", file=efile) elif (context is not None and not exc.__suppress_context__ and - context not in seen): + id(context) not in seen): print_exc(type(context), context, context.__traceback__) print("\nDuring handling of the above exception, " "another exception occurred:\n", file=efile) diff --git a/Misc/NEWS.d/next/IDLE/2017-10-17-13-26-13.bpo-28603.TMEQfp.rst b/Misc/NEWS.d/next/IDLE/2017-10-17-13-26-13.bpo-28603.TMEQfp.rst new file mode 100644 index 00000000000000..e69f7f67219020 --- /dev/null +++ b/Misc/NEWS.d/next/IDLE/2017-10-17-13-26-13.bpo-28603.TMEQfp.rst @@ -0,0 +1,2 @@ +Fix a TypeError that caused a shell restart when printing a traceback that +includes an exception that is unhashable. Patch by Zane Bitter. From 9e78d8c51cea9e4131ad29cd8bf604df1f1334a0 Mon Sep 17 00:00:00 2001 From: Zane Bitter Date: Fri, 13 Oct 2017 17:58:52 -0400 Subject: [PATCH 3/3] bpo-28603 Always print chained exceptions in PyErr_Display() Previously when printing an exception traceback in PyErr_Display(), it would stop traversing the context/cause chain when it encountered an exception that was either unhashable or compared equal to one already seen in the chain. With this change, we only stop traversing the chain when we encounter the same exception object again (indicating a loop.) --- Lib/test/test_traceback.py | 27 +++++++++++++++++++ .../2017-10-17-13-29-19.bpo-28603._-oia3.rst | 3 +++ Python/pythonrun.c | 21 ++++++++++++--- 3 files changed, 48 insertions(+), 3 deletions(-) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2017-10-17-13-29-19.bpo-28603._-oia3.rst diff --git a/Lib/test/test_traceback.py b/Lib/test/test_traceback.py index 51e21b88cdcac8..bffc03e663ff78 100644 --- a/Lib/test/test_traceback.py +++ b/Lib/test/test_traceback.py @@ -443,6 +443,33 @@ def fmt(): ' return traceback.format_stack()\n' % (__file__, lineno+1), ]) + @cpython_only + def test_unhashable(self): + from _testcapi import exception_print + + class UnhashableException(Exception): + def __eq__(self, other): + return True + + ex1 = UnhashableException('ex1') + ex2 = UnhashableException('ex2') + try: + raise ex2 from ex1 + except UnhashableException: + try: + raise ex1 + except UnhashableException: + exc_type, exc_val, exc_tb = sys.exc_info() + + with captured_output("stderr") as stderr_f: + exception_print(exc_val) + + tb = stderr_f.getvalue().strip().splitlines() + self.assertEqual(11, len(tb)) + self.assertEqual(context_message.strip(), tb[5]) + self.assertIn('UnhashableException: ex2', tb[3]) + self.assertIn('UnhashableException: ex1', tb[10]) + cause_message = ( "\nThe above exception was the direct cause " diff --git a/Misc/NEWS.d/next/Core and Builtins/2017-10-17-13-29-19.bpo-28603._-oia3.rst b/Misc/NEWS.d/next/Core and Builtins/2017-10-17-13-29-19.bpo-28603._-oia3.rst new file mode 100644 index 00000000000000..a3542accd5f7bb --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2017-10-17-13-29-19.bpo-28603._-oia3.rst @@ -0,0 +1,3 @@ +Print the full context/cause chain of exceptions on interpreter exit, even +if an exception in the chain is unhashable or compares equal to later ones. +Patch by Zane Bitter. diff --git a/Python/pythonrun.c b/Python/pythonrun.c index df814fbed97b30..25e2da44d95343 100644 --- a/Python/pythonrun.c +++ b/Python/pythonrun.c @@ -817,13 +817,21 @@ print_exception_recursive(PyObject *f, PyObject *value, PyObject *seen) if (seen != NULL) { /* Exception chaining */ - if (PySet_Add(seen, value) == -1) + PyObject *value_id = PyLong_FromVoidPtr(value); + if (value_id == NULL || PySet_Add(seen, value_id) == -1) PyErr_Clear(); else if (PyExceptionInstance_Check(value)) { + PyObject *check_id = NULL; cause = PyException_GetCause(value); context = PyException_GetContext(value); if (cause) { - res = PySet_Contains(seen, cause); + check_id = PyLong_FromVoidPtr(cause); + if (check_id == NULL) { + res = -1; + } else { + res = PySet_Contains(seen, check_id); + Py_DECREF(check_id); + } if (res == -1) PyErr_Clear(); if (res == 0) { @@ -835,7 +843,13 @@ print_exception_recursive(PyObject *f, PyObject *value, PyObject *seen) } else if (context && !((PyBaseExceptionObject *)value)->suppress_context) { - res = PySet_Contains(seen, context); + check_id = PyLong_FromVoidPtr(context); + if (check_id == NULL) { + res = -1; + } else { + res = PySet_Contains(seen, check_id); + Py_DECREF(check_id); + } if (res == -1) PyErr_Clear(); if (res == 0) { @@ -848,6 +862,7 @@ print_exception_recursive(PyObject *f, PyObject *value, PyObject *seen) Py_XDECREF(context); Py_XDECREF(cause); } + Py_XDECREF(value_id); } print_exception(f, value); if (err != 0)