From e9b3074cf931fee3cb91638cbdd694fa9eb9614b Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Mon, 23 Nov 2015 15:17:43 +0200 Subject: [PATCH] Issue #23914: Fixed SystemError raised by unpickler on broken pickle data. --- Lib/test/pickletester.py | 85 +++++++++++++++++++++++++++++++++++++++- Lib/test/test_pickle.py | 3 ++ Misc/NEWS | 2 + Modules/_pickle.c | 19 +++++++-- 4 files changed, 105 insertions(+), 4 deletions(-) diff --git a/Lib/test/pickletester.py b/Lib/test/pickletester.py index e852b011b5c..cd2423f1550 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 8dc93d2a5bf..f04c5ac7083 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 635da46f022..dee83382363 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 8a98f512e32..d3bc4200961 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();