Issue #23571: _Py_CheckFunctionResult() now gives the name of the function

which returned an invalid result (result+error or no result without error) in
the exception message.

Add also unit test to check that the exception contains the name of the
function.

Special case: the final _PyEval_EvalFrameEx() check doesn't mention the
function since it didn't execute a single function but a whole frame.

diff --git a/Include/abstract.h b/Include/abstract.h
index 56fbf8603508d7cb04919cf22a42c23e1fe4b571..83dbf94f6b904283a89673118c937b0b141cf9b7 100644 (file)
--- a/Include/abstract.h
+++ b/Include/abstract.h
@@ -267,8 +267,9 @@ xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx*/
                                           PyObject *args, PyObject *kw);
 
 #ifndef Py_LIMITED_API
-     PyAPI_FUNC(PyObject *) _Py_CheckFunctionResult(PyObject *obj,
-                                                    const char *func_name);
+     PyAPI_FUNC(PyObject *) _Py_CheckFunctionResult(PyObject *func,
+                                                    PyObject *result,
+                                                    const char *where);
 #endif
 
        /*

diff --git a/Lib/test/test_capi.py b/Lib/test/test_capi.py
index de8d65a963ce4d1feddca0e3ec711dae5f660f52..ef6e94b11451c44741537e120fd3a4646d714f69 100644 (file)
--- a/Lib/test/test_capi.py
+++ b/Lib/test/test_capi.py
@@ -6,10 +6,12 @@ import pickle
 import random
 import subprocess
 import sys
+import textwrap
 import time
 import unittest
 from test import support
 from test.support import MISSING_C_DOCSTRINGS
+from test.script_helper import assert_python_failure
 try:
     import _posixsubprocess
 except ImportError:
@@ -21,6 +23,9 @@ except ImportError:
 # Skip this test if the _testcapi module isn't available.
 _testcapi = support.import_module('_testcapi')
 
+# Were we compiled --with-pydebug or with #define Py_DEBUG?
+Py_DEBUG = hasattr(sys, 'gettotalrefcount')
+
 
 def testfunction(self):
     """some doc"""
@@ -167,6 +172,45 @@ class CAPITest(unittest.TestCase):
         o @= m1
         self.assertEqual(o, ("matmul", 42, m1))
 
+    def test_return_null_without_error(self):
+        # Issue #23571: A function must not return NULL without setting an
+        # error
+        if Py_DEBUG:
+            code = textwrap.dedent("""
+                import _testcapi
+                from test import support
+
+                with support.SuppressCrashReport():
+                    _testcapi.return_null_without_error()
+            """)
+            rc, out, err = assert_python_failure('-c', code)
+            self.assertIn(b'_Py_CheckFunctionResult: Assertion', err)
+        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')
+
+    def test_return_result_with_error(self):
+        # Issue #23571: A function must not return a result with an error set
+        if Py_DEBUG:
+            code = textwrap.dedent("""
+                import _testcapi
+                from test import support
+
+                with support.SuppressCrashReport():
+                    _testcapi.return_result_with_error()
+            """)
+            rc, out, err = assert_python_failure('-c', code)
+            self.assertIn(b'_Py_CheckFunctionResult: Assertion', err)
+        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')
+
 
 @unittest.skipUnless(threading, 'Threading required for this test.')
 class TestPendingCalls(unittest.TestCase):

diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c
index df35197198d6a20537218e6cb1134e73eb2d5475..b8e1dbc4f9d2b72cfb695db96adf860c7e13c588 100644 (file)
--- a/Modules/_testcapimodule.c
+++ b/Modules/_testcapimodule.c
@@ -3360,6 +3360,24 @@ pymarshal_read_object_from_file(PyObject* self, PyObject *args)
     return Py_BuildValue("Nl", obj, pos);
 }
 
+static PyObject*
+return_null_without_error(PyObject *self, PyObject *args)
+{
+    /* invalid call: return NULL without setting an error,
+     * _Py_CheckFunctionResult() must detect such bug at runtime. */
+    PyErr_Clear();
+    return NULL;
+}
+
+static PyObject*
+return_result_with_error(PyObject *self, PyObject *args)
+{
+    /* invalid call: return a result with an error set,
+     * _Py_CheckFunctionResult() must detect such bug at runtime. */
+    PyErr_SetNone(PyExc_ValueError);
+    Py_RETURN_NONE;
+}
+
 
 static PyMethodDef TestMethods[] = {
     {"raise_exception",         raise_exception,                 METH_VARARGS},
@@ -3519,6 +3537,10 @@ 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},
     {NULL, NULL} /* sentinel */
 };
 

diff --git a/Objects/abstract.c b/Objects/abstract.c
index 50d893d520ac283329546392c14540f8cc5261a1..a19d28c54919b838934c8f13fc75e9ea0e5fcf49 100644 (file)
--- a/Objects/abstract.c
+++ b/Objects/abstract.c
@@ -2074,10 +2074,12 @@ PyObject_CallObject(PyObject *o, PyObject *a)
 }
 
 PyObject*
-_Py_CheckFunctionResult(PyObject *result, const char *func_name)
+_Py_CheckFunctionResult(PyObject *func, PyObject *result, const char *where)
 {
     int err_occurred = (PyErr_Occurred() != NULL);
 
+    assert((func != NULL) ^ (where != NULL));
+
 #ifndef NDEBUG
     /* In debug mode: abort() with an assertion error. Use two different
        assertions, so if an assertion fails, it's possible to know
@@ -2090,8 +2092,14 @@ _Py_CheckFunctionResult(PyObject *result, const char *func_name)
 
     if (result == NULL) {
         if (!err_occurred) {
-            PyErr_Format(PyExc_SystemError,
-                         "NULL result without error in %s", func_name);
+            if (func)
+                PyErr_Format(PyExc_SystemError,
+                             "%R returned NULL without setting an error",
+                             func);
+            else
+                PyErr_Format(PyExc_SystemError,
+                             "%s returned NULL without setting an error",
+                             where);
             return NULL;
         }
     }
@@ -2102,8 +2110,14 @@ _Py_CheckFunctionResult(PyObject *result, const char *func_name)
 
             Py_DECREF(result);
 
-            PyErr_Format(PyExc_SystemError,
-                         "result with error in %s", func_name);
+            if (func)
+                PyErr_Format(PyExc_SystemError,
+                             "%R returned a result with an error set",
+                             func);
+            else
+                PyErr_Format(PyExc_SystemError,
+                             "%s returned a result with an error set",
+                             where);
             _PyErr_ChainExceptions(exc, val, tb);
             return NULL;
         }
@@ -2136,7 +2150,7 @@ PyObject_Call(PyObject *func, PyObject *arg, PyObject *kw)
 
     Py_LeaveRecursiveCall();
 
-    return _Py_CheckFunctionResult(result, "PyObject_Call");
+    return _Py_CheckFunctionResult(func, result, NULL);
 }
 
 static PyObject*

diff --git a/Objects/methodobject.c b/Objects/methodobject.c
index 85b413f09af7bb0a7b57d88d4ab156108dfd1943..b5467a4687f030e2a3e7f360f1d191942c7f189f 100644 (file)
--- a/Objects/methodobject.c
+++ b/Objects/methodobject.c
@@ -142,7 +142,7 @@ PyCFunction_Call(PyObject *func, PyObject *args, PyObject *kwds)
         }
     }
 
-    return _Py_CheckFunctionResult(res, "PyCFunction_Call");
+    return _Py_CheckFunctionResult(func, res, NULL);
 }
 
 /* Methods (the standard built-in methods, that is) */

diff --git a/Python/ceval.c b/Python/ceval.c
index 25fbc0fc79f424a211dccf41c79c39b3b7654a7a..d68cdc6292e95e8f57d80888f4cec3380bca6957 100644 (file)
--- a/Python/ceval.c
+++ b/Python/ceval.c
@@ -3253,7 +3253,7 @@ exit_eval_frame:
     f->f_executing = 0;
     tstate->frame = f->f_back;
 
-    return _Py_CheckFunctionResult(retval, "PyEval_EvalFrameEx");
+    return _Py_CheckFunctionResult(NULL, retval, "PyEval_EvalFrameEx");
 }
 
 static void
@@ -4251,14 +4251,14 @@ call_function(PyObject ***pp_stack, int oparg
             if (flags & METH_NOARGS && na == 0) {
                 C_TRACE(x, (*meth)(self,NULL));
 
-                x = _Py_CheckFunctionResult(x, "call_function");
+                x = _Py_CheckFunctionResult(func, x, NULL);
             }
             else if (flags & METH_O && na == 1) {
                 PyObject *arg = EXT_POP(*pp_stack);
                 C_TRACE(x, (*meth)(self,arg));
                 Py_DECREF(arg);
 
-                x = _Py_CheckFunctionResult(x, "call_function");
+                x = _Py_CheckFunctionResult(func, x, NULL);
             }
             else {
                 err_args(func, flags, na);