From 9275583eb03d66fef007b6d06a09a4a96ef358d1 Mon Sep 17 00:00:00 2001 From: Irit Katriel Date: Thu, 18 Nov 2021 16:05:31 +0000 Subject: [PATCH 1/6] bpo-45711: make decisions based on exc_value instead of exc_type. Strengthen redundancy assertion for NULL/None. --- Include/internal/pycore_pyerrors.h | 2 + Python/ceval.c | 19 ++++++++-- Python/errors.c | 60 ++++++++++++++++++++++++++++-- Python/pystate.c | 6 +-- Python/sysmodule.c | 7 +--- 5 files changed, 76 insertions(+), 18 deletions(-) diff --git a/Include/internal/pycore_pyerrors.h b/Include/internal/pycore_pyerrors.h index 0f4d41c7e0bab8..14ea182f4f47aa 100644 --- a/Include/internal/pycore_pyerrors.h +++ b/Include/internal/pycore_pyerrors.h @@ -28,6 +28,8 @@ static inline void _PyErr_ClearExcState(_PyErr_StackItem *exc_state) Py_XDECREF(tb); } +PyAPI_FUNC(PyObject*) _PyErr_StackItemToExcInfoTuple( + _PyErr_StackItem *err_info); PyAPI_FUNC(void) _PyErr_Fetch( PyThreadState *tstate, diff --git a/Python/ceval.c b/Python/ceval.c index 2b7b31c0913261..7d02b42bc537e4 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -1100,7 +1100,7 @@ static void _assert_exception_type_is_redundant(PyObject* type, PyObject* val) { if (type == NULL || type == Py_None) { - assert(val == NULL || val == Py_None); + assert(val == type); } else { assert(PyExceptionInstance_Check(val)); @@ -3661,7 +3661,10 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr "inherit from BaseException is not " "allowed"; PyObject *right = POP(); - PyObject *left = TOP(); + ASSERT_EXC_TYPE_IS_REDUNDANT(TOP(), SECOND()); + PyObject *left = SECOND(); + assert(PyExceptionInstance_Check(left)); + if (PyTuple_Check(right)) { Py_ssize_t i, length; length = PyTuple_GET_SIZE(right); @@ -4138,7 +4141,13 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, InterpreterFrame *frame, int thr ASSERT_EXC_TYPE_IS_REDUNDANT(type, value); _PyErr_StackItem *exc_info = tstate->exc_info; SET_THIRD(exc_info->exc_traceback); - SET_SECOND(exc_info->exc_value); + if (exc_info->exc_value != NULL) { + SET_SECOND(exc_info->exc_value); + } + else { + Py_INCREF(Py_None); + SET_SECOND(Py_None); + } if (exc_info->exc_type != NULL) { SET_TOP(exc_info->exc_type); } @@ -5843,7 +5852,9 @@ do_raise(PyThreadState *tstate, PyObject *exc, PyObject *cause) type = exc_info->exc_type; value = exc_info->exc_value; tb = exc_info->exc_traceback; - if (Py_IsNone(type) || type == NULL) { + assert(((Py_IsNone(value) || value == NULL)) == + ((Py_IsNone(type) || type == NULL))); + if (Py_IsNone(value) || value == NULL) { _PyErr_SetString(tstate, PyExc_RuntimeError, "No active exception to reraise"); return 0; diff --git a/Python/errors.c b/Python/errors.c index 519f2d459effd6..c3c6576ddcfab8 100644 --- a/Python/errors.c +++ b/Python/errors.c @@ -79,11 +79,15 @@ _PyErr_StackItem * _PyErr_GetTopmostException(PyThreadState *tstate) { _PyErr_StackItem *exc_info = tstate->exc_info; - while ((exc_info->exc_type == NULL || exc_info->exc_type == Py_None) && + while ((exc_info->exc_value == NULL || exc_info->exc_value == Py_None) && exc_info->previous_item != NULL) { + assert(exc_info->exc_type == NULL || exc_info->exc_type == Py_None); exc_info = exc_info->previous_item; } + assert(exc_info == NULL || + exc_info->previous_item == NULL || + (exc_info->exc_type != NULL && exc_info->exc_type != Py_None)); return exc_info; } @@ -471,10 +475,25 @@ _PyErr_GetExcInfo(PyThreadState *tstate, PyObject **p_type, PyObject **p_value, PyObject **p_traceback) { _PyErr_StackItem *exc_info = _PyErr_GetTopmostException(tstate); - *p_type = exc_info->exc_type; + *p_value = exc_info->exc_value; *p_traceback = exc_info->exc_traceback; + assert(*p_value == NULL || + *p_value == Py_None || + PyExceptionInstance_Check(*p_value)); + + *p_type = (*p_value != NULL && PyExceptionInstance_Check(*p_value)) ? + PyExceptionInstance_Class(*p_value) : + Py_None; + + if (exc_info->exc_type == NULL || exc_info->exc_type == Py_None) { + assert(*p_type == Py_None); + } + else { + assert(*p_type == exc_info->exc_type); + } + Py_XINCREF(*p_type); Py_XINCREF(*p_value); Py_XINCREF(*p_traceback); @@ -507,6 +526,30 @@ PyErr_SetExcInfo(PyObject *p_type, PyObject *p_value, PyObject *p_traceback) Py_XDECREF(oldtraceback); } + +PyObject* +_PyErr_StackItemToExcInfoTuple(_PyErr_StackItem *err_info) +{ + PyObject *exc_value = err_info->exc_value; + if (exc_value == NULL) { + exc_value = Py_None; + } + + assert(exc_value == Py_None || PyExceptionInstance_Check(exc_value)); + + PyObject *exc_type = PyExceptionInstance_Check(exc_value) ? + PyExceptionInstance_Class(err_info->exc_value) : + Py_None; + + return Py_BuildValue( + "(OOO)", + exc_type, + exc_value, + err_info->exc_traceback != NULL ? + err_info->exc_traceback : Py_None); +} + + /* Like PyErr_Restore(), but if an exception is already set, set the context associated with it. @@ -567,7 +610,11 @@ _PyErr_ChainStackItem(_PyErr_StackItem *exc_info) } else { exc_info_given = 1; } - if (exc_info->exc_type == NULL || exc_info->exc_type == Py_None) { + + assert( (exc_info->exc_type == NULL || exc_info->exc_type == Py_None) == + (exc_info->exc_value == NULL || exc_info->exc_value == Py_None) ); + + if (exc_info->exc_value == NULL || exc_info->exc_value == Py_None) { return; } @@ -586,7 +633,14 @@ _PyErr_ChainStackItem(_PyErr_StackItem *exc_info) exc2 = exc_info->exc_type; val2 = exc_info->exc_value; tb2 = exc_info->exc_traceback; + PyObject *exc2_before = exc2; + PyObject *val2_before = val2; + PyObject *tb2_before = tb2; _PyErr_NormalizeException(tstate, &exc2, &val2, &tb2); + /* exc_info should already be normalized */ + assert(exc2 == exc2_before); + assert(val2 == val2_before); + assert(tb2 == tb2_before); if (tb2 != NULL) { PyException_SetTraceback(val2, tb2); } diff --git a/Python/pystate.c b/Python/pystate.c index 273982b4bd2f56..c42dd88506bbe3 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -1341,11 +1341,7 @@ _PyThread_CurrentExceptions(void) if (id == NULL) { goto fail; } - PyObject *exc_info = PyTuple_Pack( - 3, - err_info->exc_type != NULL ? err_info->exc_type : Py_None, - err_info->exc_value != NULL ? err_info->exc_value : Py_None, - err_info->exc_traceback != NULL ? err_info->exc_traceback : Py_None); + PyObject *exc_info = _PyErr_StackItemToExcInfoTuple(err_info); if (exc_info == NULL) { Py_DECREF(id); goto fail; diff --git a/Python/sysmodule.c b/Python/sysmodule.c index 3e2091e70ab8a6..13fae797b29c2c 100644 --- a/Python/sysmodule.c +++ b/Python/sysmodule.c @@ -785,12 +785,7 @@ sys_exc_info_impl(PyObject *module) /*[clinic end generated code: output=3afd0940cf3a4d30 input=b5c5bf077788a3e5]*/ { _PyErr_StackItem *err_info = _PyErr_GetTopmostException(_PyThreadState_GET()); - return Py_BuildValue( - "(OOO)", - err_info->exc_type != NULL ? err_info->exc_type : Py_None, - err_info->exc_value != NULL ? err_info->exc_value : Py_None, - err_info->exc_traceback != NULL ? - err_info->exc_traceback : Py_None); + return _PyErr_StackItemToExcInfoTuple(err_info); } From fbff4f6831da24ac08a97749fc808393e482e4f6 Mon Sep 17 00:00:00 2001 From: Irit Katriel Date: Fri, 19 Nov 2021 10:38:19 +0000 Subject: [PATCH 2/6] put assertion definition under Py_DEBUG to remove Address sanitizer warning --- Python/errors.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Python/errors.c b/Python/errors.c index c3c6576ddcfab8..974bd1fa779ee6 100644 --- a/Python/errors.c +++ b/Python/errors.c @@ -633,14 +633,18 @@ _PyErr_ChainStackItem(_PyErr_StackItem *exc_info) exc2 = exc_info->exc_type; val2 = exc_info->exc_value; tb2 = exc_info->exc_traceback; +#ifdef Py_DEBUG PyObject *exc2_before = exc2; PyObject *val2_before = val2; PyObject *tb2_before = tb2; +#endif _PyErr_NormalizeException(tstate, &exc2, &val2, &tb2); +#ifdef Py_DEBUG /* exc_info should already be normalized */ assert(exc2 == exc2_before); assert(val2 == val2_before); assert(tb2 == tb2_before); +#endif if (tb2 != NULL) { PyException_SetTraceback(val2, tb2); } From 3c451a1b9d2715716c61bcffadca96b9512de9d3 Mon Sep 17 00:00:00 2001 From: Irit Katriel <1055913+iritkatriel@users.noreply.github.com> Date: Wed, 24 Nov 2021 21:59:34 +0000 Subject: [PATCH 3/6] err_info->exc_value ==> exc_value Co-authored-by: Guido van Rossum --- Python/errors.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Python/errors.c b/Python/errors.c index 1443ab57a92036..cac894acc41164 100644 --- a/Python/errors.c +++ b/Python/errors.c @@ -538,7 +538,7 @@ _PyErr_StackItemToExcInfoTuple(_PyErr_StackItem *err_info) assert(exc_value == Py_None || PyExceptionInstance_Check(exc_value)); PyObject *exc_type = PyExceptionInstance_Check(exc_value) ? - PyExceptionInstance_Class(err_info->exc_value) : + PyExceptionInstance_Class(exc_value) : Py_None; return Py_BuildValue( From 277c509bf0849a71044c89483b2f78f36791189e Mon Sep 17 00:00:00 2001 From: Irit Katriel Date: Wed, 24 Nov 2021 23:34:28 +0000 Subject: [PATCH 4/6] Simplified _PyErr_GetExcInfo. Moved assertion in _PyErr_GetTopmostException. --- Python/errors.c | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/Python/errors.c b/Python/errors.c index cac894acc41164..67f9f97fc8df8e 100644 --- a/Python/errors.c +++ b/Python/errors.c @@ -79,14 +79,15 @@ _PyErr_StackItem * _PyErr_GetTopmostException(PyThreadState *tstate) { _PyErr_StackItem *exc_info = tstate->exc_info; + assert(exc_info); + while ((exc_info->exc_value == NULL || exc_info->exc_value == Py_None) && exc_info->previous_item != NULL) { assert(exc_info->exc_type == NULL || exc_info->exc_type == Py_None); exc_info = exc_info->previous_item; } - assert(exc_info == NULL || - exc_info->previous_item == NULL || + assert(exc_info->previous_item == NULL || (exc_info->exc_type != NULL && exc_info->exc_type != Py_None)); return exc_info; } @@ -479,19 +480,14 @@ _PyErr_GetExcInfo(PyThreadState *tstate, *p_value = exc_info->exc_value; *p_traceback = exc_info->exc_traceback; - assert(*p_value == NULL || - *p_value == Py_None || - PyExceptionInstance_Check(*p_value)); - - *p_type = (*p_value != NULL && PyExceptionInstance_Check(*p_value)) ? - PyExceptionInstance_Class(*p_value) : - Py_None; - - if (exc_info->exc_type == NULL || exc_info->exc_type == Py_None) { - assert(*p_type == Py_None); + if (*p_value == NULL || *p_value == Py_None) { + assert(exc_info->exc_type == NULL || exc_info->exc_type == Py_None); + *p_type = Py_None; } else { - assert(*p_type == exc_info->exc_type); + assert(PyExceptionInstance_Check(*p_value)); + assert(exc_info->exc_type == PyExceptionInstance_Class(*p_value)); + *p_type = PyExceptionInstance_Class(*p_value); } Py_XINCREF(*p_type); From 437cebba2e8220e1766ee51bb9ee8fc842bee337 Mon Sep 17 00:00:00 2001 From: Irit Katriel Date: Wed, 24 Nov 2021 23:53:02 +0000 Subject: [PATCH 5/6] exc2 --> typ2 --- Python/errors.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/Python/errors.c b/Python/errors.c index 67f9f97fc8df8e..819401b28d5680 100644 --- a/Python/errors.c +++ b/Python/errors.c @@ -568,17 +568,17 @@ _PyErr_ChainExceptions(PyObject *exc, PyObject *val, PyObject *tb) } if (_PyErr_Occurred(tstate)) { - PyObject *exc2, *val2, *tb2; - _PyErr_Fetch(tstate, &exc2, &val2, &tb2); + PyObject *typ2, *val2, *tb2; + _PyErr_Fetch(tstate, &typ2, &val2, &tb2); _PyErr_NormalizeException(tstate, &exc, &val, &tb); if (tb != NULL) { PyException_SetTraceback(val, tb); Py_DECREF(tb); } Py_DECREF(exc); - _PyErr_NormalizeException(tstate, &exc2, &val2, &tb2); + _PyErr_NormalizeException(tstate, &typ2, &val2, &tb2); PyException_SetContext(val2, val); - _PyErr_Restore(tstate, exc2, val2, tb2); + _PyErr_Restore(tstate, typ2, val2, tb2); } else { _PyErr_Restore(tstate, exc, val, tb); @@ -625,19 +625,19 @@ _PyErr_ChainStackItem(_PyErr_StackItem *exc_info) PyObject *exc, *val, *tb; _PyErr_Fetch(tstate, &exc, &val, &tb); - PyObject *exc2, *val2, *tb2; - exc2 = exc_info->exc_type; + PyObject *typ2, *val2, *tb2; + typ2 = exc_info->exc_type; val2 = exc_info->exc_value; tb2 = exc_info->exc_traceback; #ifdef Py_DEBUG - PyObject *exc2_before = exc2; + PyObject *typ2_before = typ2; PyObject *val2_before = val2; PyObject *tb2_before = tb2; #endif - _PyErr_NormalizeException(tstate, &exc2, &val2, &tb2); + _PyErr_NormalizeException(tstate, &typ2, &val2, &tb2); #ifdef Py_DEBUG /* exc_info should already be normalized */ - assert(exc2 == exc2_before); + assert(typ2 == typ2_before); assert(val2 == val2_before); assert(tb2 == tb2_before); #endif From de67ed658b0bf862b565e146d18aa180e10210cf Mon Sep 17 00:00:00 2001 From: Irit Katriel Date: Wed, 24 Nov 2021 23:54:34 +0000 Subject: [PATCH 6/6] exc --> typ --- Python/errors.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/Python/errors.c b/Python/errors.c index 819401b28d5680..6e74d19b78ef33 100644 --- a/Python/errors.c +++ b/Python/errors.c @@ -552,36 +552,36 @@ _PyErr_StackItemToExcInfoTuple(_PyErr_StackItem *err_info) The caller is responsible for ensuring that this call won't create any cycles in the exception context chain. */ void -_PyErr_ChainExceptions(PyObject *exc, PyObject *val, PyObject *tb) +_PyErr_ChainExceptions(PyObject *typ, PyObject *val, PyObject *tb) { - if (exc == NULL) + if (typ == NULL) return; PyThreadState *tstate = _PyThreadState_GET(); - if (!PyExceptionClass_Check(exc)) { + if (!PyExceptionClass_Check(typ)) { _PyErr_Format(tstate, PyExc_SystemError, "_PyErr_ChainExceptions: " "exception %R is not a BaseException subclass", - exc); + typ); return; } if (_PyErr_Occurred(tstate)) { PyObject *typ2, *val2, *tb2; _PyErr_Fetch(tstate, &typ2, &val2, &tb2); - _PyErr_NormalizeException(tstate, &exc, &val, &tb); + _PyErr_NormalizeException(tstate, &typ, &val, &tb); if (tb != NULL) { PyException_SetTraceback(val, tb); Py_DECREF(tb); } - Py_DECREF(exc); + Py_DECREF(typ); _PyErr_NormalizeException(tstate, &typ2, &val2, &tb2); PyException_SetContext(val2, val); _PyErr_Restore(tstate, typ2, val2, tb2); } else { - _PyErr_Restore(tstate, exc, val, tb); + _PyErr_Restore(tstate, typ, val, tb); } } @@ -622,8 +622,8 @@ _PyErr_ChainStackItem(_PyErr_StackItem *exc_info) tstate->exc_info = exc_info; } - PyObject *exc, *val, *tb; - _PyErr_Fetch(tstate, &exc, &val, &tb); + PyObject *typ, *val, *tb; + _PyErr_Fetch(tstate, &typ, &val, &tb); PyObject *typ2, *val2, *tb2; typ2 = exc_info->exc_type; @@ -646,8 +646,8 @@ _PyErr_ChainStackItem(_PyErr_StackItem *exc_info) } /* _PyErr_SetObject sets the context from PyThreadState. */ - _PyErr_SetObject(tstate, exc, val); - Py_DECREF(exc); // since _PyErr_Occurred was true + _PyErr_SetObject(tstate, typ, val); + Py_DECREF(typ); // since _PyErr_Occurred was true Py_XDECREF(val); Py_XDECREF(tb);