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).

Author: vstinner

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);
+PyAPI_FUNC(PyObject *) _PyNumber_Index(PyObject *o);

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

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 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 @@
 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 @@ def test_return_null_without_error(self):
                     _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: <built-in function '
-                                 br'return_null_without_error> returned NULL '
-                                 br'without setting an error\n'
-                             br'\n'
-                             br'Current thread.*:\n'
-                             br'  File .*", line 6 in <module>')
+            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: <built-in function return_null_without_error> '
+                    r'returned NULL without setting an exception\n'
+                r'\n'
+                r'Current thread.*:\n'
+                r'  File .*", line 6 in <module>\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 @@ def test_return_result_with_error(self):
                     _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: <built-in '
-                                br'function return_result_with_error> '
-                                br'returned a result with an error set\n'
-                             br'\n'
-                             br'Current thread.*:\n'
-                             br'  File .*, line 6 in <module>')
+            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: <built-in '
+                        r'function return_result_with_error> '
+                        r'returned a result with an exception set\n'
+                    r'\n'
+                    r'Current thread.*:\n'
+                    r'  File .*, line 6 in <module>\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 <module>\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 @@ def check_fatal_error(self, code, expected, not_expected=()):
         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)
 

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},

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 <ctype.h>
 #include <stddef.h>               // 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;
     }
 

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 */