Issue #26168: Fixed possible refleaks in failing Py_BuildValue() with the "N"

format unit.

Lib/test/test_capi.py

@@ -238,6 +238,9 @@ class CAPITest(unittest.TestCase):
                              'return_result_with_error.* '
                              'returned a result with an error set')
 
+    def test_buildvalue_N(self):
+        _testcapi.test_buildvalue_N()
+
 
 @unittest.skipUnless(threading, 'Threading required for this test.')
 class TestPendingCalls(unittest.TestCase):

Misc/NEWS

@@ -41,6 +41,9 @@ Release date: 2016-05-16
 Core and Builtins
 -----------------
 
+- Issue #26168: Fixed possible refleaks in failing Py_BuildValue() with the "N"
+  format unit.
+
 - Issue #26991: Fix possible refleak when creating a function with annotations.
 
 - Issue #27039: Fixed bytearray.remove() for values greater than 127.  Based on

Modules/_testcapimodule.c

@@ -872,6 +872,100 @@ test_L_code(PyObject *self)
 
 #endif  /* ifdef HAVE_LONG_LONG */
 
+static PyObject *
+return_none(void *unused)
+{
+    Py_RETURN_NONE;
+}
+
+static PyObject *
+raise_error(void *unused)
+{
+    PyErr_SetNone(PyExc_ValueError);
+    return NULL;
+}
+
+static int
+test_buildvalue_N_error(const char *fmt)
+{
+    PyObject *arg, *res;
+
+    arg = PyList_New(0);
+    if (arg == NULL) {
+        return -1;
+    }
+
+    Py_INCREF(arg);
+    res = Py_BuildValue(fmt, return_none, NULL, arg);
+    if (res == NULL) {
+        return -1;
+    }
+    Py_DECREF(res);
+    if (Py_REFCNT(arg) != 1) {
+        PyErr_Format(TestError, "test_buildvalue_N: "
+                     "arg was not decrefed in successful "
+                     "Py_BuildValue(\"%s\")", fmt);
+        return -1;
+    }
+
+    Py_INCREF(arg);
+    res = Py_BuildValue(fmt, raise_error, NULL, arg);
+    if (res != NULL || !PyErr_Occurred()) {
+        PyErr_Format(TestError, "test_buildvalue_N: "
+                     "Py_BuildValue(\"%s\") didn't complain", fmt);
+        return -1;
+    }
+    PyErr_Clear();
+    if (Py_REFCNT(arg) != 1) {
+        PyErr_Format(TestError, "test_buildvalue_N: "
+                     "arg was not decrefed in failed "
+                     "Py_BuildValue(\"%s\")", fmt);
+        return -1;
+    }
+    Py_DECREF(arg);
+    return 0;
+}
+
+static PyObject *
+test_buildvalue_N(PyObject *self, PyObject *noargs)
+{
+    PyObject *arg, *res;
+
+    arg = PyList_New(0);
+    if (arg == NULL) {
+        return NULL;
+    }
+    Py_INCREF(arg);
+    res = Py_BuildValue("N", arg);
+    if (res == NULL) {
+        return NULL;
+    }
+    if (res != arg) {
+        return raiseTestError("test_buildvalue_N",
+                              "Py_BuildValue(\"N\") returned wrong result");
+    }
+    if (Py_REFCNT(arg) != 2) {
+        return raiseTestError("test_buildvalue_N",
+                              "arg was not decrefed in Py_BuildValue(\"N\")");
+    }
+    Py_DECREF(res);
+    Py_DECREF(arg);
+
+    if (test_buildvalue_N_error("O&N") < 0)
+        return NULL;
+    if (test_buildvalue_N_error("(O&N)") < 0)
+        return NULL;
+    if (test_buildvalue_N_error("[O&N]") < 0)
+        return NULL;
+    if (test_buildvalue_N_error("{O&N}") < 0)
+        return NULL;
+    if (test_buildvalue_N_error("{()O&(())N}") < 0)
+        return NULL;
+
+    Py_RETURN_NONE;
+}
+
+
 static PyObject *
 get_args(PyObject *self, PyObject *args)
 {
@@ -3861,6 +3955,7 @@ static PyMethodDef TestMethods[] = {
     {"test_pep3118_obsolete_write_locks", (PyCFunction)test_pep3118_obsolete_write_locks, METH_NOARGS},
 #endif
     {"getbuffer_with_null_view", getbuffer_with_null_view, METH_O},
+    {"test_buildvalue_N",       test_buildvalue_N,               METH_NOARGS},
     {"get_args", get_args, METH_VARARGS},
     {"get_kwargs", (PyCFunction)get_kwargs, METH_VARARGS|METH_KEYWORDS},
     {"getargs_tuple",           getargs_tuple,                   METH_VARARGS},

Python/modsupport.c

@@ -63,48 +63,84 @@ static PyObject *do_mkdict(const char**, va_list *, int, int, int);
 static PyObject *do_mkvalue(const char**, va_list *, int);
 
 
+static void
+do_ignore(const char **p_format, va_list *p_va, int endchar, int n, int flags)
+{
+    PyObject *v;
+    int i;
+    assert(PyErr_Occurred());
+    v = PyTuple_New(n);
+    for (i = 0; i < n; i++) {
+        PyObject *exception, *value, *tb, *w;
+
+        PyErr_Fetch(&exception, &value, &tb);
+        w = do_mkvalue(p_format, p_va, flags);
+        PyErr_Restore(exception, value, tb);
+        if (w != NULL) {
+            if (v != NULL) {
+                PyTuple_SET_ITEM(v, i, w);
+            }
+            else {
+                Py_DECREF(w);
+            }
+        }
+    }
+    Py_XDECREF(v);
+    if (**p_format != endchar) {
+        PyErr_SetString(PyExc_SystemError,
+                        "Unmatched paren in format");
+        return;
+    }
+    if (endchar)
+        ++*p_format;
+}
+
 static PyObject *
 do_mkdict(const char **p_format, va_list *p_va, int endchar, int n, int flags)
 {
     PyObject *d;
     int i;
-    int itemfailed = 0;
     if (n < 0)
         return NULL;
-    if ((d = PyDict_New()) == NULL)
+    if (n % 2) {
+        PyErr_SetString(PyExc_SystemError,
+                        "Bad dict format");
+        do_ignore(p_format, p_va, endchar, n, flags);
         return NULL;
+    }
     /* Note that we can't bail immediately on error as this will leak
        refcounts on any 'N' arguments. */
+    if ((d = PyDict_New()) == NULL) {
+        do_ignore(p_format, p_va, endchar, n, flags);
+        return NULL;
+    }
     for (i = 0; i < n; i+= 2) {
         PyObject *k, *v;
-        int err;
+
         k = do_mkvalue(p_format, p_va, flags);
         if (k == NULL) {
-            itemfailed = 1;
+            do_ignore(p_format, p_va, endchar, n - i - 1, flags);
-            Py_INCREF(Py_None);
-            k = Py_None;
-        }
-        v = do_mkvalue(p_format, p_va, flags);
-        if (v == NULL) {
-            itemfailed = 1;
-            Py_INCREF(Py_None);
-            v = Py_None;
-        }
-        err = PyDict_SetItem(d, k, v);
-        Py_DECREF(k);
-        Py_DECREF(v);
-        if (err < 0 || itemfailed) {
             Py_DECREF(d);
             return NULL;
         }
+        v = do_mkvalue(p_format, p_va, flags);
+        if (v == NULL || PyDict_SetItem(d, k, v) < 0) {
+            do_ignore(p_format, p_va, endchar, n - i - 2, flags);
+            Py_DECREF(k);
+            Py_XDECREF(v);
+            Py_DECREF(d);
+            return NULL;
+        }
+        Py_DECREF(k);
+        Py_DECREF(v);
     }
-    if (d != NULL && **p_format != endchar) {
+    if (**p_format != endchar) {
         Py_DECREF(d);
-        d = NULL;
         PyErr_SetString(PyExc_SystemError,
                         "Unmatched paren in format");
+        return NULL;
     }
-    else if (endchar)
+    if (endchar)
         ++*p_format;
     return d;
 }
@@ -114,29 +150,24 @@ do_mklist(const char **p_format, va_list *p_va, int endchar, int n, int flags)
 {
     PyObject *v;
     int i;
-    int itemfailed = 0;
     if (n < 0)
         return NULL;
-    v = PyList_New(n);
-    if (v == NULL)
-        return NULL;
     /* Note that we can't bail immediately on error as this will leak
        refcounts on any 'N' arguments. */
+    v = PyList_New(n);
+    if (v == NULL) {
+        do_ignore(p_format, p_va, endchar, n, flags);
+        return NULL;
+    }
     for (i = 0; i < n; i++) {
         PyObject *w = do_mkvalue(p_format, p_va, flags);
         if (w == NULL) {
-            itemfailed = 1;
+            do_ignore(p_format, p_va, endchar, n - i - 1, flags);
-            Py_INCREF(Py_None);
+            Py_DECREF(v);
-            w = Py_None;
+            return NULL;
         }
         PyList_SET_ITEM(v, i, w);
     }
-
-    if (itemfailed) {
-        /* do_mkvalue() should have already set an error */
-        Py_DECREF(v);
-        return NULL;
-    }
     if (**p_format != endchar) {
         Py_DECREF(v);
         PyErr_SetString(PyExc_SystemError,
@@ -153,37 +184,23 @@ do_mktuple(const char **p_format, va_list *p_va, int endchar, int n, int flags)
 {
     PyObject *v;
     int i;
-    int itemfailed = 0;
     if (n < 0)
         return NULL;
-    if ((v = PyTuple_New(n)) == NULL)
-        return NULL;
     /* Note that we can't bail immediately on error as this will leak
        refcounts on any 'N' arguments. */
+    if ((v = PyTuple_New(n)) == NULL) {
+        do_ignore(p_format, p_va, endchar, n, flags);
+        return NULL;
+    }
     for (i = 0; i < n; i++) {
-        PyObject *w;
+        PyObject *w = do_mkvalue(p_format, p_va, flags);
-
-        if (itemfailed) {
-            PyObject *exception, *value, *tb;
-            PyErr_Fetch(&exception, &value, &tb);
-            w = do_mkvalue(p_format, p_va, flags);
-            PyErr_Restore(exception, value, tb);
-        }
-        else {
-            w = do_mkvalue(p_format, p_va, flags);
-        }
         if (w == NULL) {
-            itemfailed = 1;
+            do_ignore(p_format, p_va, endchar, n - i - 1, flags);
-            Py_INCREF(Py_None);
+            Py_DECREF(v);
-            w = Py_None;
+            return NULL;
         }
         PyTuple_SET_ITEM(v, i, w);
     }
-    if (itemfailed) {
-        /* do_mkvalue() should have already set an error */
-        Py_DECREF(v);
-        return NULL;
-    }
     if (**p_format != endchar) {
         Py_DECREF(v);
         PyErr_SetString(PyExc_SystemError,