Issue #23571: PyObject_Call(), PyCFunction_Call() and call_function() now

raise a SystemError if a function returns a result and raises an exception.
The SystemError is chained to the previous exception.

Refactor also PyObject_Call() and PyCFunction_Call() to make them more readable.

Remove some checks which became useless (duplicate checks).

Change reviewed by Serhiy Storchaka.

7 files changed, 125 insertions, 96 deletions

diff --git a/Include/abstract.h b/Include/abstract.h
index db70f21..56fbf86 100644
--- a/Include/abstract.h
+++ b/Include/abstract.h
@@ -266,6 +266,11 @@ xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx*/
      PyAPI_FUNC(PyObject *) PyObject_Call(PyObject *callable_object,
                                           PyObject *args, PyObject *kw);
 
+#ifndef Py_LIMITED_API
+     PyAPI_FUNC(PyObject *) _Py_CheckFunctionResult(PyObject *obj,
+                                                    const char *func_name);
+#endif
+
        /*
      Call a callable Python object, callable_object, with
      arguments and keywords arguments.  The 'args' argument can not be
diff --git a/Misc/NEWS b/Misc/NEWS
index 6808d37..9acb5d9 100644
--- a/Misc/NEWS
+++ b/Misc/NEWS
@@ -10,6 +10,10 @@ Release date: 2015-03-08
 Core and Builtins
 -----------------
 
+- Issue #23571: PyObject_Call() and PyCFunction_Call() now raise a SystemError
+  if a function returns a result and raises an exception. The SystemError is
+  chained to the previous exception.
+
 Library
 -------
 
diff --git a/Modules/_io/bufferedio.c b/Modules/_io/bufferedio.c
index 692ce41..370bb5e 100644
--- a/Modules/_io/bufferedio.c
+++ b/Modules/_io/bufferedio.c
@@ -680,11 +680,7 @@ static void
 _set_BlockingIOError(char *msg, Py_ssize_t written)
 {
     PyObject *err;
-#ifdef Py_DEBUG
-    /* in debug mode, PyEval_EvalFrameEx() fails with an assertion error
-       if an exception is set when it is called */
     PyErr_Clear();
-#endif
     err = PyObject_CallFunction(PyExc_BlockingIOError, "isn",
                                 errno, msg, written);
     if (err)
diff --git a/Modules/_sqlite/cursor.c b/Modules/_sqlite/cursor.c
index 7fe00e3..c1599c0 100644
--- a/Modules/_sqlite/cursor.c
+++ b/Modules/_sqlite/cursor.c
@@ -334,11 +334,7 @@ PyObject* _pysqlite_fetch_one_row(pysqlite_Cursor* self)
                 if (self->connection->text_factory == (PyObject*)&PyUnicode_Type) {
                     converted = PyUnicode_FromStringAndSize(val_str, nbytes);
                     if (!converted) {
-#ifdef Py_DEBUG
-                        /* in debug mode, type_call() fails with an assertion
-                           error if an exception is set when it is called */
                         PyErr_Clear();
-#endif
                         colname = sqlite3_column_name(self->statement->st, i);
                         if (!colname) {
                             colname = "<unknown column name>";
diff --git a/Objects/abstract.c b/Objects/abstract.c
index 06e3382..ab13476 100644
--- a/Objects/abstract.c
+++ b/Objects/abstract.c
@@ -2073,37 +2073,70 @@ PyObject_CallObject(PyObject *o, PyObject *a)
     return PyEval_CallObjectWithKeywords(o, a, NULL);
 }
 
+PyObject*
+_Py_CheckFunctionResult(PyObject *result, const char *func_name)
+{
+    int err_occurred = (PyErr_Occurred() != NULL);
+
+#ifdef NDEBUG
+    /* In debug mode: abort() with an assertion error. Use two different
+       assertions, so if an assertion fails, it's possible to know
+       if result was set or not and if an exception was raised or not. */
+    if (result != NULL)
+        assert(!err_occurred);
+    else
+        assert(err_occurred);
+#endif
+
+    if (result == NULL) {
+        if (!err_occurred) {
+            PyErr_Format(PyExc_SystemError,
+                         "NULL result without error in %s", func_name);
+            return NULL;
+        }
+    }
+    else {
+        if (err_occurred) {
+            PyObject *exc, *val, *tb;
+            PyErr_Fetch(&exc, &val, &tb);
+
+            Py_DECREF(result);
+
+            PyErr_Format(PyExc_SystemError,
+                         "result with error in %s", func_name);
+            _PyErr_ChainExceptions(exc, val, tb);
+            return NULL;
+        }
+    }
+    return result;
+}
+
 PyObject *
 PyObject_Call(PyObject *func, PyObject *arg, PyObject *kw)
 {
     ternaryfunc call;
+    PyObject *result;
 
     /* PyObject_Call() must not be called with an exception set,
        because it may clear it (directly or indirectly) and so the
        caller looses its exception */
     assert(!PyErr_Occurred());
 
-    if ((call = func->ob_type->tp_call) != NULL) {
-        PyObject *result;
-        if (Py_EnterRecursiveCall(" while calling a Python object"))
-            return NULL;
-        result = (*call)(func, arg, kw);
-        Py_LeaveRecursiveCall();
-#ifdef NDEBUG
-        if (result == NULL && !PyErr_Occurred()) {
-            PyErr_SetString(
-                PyExc_SystemError,
-                "NULL result without error in PyObject_Call");
-        }
-#else
-        assert((result != NULL && !PyErr_Occurred())
-                || (result == NULL && PyErr_Occurred()));
-#endif
-        return result;
+    call = func->ob_type->tp_call;
+    if (call == NULL) {
+        PyErr_Format(PyExc_TypeError, "'%.200s' object is not callable",
+                     func->ob_type->tp_name);
+        return NULL;
     }
-    PyErr_Format(PyExc_TypeError, "'%.200s' object is not callable",
-                 func->ob_type->tp_name);
-    return NULL;
+
+    if (Py_EnterRecursiveCall(" while calling a Python object"))
+        return NULL;
+
+    result = (*call)(func, arg, kw);
+
+    Py_LeaveRecursiveCall();
+
+    return _Py_CheckFunctionResult(result, "PyObject_Call");
 }
 
 static PyObject*
diff --git a/Objects/methodobject.c b/Objects/methodobject.c
index 686baf9..85b413f 100644
--- a/Objects/methodobject.c
+++ b/Objects/methodobject.c
@@ -78,68 +78,71 @@ PyCFunction_GetFlags(PyObject *op)
 }
 
 PyObject *
-PyCFunction_Call(PyObject *func, PyObject *arg, PyObject *kw)
+PyCFunction_Call(PyObject *func, PyObject *args, PyObject *kwds)
 {
-#define CHECK_RESULT(res) assert(res != NULL || PyErr_Occurred())
-
     PyCFunctionObject* f = (PyCFunctionObject*)func;
     PyCFunction meth = PyCFunction_GET_FUNCTION(func);
     PyObject *self = PyCFunction_GET_SELF(func);
-    PyObject *res;
+    PyObject *arg, *res;
     Py_ssize_t size;
+    int flags;
 
-    switch (PyCFunction_GET_FLAGS(func) & ~(METH_CLASS | METH_STATIC | METH_COEXIST)) {
-    case METH_VARARGS:
-        if (kw == NULL || PyDict_Size(kw) == 0) {
-            res = (*meth)(self, arg);
-            CHECK_RESULT(res);
-            return res;
-        }
-        break;
-    case METH_VARARGS | METH_KEYWORDS:
-        res = (*(PyCFunctionWithKeywords)meth)(self, arg, kw);
-        CHECK_RESULT(res);
-        return res;
-    case METH_NOARGS:
-        if (kw == NULL || PyDict_Size(kw) == 0) {
-            size = PyTuple_GET_SIZE(arg);
-            if (size == 0) {
-                res = (*meth)(self, NULL);
-                CHECK_RESULT(res);
-                return res;
-            }
-            PyErr_Format(PyExc_TypeError,
-                "%.200s() takes no arguments (%zd given)",
-                f->m_ml->ml_name, size);
+    /* PyCFunction_Call() must not be called with an exception set,
+       because it may clear it (directly or indirectly) and so the
+       caller looses its exception */
+    assert(!PyErr_Occurred());
+
+    flags = PyCFunction_GET_FLAGS(func) & ~(METH_CLASS | METH_STATIC | METH_COEXIST);
+
+    if (flags == (METH_VARARGS | METH_KEYWORDS)) {
+        res = (*(PyCFunctionWithKeywords)meth)(self, args, kwds);
+    }
+    else {
+        if (kwds != NULL && PyDict_Size(kwds) != 0) {
+            PyErr_Format(PyExc_TypeError, "%.200s() takes no keyword arguments",
+                         f->m_ml->ml_name);
             return NULL;
         }
-        break;
-    case METH_O:
-        if (kw == NULL || PyDict_Size(kw) == 0) {
-            size = PyTuple_GET_SIZE(arg);
-            if (size == 1) {
-                res = (*meth)(self, PyTuple_GET_ITEM(arg, 0));
-                CHECK_RESULT(res);
-                return res;
+
+        switch (flags) {
+        case METH_VARARGS:
+            res = (*meth)(self, args);
+            break;
+
+        case METH_NOARGS:
+            size = PyTuple_GET_SIZE(args);
+            if (size != 0) {
+                PyErr_Format(PyExc_TypeError,
+                    "%.200s() takes no arguments (%zd given)",
+                    f->m_ml->ml_name, size);
+                return NULL;
             }
-            PyErr_Format(PyExc_TypeError,
-                "%.200s() takes exactly one argument (%zd given)",
-                f->m_ml->ml_name, size);
+
+            res = (*meth)(self, NULL);
+            break;
+
+        case METH_O:
+            size = PyTuple_GET_SIZE(args);
+            if (size != 1) {
+                PyErr_Format(PyExc_TypeError,
+                    "%.200s() takes exactly one argument (%zd given)",
+                    f->m_ml->ml_name, size);
+                return NULL;
+            }
+
+            arg = PyTuple_GET_ITEM(args, 0);
+            res = (*meth)(self, arg);
+            break;
+
+        default:
+            PyErr_SetString(PyExc_SystemError,
+                            "Bad call flags in PyCFunction_Call. "
+                            "METH_OLDARGS is no longer supported!");
             return NULL;
         }
-        break;
-    default:
-        PyErr_SetString(PyExc_SystemError, "Bad call flags in "
-                        "PyCFunction_Call. METH_OLDARGS is no "
-                        "longer supported!");
-
-        return NULL;
     }
-    PyErr_Format(PyExc_TypeError, "%.200s() takes no keyword arguments",
-                 f->m_ml->ml_name);
-    return NULL;
 
-#undef CHECK_RESULT
+    return _Py_CheckFunctionResult(res, "PyCFunction_Call");
 }
 
 /* Methods (the standard built-in methods, that is) */
diff --git a/Python/ceval.c b/Python/ceval.c
index 5cefdcf..1c6089d 100644
--- a/Python/ceval.c
+++ b/Python/ceval.c
@@ -3192,8 +3192,7 @@ fast_block_end:
     if (why != WHY_RETURN)
         retval = NULL;
 
-    assert((retval != NULL && !PyErr_Occurred())
-            || (retval == NULL && PyErr_Occurred()));
+    assert((retval != NULL) ^ (PyErr_Occurred() != NULL));
 
 fast_yield:
     if (co->co_flags & CO_GENERATOR) {
@@ -3254,7 +3253,7 @@ exit_eval_frame:
     f->f_executing = 0;
     tstate->frame = f->f_back;
 
-    return retval;
+    return _Py_CheckFunctionResult(retval, "PyEval_EvalFrameEx");
 }
 
 static void
@@ -4119,13 +4118,6 @@ PyEval_CallObjectWithKeywords(PyObject *func, PyObject *arg, PyObject *kw)
 {
     PyObject *result;
 
-#ifdef Py_DEBUG
-    /* PyEval_CallObjectWithKeywords() must not be called with an exception
-       set, because it may clear it (directly or indirectly)
-       and so the caller looses its exception */
-    assert(!PyErr_Occurred());
-#endif
-
     if (arg == NULL) {
         arg = PyTuple_New(0);
         if (arg == NULL)
@@ -4149,8 +4141,6 @@ PyEval_CallObjectWithKeywords(PyObject *func, PyObject *arg, PyObject *kw)
     result = PyObject_Call(func, arg, kw);
     Py_DECREF(arg);
 
-    assert((result != NULL && !PyErr_Occurred())
-           || (result == NULL && PyErr_Occurred()));
     return result;
 }
 
@@ -4253,11 +4243,15 @@ call_function(PyObject ***pp_stack, int oparg
             PyObject *self = PyCFunction_GET_SELF(func);
             if (flags & METH_NOARGS && na == 0) {
                 C_TRACE(x, (*meth)(self,NULL));
+
+                x = _Py_CheckFunctionResult(x, "call_function");
             }
             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");
             }
             else {
                 err_args(func, flags, na);
@@ -4277,7 +4271,8 @@ call_function(PyObject ***pp_stack, int oparg
                 x = NULL;
             }
         }
-    } else {
+    }
+    else {
         if (PyMethod_Check(func) && PyMethod_GET_SELF(func) != NULL) {
             /* optimize access to bound methods */
             PyObject *self = PyMethod_GET_SELF(func);
@@ -4299,9 +4294,9 @@ call_function(PyObject ***pp_stack, int oparg
             x = do_call(func, pp_stack, na, nk);
         READ_TIMESTAMP(*pintr1);
         Py_DECREF(func);
+
+        assert((x != NULL) ^ (PyErr_Occurred() != NULL));
     }
-    assert((x != NULL && !PyErr_Occurred())
-           || (x == NULL && PyErr_Occurred()));
 
     /* Clear the stack of the function object.  Also removes
        the arguments in case they weren't consumed already
@@ -4313,8 +4308,7 @@ call_function(PyObject ***pp_stack, int oparg
         PCALL(PCALL_POP);
     }
 
-    assert((x != NULL && !PyErr_Occurred())
-           || (x == NULL && PyErr_Occurred()));
+    assert((x != NULL) ^ (PyErr_Occurred() != NULL));
     return x;
 }
 
@@ -4601,8 +4595,6 @@ ext_call_fail:
     Py_XDECREF(callargs);
     Py_XDECREF(kwdict);
     Py_XDECREF(stararg);
-    assert((result != NULL && !PyErr_Occurred())
-           || (result == NULL && PyErr_Occurred()));
     return result;
 }