From c9b8e9c421b57acdcaf24fab0c93bc29b3ef7c67 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Wed, 27 Jan 2021 17:39:16 +0100 Subject: [PATCH] bpo-42979: Enhance abstract.c assertions checking slot result (GH-24352) * bpo-42979: Enhance abstract.c assertions checking slot result Add _Py_CheckSlotResult() function which fails with a fatal error if a slot function succeeded with an exception set or failed with no exception set: write the slot name, the type name and the current exception (if an exception is set). --- Include/cpython/abstract.h | 2 +- Include/internal/pycore_object.h | 6 ++ Lib/test/test_capi.py | 97 +++++++++++++++++++++----------- Modules/_testcapimodule.c | 19 +++++-- Objects/abstract.c | 17 +++--- Objects/call.c | 36 ++++++++++-- 6 files changed, 126 insertions(+), 51 deletions(-) diff --git a/Include/cpython/abstract.h b/Include/cpython/abstract.h index 1083942c149..7a4219c8b33 100644 --- a/Include/cpython/abstract.h +++ b/Include/cpython/abstract.h @@ -376,4 +376,4 @@ PyAPI_FUNC(void) _Py_add_one_to_index_C(int nd, Py_ssize_t *index, PyAPI_FUNC(int) _Py_convert_optional_to_ssize_t(PyObject *, void *); /* Same as PyNumber_Index but can return an instance of a subclass of int. */ -PyAPI_FUNC(PyObject *) _PyNumber_Index(PyObject *o); \ No newline at end of file +PyAPI_FUNC(PyObject *) _PyNumber_Index(PyObject *o); diff --git a/Include/internal/pycore_object.h b/Include/internal/pycore_object.h index 3975765a46c..3cd27b035c2 100644 --- a/Include/internal/pycore_object.h +++ b/Include/internal/pycore_object.h @@ -168,6 +168,12 @@ _PyObject_IS_GC(PyObject *obj) // Fast inlined version of PyType_IS_GC() #define _PyType_IS_GC(t) _PyType_HasFeature((t), Py_TPFLAGS_HAVE_GC) +// Usage: assert(_Py_CheckSlotResult(obj, "__getitem__", result != NULL))); +extern int _Py_CheckSlotResult( + PyObject *obj, + const char *slot_name, + int success); + #ifdef __cplusplus } #endif diff --git a/Lib/test/test_capi.py b/Lib/test/test_capi.py index 8e92a50026c..1b18bfad553 100644 --- a/Lib/test/test_capi.py +++ b/Lib/test/test_capi.py @@ -2,6 +2,8 @@ # these are all functions _testcapi exports whose name begins with 'test_'. from collections import OrderedDict +import importlib.machinery +import importlib.util import os import pickle import random @@ -13,8 +15,6 @@ import threading import time import unittest import weakref -import importlib.machinery -import importlib.util from test import support from test.support import MISSING_C_DOCSTRINGS from test.support import import_helper @@ -35,6 +35,10 @@ import _testinternalcapi Py_DEBUG = hasattr(sys, 'gettotalrefcount') +def decode_stderr(err): + return err.decode('utf-8', 'replace').replace('\r', '') + + def testfunction(self): """some doc""" return self @@ -207,23 +211,22 @@ class CAPITest(unittest.TestCase): _testcapi.return_null_without_error() """) rc, out, err = assert_python_failure('-c', code) - self.assertRegex(err.replace(b'\r', b''), - br'Fatal Python error: _Py_CheckFunctionResult: ' - br'a function returned NULL ' - br'without setting an error\n' - br'Python runtime state: initialized\n' - br'SystemError: returned NULL ' - br'without setting an error\n' - br'\n' - br'Current thread.*:\n' - br' File .*", line 6 in ') + err = decode_stderr(err) + self.assertRegex(err, + r'Fatal Python error: _Py_CheckFunctionResult: ' + r'a function returned NULL without setting an exception\n' + r'Python runtime state: initialized\n' + r'SystemError: ' + r'returned NULL without setting an exception\n' + r'\n' + r'Current thread.*:\n' + r' File .*", line 6 in \n') else: with self.assertRaises(SystemError) as cm: _testcapi.return_null_without_error() self.assertRegex(str(cm.exception), 'return_null_without_error.* ' - 'returned NULL without setting an error') + 'returned NULL without setting an exception') def test_return_result_with_error(self): # Issue #23571: A function must not return a result with an error set @@ -236,28 +239,58 @@ class CAPITest(unittest.TestCase): _testcapi.return_result_with_error() """) rc, out, err = assert_python_failure('-c', code) - self.assertRegex(err.replace(b'\r', b''), - br'Fatal Python error: _Py_CheckFunctionResult: ' - br'a function returned a result ' - br'with an error set\n' - br'Python runtime state: initialized\n' - br'ValueError\n' - br'\n' - br'The above exception was the direct cause ' - br'of the following exception:\n' - br'\n' - br'SystemError: ' - br'returned a result with an error set\n' - br'\n' - br'Current thread.*:\n' - br' File .*, line 6 in ') + err = decode_stderr(err) + self.assertRegex(err, + r'Fatal Python error: _Py_CheckFunctionResult: ' + r'a function returned a result with an exception set\n' + r'Python runtime state: initialized\n' + r'ValueError\n' + r'\n' + r'The above exception was the direct cause ' + r'of the following exception:\n' + r'\n' + r'SystemError: ' + r'returned a result with an exception set\n' + r'\n' + r'Current thread.*:\n' + r' File .*, line 6 in \n') else: with self.assertRaises(SystemError) as cm: _testcapi.return_result_with_error() self.assertRegex(str(cm.exception), 'return_result_with_error.* ' - 'returned a result with an error set') + 'returned a result with an exception set') + + def test_getitem_with_error(self): + # Test _Py_CheckSlotResult(). Raise an exception and then calls + # PyObject_GetItem(): check that the assertion catchs the bug. + # PyObject_GetItem() must not be called with an exception set. + code = textwrap.dedent(""" + import _testcapi + from test import support + + with support.SuppressCrashReport(): + _testcapi.getitem_with_error({1: 2}, 1) + """) + rc, out, err = assert_python_failure('-c', code) + err = decode_stderr(err) + if 'SystemError: ' not in err: + self.assertRegex(err, + r'Fatal Python error: _Py_CheckSlotResult: ' + r'Slot __getitem__ of type dict succeeded ' + r'with an exception set\n' + r'Python runtime state: initialized\n' + r'ValueError: bug\n' + r'\n' + r'Current thread .* \(most recent call first\):\n' + r' File .*, line 6 in \n' + r'\n' + r'Extension modules: _testcapi \(total: 1\)\n') + else: + # Python built with NDEBUG macro defined: + # test _Py_CheckFunctionResult() instead. + self.assertIn('returned a result with an exception set', err) def test_buildvalue_N(self): _testcapi.test_buildvalue_N() @@ -551,7 +584,7 @@ class CAPITest(unittest.TestCase): with support.SuppressCrashReport(): rc, out, err = assert_python_failure('-sSI', '-c', code) - err = err.replace(b'\r', b'').decode('ascii', 'replace') + err = decode_stderr(err) self.assertIn('Fatal Python error: test_fatal_error: MESSAGE\n', err) diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index 2a5b3d9c50b..ed59c32e9c5 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -4736,6 +4736,18 @@ return_result_with_error(PyObject *self, PyObject *args) Py_RETURN_NONE; } +static PyObject* +getitem_with_error(PyObject *self, PyObject *args) +{ + PyObject *map, *key; + if (!PyArg_ParseTuple(args, "OO", &map, &key)) { + return NULL; + } + + PyErr_SetString(PyExc_ValueError, "bug"); + return PyObject_GetItem(map, key); +} + static PyObject * test_pytime_fromseconds(PyObject *self, PyObject *args) { @@ -5901,10 +5913,9 @@ static PyMethodDef TestMethods[] = { pymarshal_read_last_object_from_file, METH_VARARGS}, {"pymarshal_read_object_from_file", pymarshal_read_object_from_file, METH_VARARGS}, - {"return_null_without_error", - return_null_without_error, METH_NOARGS}, - {"return_result_with_error", - return_result_with_error, METH_NOARGS}, + {"return_null_without_error", return_null_without_error, METH_NOARGS}, + {"return_result_with_error", return_result_with_error, METH_NOARGS}, + {"getitem_with_error", getitem_with_error, METH_VARARGS}, {"PyTime_FromSeconds", test_pytime_fromseconds, METH_VARARGS}, {"PyTime_FromSecondsObject", test_pytime_fromsecondsobject, METH_VARARGS}, {"PyTime_AsSecondsDouble", test_pytime_assecondsdouble, METH_VARARGS}, diff --git a/Objects/abstract.c b/Objects/abstract.c index 44ed5b3932b..cc452eaaf3d 100644 --- a/Objects/abstract.c +++ b/Objects/abstract.c @@ -1,11 +1,12 @@ /* Abstract Object Interface (many thanks to Jim Fulton) */ #include "Python.h" -#include "pycore_unionobject.h" // _Py_UnionType && _Py_Union() #include "pycore_abstract.h" // _PyIndex_Check() #include "pycore_ceval.h" // _Py_EnterRecursiveCall() +#include "pycore_object.h" // _Py_CheckSlotResult() #include "pycore_pyerrors.h" // _PyErr_Occurred() #include "pycore_pystate.h" // _PyThreadState_GET() +#include "pycore_unionobject.h" // _Py_UnionType && _Py_Union() #include #include // offsetof() #include "longintrepr.h" @@ -61,7 +62,7 @@ PyObject_Size(PyObject *o) m = Py_TYPE(o)->tp_as_sequence; if (m && m->sq_length) { Py_ssize_t len = m->sq_length(o); - assert(len >= 0 || PyErr_Occurred()); + assert(_Py_CheckSlotResult(o, "__len__", len >= 0)); return len; } @@ -160,7 +161,7 @@ PyObject_GetItem(PyObject *o, PyObject *key) m = Py_TYPE(o)->tp_as_mapping; if (m && m->mp_subscript) { PyObject *item = m->mp_subscript(o, key); - assert((item != NULL) ^ (PyErr_Occurred() != NULL)); + assert(_Py_CheckSlotResult(o, "__getitem__", item != NULL)); return item; } @@ -1564,7 +1565,7 @@ PySequence_Size(PyObject *s) m = Py_TYPE(s)->tp_as_sequence; if (m && m->sq_length) { Py_ssize_t len = m->sq_length(s); - assert(len >= 0 || PyErr_Occurred()); + assert(_Py_CheckSlotResult(s, "__len__", len >= 0)); return len; } @@ -1708,8 +1709,8 @@ PySequence_GetItem(PyObject *s, Py_ssize_t i) if (i < 0) { if (m->sq_length) { Py_ssize_t l = (*m->sq_length)(s); + assert(_Py_CheckSlotResult(s, "__len__", l >= 0)); if (l < 0) { - assert(PyErr_Occurred()); return NULL; } i += l; @@ -1762,8 +1763,8 @@ PySequence_SetItem(PyObject *s, Py_ssize_t i, PyObject *o) if (i < 0) { if (m->sq_length) { Py_ssize_t l = (*m->sq_length)(s); + assert(_Py_CheckSlotResult(s, "__len__", l >= 0)); if (l < 0) { - assert(PyErr_Occurred()); return -1; } i += l; @@ -1795,8 +1796,8 @@ PySequence_DelItem(PyObject *s, Py_ssize_t i) if (i < 0) { if (m->sq_length) { Py_ssize_t l = (*m->sq_length)(s); + assert(_Py_CheckSlotResult(s, "__len__", l >= 0)); if (l < 0) { - assert(PyErr_Occurred()); return -1; } i += l; @@ -2145,7 +2146,7 @@ PyMapping_Size(PyObject *o) m = Py_TYPE(o)->tp_as_mapping; if (m && m->mp_length) { Py_ssize_t len = m->mp_length(o); - assert(len >= 0 || PyErr_Occurred()); + assert(_Py_CheckSlotResult(o, "__len__", len >= 0)); return len; } diff --git a/Objects/call.c b/Objects/call.c index 35b06a9b9c4..1fb85efab61 100644 --- a/Objects/call.c +++ b/Objects/call.c @@ -39,16 +39,16 @@ _Py_CheckFunctionResult(PyThreadState *tstate, PyObject *callable, if (!_PyErr_Occurred(tstate)) { if (callable) _PyErr_Format(tstate, PyExc_SystemError, - "%R returned NULL without setting an error", + "%R returned NULL without setting an exception", callable); else _PyErr_Format(tstate, PyExc_SystemError, - "%s returned NULL without setting an error", + "%s returned NULL without setting an exception", where); #ifdef Py_DEBUG /* Ensure that the bug is caught in debug mode. Py_FatalError() logs the SystemError exception raised above. */ - Py_FatalError("a function returned NULL without setting an error"); + Py_FatalError("a function returned NULL without setting an exception"); #endif return NULL; } @@ -60,17 +60,17 @@ _Py_CheckFunctionResult(PyThreadState *tstate, PyObject *callable, if (callable) { _PyErr_FormatFromCauseTstate( tstate, PyExc_SystemError, - "%R returned a result with an error set", callable); + "%R returned a result with an exception set", callable); } else { _PyErr_FormatFromCauseTstate( tstate, PyExc_SystemError, - "%s returned a result with an error set", where); + "%s returned a result with an exception set", where); } #ifdef Py_DEBUG /* Ensure that the bug is caught in debug mode. Py_FatalError() logs the SystemError exception raised above. */ - Py_FatalError("a function returned a result with an error set"); + Py_FatalError("a function returned a result with an exception set"); #endif return NULL; } @@ -79,6 +79,30 @@ _Py_CheckFunctionResult(PyThreadState *tstate, PyObject *callable, } +int +_Py_CheckSlotResult(PyObject *obj, const char *slot_name, int success) +{ + PyThreadState *tstate = _PyThreadState_GET(); + if (!success) { + if (!_PyErr_Occurred(tstate)) { + _Py_FatalErrorFormat(__func__, + "Slot %s of type %s failed " + "without setting an exception", + slot_name, Py_TYPE(obj)->tp_name); + } + } + else { + if (_PyErr_Occurred(tstate)) { + _Py_FatalErrorFormat(__func__, + "Slot %s of type %s succeeded " + "with an exception set", + slot_name, Py_TYPE(obj)->tp_name); + } + } + return 1; +} + + /* --- Core PyObject call functions ------------------------------- */ /* Call a callable Python object without any arguments */