From 856754a3def33cd8aba6effb52943d272a5db4c1 Mon Sep 17 00:00:00 2001 From: Sam Edwards Date: Tue, 5 Nov 2019 18:43:59 -0700 Subject: [PATCH] general: Don't abuse PyErr_Restore The intended purpose of this function is to restore an exception that has already been raised and saved with PyErr_Fetch. It should not be used to raise new exceptions nor should it be used to clear the current exception. The especially egregious example is `PyErr_Restore(exc_type, nullptr, nullptr);` as the null value may not be handled correctly. --- dtool/src/interrogatedb/py_panda.cxx | 18 ++++-------------- dtool/src/interrogatedb/py_wrappers.cxx | 10 +++++----- panda/src/event/asyncFuture_ext.cxx | 11 ++++------- panda/src/event/pythonTask.cxx | 2 +- 4 files changed, 14 insertions(+), 27 deletions(-) diff --git a/dtool/src/interrogatedb/py_panda.cxx b/dtool/src/interrogatedb/py_panda.cxx index 5d17d8851f..725e9bdedd 100644 --- a/dtool/src/interrogatedb/py_panda.cxx +++ b/dtool/src/interrogatedb/py_panda.cxx @@ -156,8 +156,7 @@ PyObject *Dtool_Raise_AssertionError() { #else PyObject *message = PyString_FromString(notify->get_assert_error_message().c_str()); #endif - Py_INCREF(PyExc_AssertionError); - PyErr_Restore(PyExc_AssertionError, message, nullptr); + PyErr_SetObject(PyExc_AssertionError, message); notify->clear_assert_failed(); return nullptr; } @@ -166,14 +165,7 @@ PyObject *Dtool_Raise_AssertionError() { * Raises a TypeError with the given message, and returns NULL. */ PyObject *Dtool_Raise_TypeError(const char *message) { - // PyErr_Restore is what PyErr_SetString would have ended up calling - // eventually anyway, so we might as well just get to the point. - Py_INCREF(PyExc_TypeError); -#if PY_MAJOR_VERSION >= 3 - PyErr_Restore(PyExc_TypeError, PyUnicode_FromString(message), nullptr); -#else - PyErr_Restore(PyExc_TypeError, PyString_FromString(message), nullptr); -#endif + PyErr_SetString(PyExc_TypeError, message); return nullptr; } @@ -194,8 +186,7 @@ PyObject *Dtool_Raise_ArgTypeError(PyObject *obj, int param, const char *functio function_name, param, type_name, Py_TYPE(obj)->tp_name); - Py_INCREF(PyExc_TypeError); - PyErr_Restore(PyExc_TypeError, message, nullptr); + PyErr_SetObject(PyExc_TypeError, message); return nullptr; } @@ -214,8 +205,7 @@ PyObject *Dtool_Raise_AttributeError(PyObject *obj, const char *attribute) { "'%.100s' object has no attribute '%.200s'", Py_TYPE(obj)->tp_name, attribute); - Py_INCREF(PyExc_AttributeError); - PyErr_Restore(PyExc_AttributeError, message, nullptr); + PyErr_SetObject(PyExc_AttributeError, message); return nullptr; } diff --git a/dtool/src/interrogatedb/py_wrappers.cxx b/dtool/src/interrogatedb/py_wrappers.cxx index 6aed3d2bbb..0b1bb59c66 100644 --- a/dtool/src/interrogatedb/py_wrappers.cxx +++ b/dtool/src/interrogatedb/py_wrappers.cxx @@ -75,7 +75,7 @@ static PyObject *Dtool_SequenceWrapper_repr(PyObject *self) { } if (len < 0) { - PyErr_Restore(nullptr, nullptr, nullptr); + PyErr_Clear(); return Dtool_WrapperBase_repr(self); } @@ -422,7 +422,7 @@ static int Dtool_MappingWrapper_contains(PyObject *self, PyObject *key) { return 1; } else if (_PyErr_OCCURRED() == PyExc_KeyError || _PyErr_OCCURRED() == PyExc_TypeError) { - PyErr_Restore(nullptr, nullptr, nullptr); + PyErr_Clear(); return 0; } else { return -1; @@ -480,7 +480,7 @@ static PyObject *Dtool_MappingWrapper_get(PyObject *self, PyObject *args) { if (value != nullptr) { return value; } else if (_PyErr_OCCURRED() == PyExc_KeyError) { - PyErr_Restore(nullptr, nullptr, nullptr); + PyErr_Clear(); Py_INCREF(defvalue); return defvalue; } else { @@ -944,7 +944,7 @@ static PyObject *Dtool_MutableMappingWrapper_pop(PyObject *self, PyObject *args) return nullptr; } } else if (_PyErr_OCCURRED() == PyExc_KeyError) { - PyErr_Restore(nullptr, nullptr, nullptr); + PyErr_Clear(); Py_INCREF(defvalue); return defvalue; } else { @@ -1044,7 +1044,7 @@ static PyObject *Dtool_MutableMappingWrapper_setdefault(PyObject *self, PyObject if (value != nullptr) { return value; } else if (_PyErr_OCCURRED() == PyExc_KeyError) { - PyErr_Restore(nullptr, nullptr, nullptr); + PyErr_Clear(); if (wrap->_setitem_func(wrap->_base._self, key, defvalue) == 0) { Py_INCREF(defvalue); return defvalue; diff --git a/panda/src/event/asyncFuture_ext.cxx b/panda/src/event/asyncFuture_ext.cxx index 5bf7cb3c94..c4cc4b5302 100644 --- a/panda/src/event/asyncFuture_ext.cxx +++ b/panda/src/event/asyncFuture_ext.cxx @@ -102,7 +102,7 @@ static PyObject *get_done_result(const AsyncFuture *future) { if (value != nullptr) { return value; } - PyErr_Restore(nullptr, nullptr, nullptr); + PyErr_Clear(); Py_DECREF(wrap); } } @@ -132,8 +132,7 @@ static PyObject *get_done_result(const AsyncFuture *future) { nullptr, nullptr); } } - Py_INCREF(exc_type); - PyErr_Restore(exc_type, nullptr, nullptr); + PyErr_SetNone(exc_type); return nullptr; } } @@ -154,8 +153,7 @@ static PyObject *gen_next(PyObject *self) { } else { PyObject *result = get_done_result(future); if (result != nullptr) { - Py_INCREF(PyExc_StopIteration); - PyErr_Restore(PyExc_StopIteration, result, nullptr); + PyErr_SetObject(PyExc_StopIteration, result); } return nullptr; } @@ -225,8 +223,7 @@ result(PyObject *timeout) const { nullptr, nullptr); } } - Py_INCREF(exc_type); - PyErr_Restore(exc_type, nullptr, nullptr); + PyErr_SetNone(exc_type); return nullptr; } } diff --git a/panda/src/event/pythonTask.cxx b/panda/src/event/pythonTask.cxx index fdf6be440c..86c05b33d7 100644 --- a/panda/src/event/pythonTask.cxx +++ b/panda/src/event/pythonTask.cxx @@ -539,7 +539,7 @@ do_python_task() { result = Py_None; Py_INCREF(result); #endif - PyErr_Restore(nullptr, nullptr, nullptr); + PyErr_Clear(); // If we passed a coroutine into the task, eg. something like: // taskMgr.add(my_async_function())