From efde146b0c42f2643f96d00896c99a90d501fb69 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Sat, 21 Mar 2015 15:04:43 +0100 Subject: [PATCH] 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. --- Include/abstract.h | 5 +++-- Lib/test/test_capi.py | 44 +++++++++++++++++++++++++++++++++++++++ Modules/_testcapimodule.c | 22 ++++++++++++++++++++ Objects/abstract.c | 26 +++++++++++++++++------ Objects/methodobject.c | 2 +- Python/ceval.c | 6 +++--- 6 files changed, 93 insertions(+), 12 deletions(-) diff --git a/Include/abstract.h b/Include/abstract.h index 56fbf860350..83dbf94f6b9 100644 --- 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 de8d65a963c..ef6e94b1145 100644 --- 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 df35197198d..b8e1dbc4f9d 100644 --- 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 50d893d520a..a19d28c5491 100644 --- 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 85b413f09af..b5467a4687f 100644 --- 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 25fbc0fc79f..d68cdc6292e 100644 --- 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);