Issue #23914: Fixed SystemError raised by unpickler on broken pickle data.

4 files changed, 105 insertions, 4 deletions

diff --git a/Lib/test/pickletester.py b/Lib/test/pickletester.py
index e852b01..cd2423f 100644
--- a/Lib/test/pickletester.py
+++ b/Lib/test/pickletester.py
@@ -12,7 +12,7 @@ import weakref
 from http.cookies import SimpleCookie
 
 from test.support import (
-    TestFailed, TESTFN, run_with_locale, no_tracing,
+    TestFailed, TESTFN, run_with_locale, no_tracing, captured_stdout,
     _2G, _4G, bigmemtest,
     )
 
@@ -987,6 +987,89 @@ class AbstractUnpickleTests(unittest.TestCase):
             self.assertIs(type(unpickled), collections.UserDict)
             self.assertEqual(unpickled, collections.UserDict({1: 2}))
 
+    def test_bad_stack(self):
+        badpickles = [
+            b'0.',              # POP
+            b'1.',              # POP_MARK
+            b'2.',              # DUP
+            # b'(2.',           # PyUnpickler doesn't raise
+            b'R.',              # REDUCE
+            b')R.',
+            b'a.',              # APPEND
+            b'Na.',
+            b'b.',              # BUILD
+            b'Nb.',
+            b'd.',              # DICT
+            b'e.',              # APPENDS
+            # b'(e.',           # PyUnpickler raises AttributeError
+            b'ibuiltins\nlist\n.',  # INST
+            b'l.',              # LIST
+            b'o.',              # OBJ
+            b'(o.',
+            b'p1\n.',           # PUT
+            b'q\x00.',          # BINPUT
+            b'r\x00\x00\x00\x00.',  # LONG_BINPUT
+            b's.',              # SETITEM
+            b'Ns.',
+            b'NNs.',
+            b't.',              # TUPLE
+            b'u.',              # SETITEMS
+            b'(u.',
+            b'}(Nu.',
+            b'\x81.',           # NEWOBJ
+            b')\x81.',
+            b'\x85.',           # TUPLE1
+            b'\x86.',           # TUPLE2
+            b'N\x86.',
+            b'\x87.',           # TUPLE3
+            b'N\x87.',
+            b'NN\x87.',
+            b'\x90.',           # ADDITEMS
+            # b'(\x90.',        # PyUnpickler raises AttributeError
+            b'\x91.',           # FROZENSET
+            b'\x92.',           # NEWOBJ_EX
+            b')}\x92.',
+            b'\x93.',           # STACK_GLOBAL
+            b'Vlist\n\x93.',
+            b'\x94.',           # MEMOIZE
+        ]
+        for p in badpickles:
+            with self.subTest(p):
+                self.assertRaises(self.bad_stack_errors, self.loads, p)
+
+    def test_bad_mark(self):
+        badpickles = [
+            b'cbuiltins\nlist\n)(R.',           # REDUCE
+            b'cbuiltins\nlist\n()R.',
+            b']N(a.',                           # APPEND
+            b'cbuiltins\nValueError\n)R}(b.',   # BUILD
+            b'cbuiltins\nValueError\n)R(}b.',
+            b'(Nd.',                            # DICT
+            b'}NN(s.',                          # SETITEM
+            b'}N(Ns.',
+            b'cbuiltins\nlist\n)(\x81.',        # NEWOBJ
+            b'cbuiltins\nlist\n()\x81.',
+            b'N(\x85.',                         # TUPLE1
+            b'NN(\x86.',                        # TUPLE2
+            b'N(N\x86.',
+            b'NNN(\x87.',                       # TUPLE3
+            b'NN(N\x87.',
+            b'N(NN\x87.',
+            b'cbuiltins\nlist\n)}(\x92.',       # NEWOBJ_EX
+            b'cbuiltins\nlist\n)(}\x92.',
+            b'cbuiltins\nlist\n()}\x92.',
+            b'Vbuiltins\n(Vlist\n\x93.',        # STACK_GLOBAL
+            b'Vbuiltins\nVlist\n(\x93.',
+        ]
+        for p in badpickles:
+            # PyUnpickler prints reduce errors to stdout
+            with self.subTest(p), captured_stdout():
+                try:
+                    self.loads(p)
+                except (IndexError, AttributeError, TypeError,
+                        pickle.UnpicklingError):
+                    pass
+
 
 class AbstractPickleTests(unittest.TestCase):
     # Subclass must define self.dumps, self.loads.
diff --git a/Lib/test/test_pickle.py b/Lib/test/test_pickle.py
index 8dc93d2..f04c5ac 100644
--- a/Lib/test/test_pickle.py
+++ b/Lib/test/test_pickle.py
@@ -32,6 +32,7 @@ class PickleTests(AbstractPickleModuleTests):
 class PyUnpicklerTests(AbstractUnpickleTests):
 
     unpickler = pickle._Unpickler
+    bad_stack_errors = (IndexError,)
 
     def loads(self, buf, **kwds):
         f = io.BytesIO(buf)
@@ -62,6 +63,7 @@ class InMemoryPickleTests(AbstractPickleTests, AbstractUnpickleTests,
 
     pickler = pickle._Pickler
     unpickler = pickle._Unpickler
+    bad_stack_errors = (pickle.UnpicklingError, IndexError)
 
     def dumps(self, arg, protocol=None):
         return pickle.dumps(arg, protocol)
@@ -119,6 +121,7 @@ class PyChainDispatchTableTests(AbstractDispatchTableTests):
 if has_c_implementation:
     class CUnpicklerTests(PyUnpicklerTests):
         unpickler = _pickle.Unpickler
+        bad_stack_errors = (pickle.UnpicklingError,)
 
     class CPicklerTests(PyPicklerTests):
         pickler = _pickle.Pickler
diff --git a/Misc/NEWS b/Misc/NEWS
index 635da46..dee8338 100644
--- a/Misc/NEWS
+++ b/Misc/NEWS
@@ -106,6 +106,8 @@ Core and Builtins
 Library
 -------
 
+- Issue #23914: Fixed SystemError raised by unpickler on broken pickle data.
+
 - Issue #25691: Fixed crash on deleting ElementTree.Element attributes.
 
 - Issue #25624: ZipFile now always writes a ZIP_STORED header for directory
diff --git a/Modules/_pickle.c b/Modules/_pickle.c
index 8a98f51..d3bc420 100644
--- a/Modules/_pickle.c
+++ b/Modules/_pickle.c
@@ -448,8 +448,8 @@ Pdata_grow(Pdata *self)
 static PyObject *
 Pdata_pop(Pdata *self)
 {
-    PickleState *st = _Pickle_GetGlobalState();
     if (Py_SIZE(self) == 0) {
+        PickleState *st = _Pickle_GetGlobalState();
         PyErr_SetString(st->UnpicklingError, "bad pickle data");
         return NULL;
     }
@@ -5079,6 +5079,9 @@ load_obj(UnpicklerObject *self)
     if ((i = marker(self)) < 0)
         return -1;
 
+    if (Py_SIZE(self->stack) - i < 1)
+        return stack_underflow();
+
     args = Pdata_poptuple(self->stack, i + 1);
     if (args == NULL)
         return -1;
@@ -5737,13 +5740,18 @@ do_append(UnpicklerObject *self, Py_ssize_t x)
 static int
 load_append(UnpicklerObject *self)
 {
+    if (Py_SIZE(self->stack) - 1 <= 0)
+        return stack_underflow();
     return do_append(self, Py_SIZE(self->stack) - 1);
 }
 
 static int
 load_appends(UnpicklerObject *self)
 {
-    return do_append(self, marker(self));
+    Py_ssize_t i = marker(self);
+    if (i < 0)
+        return -1;
+    return do_append(self, i);
 }
 
 static int
@@ -5793,7 +5801,10 @@ load_setitem(UnpicklerObject *self)
 static int
 load_setitems(UnpicklerObject *self)
 {
-    return do_setitems(self, marker(self));
+    Py_ssize_t i = marker(self);
+    if (i < 0)
+        return -1;
+    return do_setitems(self, i);
 }
 
 static int
@@ -5803,6 +5814,8 @@ load_additems(UnpicklerObject *self)
     Py_ssize_t mark, len, i;
 
     mark =  marker(self);
+    if (mark < 0)
+        return -1;
     len = Py_SIZE(self->stack);
     if (mark > len || mark <= 0)
         return stack_underflow();