Issue #28925: cPickle now correctly propagates errors when unpickle instances

of old-style classes.

Lib/test/pickletester.py

@@ -147,6 +147,17 @@ class E(C):
 class H(object):
     pass
 
+class MyErr(Exception):
+    def __init__(self):
+        pass
+
+class I:
+    def __init__(self, *args, **kwargs):
+        raise MyErr()
+
+    def __getinitargs__(self):
+        return ()
+
 # Hashable mutable key
 class K(object):
     def __init__(self, value):
@@ -165,6 +176,8 @@ __main__.E = E
 E.__module__ = "__main__"
 __main__.H = H
 H.__module__ = "__main__"
+__main__.I = I
+I.__module__ = "__main__"
 __main__.K = K
 K.__module__ = "__main__"
 
@@ -623,6 +636,36 @@ class AbstractUnpickleTests(unittest.TestCase):
                        'q\x00oq\x01}q\x02b.').replace('X', xname)
             self.assert_is_copy(X(*args), self.loads(pickle2))
 
+    def test_load_classic_instance_error(self):
+        # Issue #28925.
+        # Protocol 0 (text mode pickle):
+        """
+         0: (    MARK
+         1: i        INST       '__main__ I' (MARK at 0)
+        13: (    MARK
+        14: d        DICT       (MARK at 13)
+        15: b    BUILD
+        16: .    STOP
+        """
+        pickle0 = ('(i__main__\n'
+                   'I\n'
+                   '(db.')
+        self.assertRaises(MyErr, self.loads, pickle0)
+
+        # Protocol 1 (binary mode pickle)
+        """
+         0: (    MARK
+         1: c        GLOBAL     '__main__ I'
+        13: o        OBJ        (MARK at 0)
+        14: }    EMPTY_DICT
+        15: b    BUILD
+        16: .    STOP
+        """
+        pickle1 = ('(c__main__\n'
+                   'I\n'
+                   'o}b.')
+        self.assertRaises(MyErr, self.loads, pickle1)
+
     def test_load_str(self):
         # From Python 2: pickle.dumps('a\x00\xa0', protocol=0)
         self.assertEqual(self.loads("S'a\\x00\\xa0'\n."), 'a\x00\xa0')

Misc/NEWS

@@ -13,6 +13,10 @@ Core and Builtins
 Library
 -------
 
+- Issue #28925: cPickle now correctly propagates errors when unpickle instances
+  of old-style classes.
+
+
 What's New in Python 2.7.13
 ===========================
 

Modules/cPickle.c

@@ -3875,52 +3875,22 @@ load_dict(Unpicklerobject *self)
 static PyObject *
 Instance_New(PyObject *cls, PyObject *args)
 {
-    PyObject *r = 0;
-
     if (PyClass_Check(cls)) {
         int l;
 
-        if ((l=PyObject_Size(args)) < 0) goto err;
+        if ((l=PyObject_Size(args)) < 0) return NULL;
         if (!( l ))  {
-            PyObject *__getinitargs__;
-
-            __getinitargs__ = PyObject_GetAttr(cls,
-                                       __getinitargs___str);
-            if (!__getinitargs__)  {
+            if (!PyObject_HasAttr(cls, __getinitargs___str))  {
                 /* We have a class with no __getinitargs__,
                    so bypass usual construction  */
-                PyObject *inst;
-
-                PyErr_Clear();
-                if (!( inst=PyInstance_NewRaw(cls, NULL)))
-                    goto err;
-                return inst;
+                return PyInstance_NewRaw(cls, NULL);
             }
-            Py_DECREF(__getinitargs__);
         }
 
-        if ((r=PyInstance_New(cls, args, NULL))) return r;
-        else goto err;
+        return PyInstance_New(cls, args, NULL);
     }
 
-    if ((r=PyObject_CallObject(cls, args))) return r;
-
-  err:
-    {
-        PyObject *tp, *v, *tb, *tmp_value;
-
-        PyErr_Fetch(&tp, &v, &tb);
-        tmp_value = v;
-        /* NULL occurs when there was a KeyboardInterrupt */
-        if (tmp_value == NULL)
-            tmp_value = Py_None;
-        if ((r = PyTuple_Pack(3, tmp_value, cls, args))) {
-            Py_XDECREF(v);
-            v=r;
-        }
-        PyErr_Restore(tp,v,tb);
-    }
-    return NULL;
+    return PyObject_CallObject(cls, args);
 }