From afdb09406715f2685c75e93608d9740c22ba0ccf Mon Sep 17 00:00:00 2001 From: Zackery Spytz Date: Sun, 6 Oct 2019 07:40:23 -0600 Subject: [PATCH 1/9] bpo-38384: Fix a possible assertion failure in _pickle In _Unpickler_SetInputStream(), _PyObject_LookupAttrId() is called three times in a row without any error handling. If an exception occurs during the first or second call, _PyObject_LookupAttrId() will be called with a live exception. --- Modules/_pickle.c | 33 +++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/Modules/_pickle.c b/Modules/_pickle.c index 7370be5938831a..299ac2f8a17129 100644 --- a/Modules/_pickle.c +++ b/Modules/_pickle.c @@ -1609,22 +1609,27 @@ _Unpickler_SetInputStream(UnpicklerObject *self, PyObject *file) if (_PyObject_LookupAttrId(file, &PyId_peek, &self->peek) < 0) { return -1; } - (void)_PyObject_LookupAttrId(file, &PyId_read, &self->read); - (void)_PyObject_LookupAttrId(file, &PyId_readinto, &self->readinto); - (void)_PyObject_LookupAttrId(file, &PyId_readline, &self->readline); - if (!self->readline || !self->readinto || !self->read) { - if (!PyErr_Occurred()) { - PyErr_SetString(PyExc_TypeError, - "file must have 'read', 'readinto' and " - "'readline' attributes"); - } - Py_CLEAR(self->read); - Py_CLEAR(self->readinto); - Py_CLEAR(self->readline); - Py_CLEAR(self->peek); - return -1; + if (_PyObject_LookupAttrId(file, &PyId_read, &self->read) <= 0) { + goto error; + } + if (_PyObject_LookupAttrId(file, &PyId_readinto, &self->readinto) <= 0) { + goto error; + } + if (_PyObject_LookupAttrId(file, &PyId_readline, &self->readline) <= 0) { + goto error; } return 0; +error: + if (!PyErr_Occurred()) { + PyErr_SetString(PyExc_TypeError, + "file must have 'read', 'readinto' and " + "'readline' attributes"); + } + Py_CLEAR(self->read); + Py_CLEAR(self->readinto); + Py_CLEAR(self->readline); + Py_CLEAR(self->peek); + return -1; } /* Returns -1 (with an exception set) on failure, 0 on success. This may From b81c706a59baeea2dc7ea761864dc1c234bddb12 Mon Sep 17 00:00:00 2001 From: Zackery Spytz Date: Fri, 22 Nov 2019 11:50:30 -0700 Subject: [PATCH 2/9] Add a test. --- Lib/test/pickletester.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/Lib/test/pickletester.py b/Lib/test/pickletester.py index c9f374678ae35a..eb05cdcd9e37b4 100644 --- a/Lib/test/pickletester.py +++ b/Lib/test/pickletester.py @@ -3136,6 +3136,18 @@ def __init__(self): pass self.assertRaises(pickle.PicklingError, BadPickler().dump, 0) self.assertRaises(pickle.UnpicklingError, BadUnpickler().load) + def test_load_read_exception(self): + class F: + @property + def read(self): + 1/0 + def readinto(self): + 1/0 + def readline(self): + 1/0 + + self.assertRaises(ZeroDivisionError, pickle.load, F()) + def check_dumps_loads_oob_buffers(self, dumps, loads): # No need to do the full gamut of tests here, just enough to # check that dumps() and loads() redirect their arguments From ccaeb979b87c15fc0d570217e60ea07397e9159c Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Wed, 20 Oct 2021 21:58:13 +0300 Subject: [PATCH 3/9] Rewrite test. --- Lib/test/pickletester.py | 58 +++++++++++++++++++++++++++++++++------- 1 file changed, 48 insertions(+), 10 deletions(-) diff --git a/Lib/test/pickletester.py b/Lib/test/pickletester.py index ba63f0a5d9a5b1..be59c3219a3215 100644 --- a/Lib/test/pickletester.py +++ b/Lib/test/pickletester.py @@ -3370,17 +3370,55 @@ def __init__(self): pass self.assertRaises(pickle.PicklingError, BadPickler().dump, 0) self.assertRaises(pickle.UnpicklingError, BadUnpickler().load) - def test_load_read_exception(self): + def test_bad_file(self): + # bpo-38384: Crash in _pickle if the read attribute raises error. + def raises_oserror(self, *args, **kwargs): + raise OSError + @property + def bad_property(self): + 1/0 + + # File without read + class F: + readline = raises_oserror + self.assertRaises((AttributeError, TypeError), self.Unpickler, F()) + + # File without readline + class F: + read = raises_oserror + self.assertRaises((AttributeError, TypeError), self.Unpickler, F()) + + # File with bad read + class F: + read = bad_property + readline = raises_oserror + self.assertRaises(ZeroDivisionError, self.Unpickler, F()) + + # File with bad readline class F: - @property - def read(self): - 1/0 - def readinto(self): - 1/0 - def readline(self): - 1/0 - - self.assertRaises(ZeroDivisionError, pickle.load, F()) + readline = bad_property + read = raises_oserror + self.assertRaises(ZeroDivisionError, self.Unpickler, F()) + + # File with bad peek + class F: + peek = bad_property + read = raises_oserror + readline = raises_oserror + try: + self.Unpickler(F()) + except ZeroDivisionError: + pass + + # File with bad readinto + class F: + readinto = bad_property + read = raises_oserror + readline = raises_oserror + try: + self.Unpickler(F()) + except ZeroDivisionError: + pass def check_dumps_loads_oob_buffers(self, dumps, loads): # No need to do the full gamut of tests here, just enough to From bf18cec625cc0f93e15bdc1ab8a437d6fc9489ee Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Wed, 20 Oct 2021 22:06:56 +0300 Subject: [PATCH 4/9] Add a NEWS entry and a test for pickler --- Lib/test/pickletester.py | 15 ++++++++++++++- .../2021-10-20-22-02-23.bpo-38384.7XK1_N.rst | 2 ++ 2 files changed, 16 insertions(+), 1 deletion(-) create mode 100644 Misc/NEWS.d/next/Library/2021-10-20-22-02-23.bpo-38384.7XK1_N.rst diff --git a/Lib/test/pickletester.py b/Lib/test/pickletester.py index be59c3219a3215..a3315a93c30f90 100644 --- a/Lib/test/pickletester.py +++ b/Lib/test/pickletester.py @@ -3370,7 +3370,7 @@ def __init__(self): pass self.assertRaises(pickle.PicklingError, BadPickler().dump, 0) self.assertRaises(pickle.UnpicklingError, BadUnpickler().load) - def test_bad_file(self): + def test_unpickler_bad_file(self): # bpo-38384: Crash in _pickle if the read attribute raises error. def raises_oserror(self, *args, **kwargs): raise OSError @@ -3420,6 +3420,19 @@ class F: except ZeroDivisionError: pass + def test_pickler_bad_file(self): + # File without write + class F: + pass + self.assertRaises(TypeError, self.Pickler, F()) + + # File with bad write + class F: + @property + def write(self): + 1/0 + self.assertRaises(ZeroDivisionError, self.Pickler, F()) + def check_dumps_loads_oob_buffers(self, dumps, loads): # No need to do the full gamut of tests here, just enough to # check that dumps() and loads() redirect their arguments diff --git a/Misc/NEWS.d/next/Library/2021-10-20-22-02-23.bpo-38384.7XK1_N.rst b/Misc/NEWS.d/next/Library/2021-10-20-22-02-23.bpo-38384.7XK1_N.rst new file mode 100644 index 00000000000000..517fca9a916a14 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2021-10-20-22-02-23.bpo-38384.7XK1_N.rst @@ -0,0 +1,2 @@ +Fix a crash in the C implementation of :mod:`pickle` if error occurred when +look up the "read" attribute of the file. From 6e24487e52b7ecf4b455fac5550efe9ed1fe8fdd Mon Sep 17 00:00:00 2001 From: Zackery Spytz Date: Thu, 22 Dec 2022 03:30:39 -0800 Subject: [PATCH 5/9] Modify a test and the news entry (which weren't written by me). --- Lib/test/pickletester.py | 2 +- .../next/Library/2021-10-20-22-02-23.bpo-38384.7XK1_N.rst | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Lib/test/pickletester.py b/Lib/test/pickletester.py index a3315a93c30f90..44bf1eddc0c2a3 100644 --- a/Lib/test/pickletester.py +++ b/Lib/test/pickletester.py @@ -3371,7 +3371,7 @@ def __init__(self): pass self.assertRaises(pickle.UnpicklingError, BadUnpickler().load) def test_unpickler_bad_file(self): - # bpo-38384: Crash in _pickle if the read attribute raises error. + # bpo-38384: Crash in _pickle if the read attribute raises an error. def raises_oserror(self, *args, **kwargs): raise OSError @property diff --git a/Misc/NEWS.d/next/Library/2021-10-20-22-02-23.bpo-38384.7XK1_N.rst b/Misc/NEWS.d/next/Library/2021-10-20-22-02-23.bpo-38384.7XK1_N.rst index 517fca9a916a14..132e945d7dfba1 100644 --- a/Misc/NEWS.d/next/Library/2021-10-20-22-02-23.bpo-38384.7XK1_N.rst +++ b/Misc/NEWS.d/next/Library/2021-10-20-22-02-23.bpo-38384.7XK1_N.rst @@ -1,2 +1,2 @@ -Fix a crash in the C implementation of :mod:`pickle` if error occurred when -look up the "read" attribute of the file. +Fix a crash in the C implementation of :mod:`pickle` if an error occurred +while looking up the ``read`` attribute of the file. From cd5f794da6e540f9fc83b9655493a4a67fc526a0 Mon Sep 17 00:00:00 2001 From: Oleg Iarygin Date: Thu, 20 Apr 2023 22:59:21 +0400 Subject: [PATCH 6/9] Address the comment of Serhiy --- Lib/test/pickletester.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/Lib/test/pickletester.py b/Lib/test/pickletester.py index 4d809c18b5a409..5f44699edcb689 100644 --- a/Lib/test/pickletester.py +++ b/Lib/test/pickletester.py @@ -3490,6 +3490,11 @@ def raises_oserror(self, *args, **kwargs): def bad_property(self): 1/0 + # File without read and readline + class F: + pass + self.assertRaises((AttributeError, TypeError), self.Unpickler, F()) + # File without read class F: readline = raises_oserror @@ -3512,6 +3517,16 @@ class F: read = raises_oserror self.assertRaises(ZeroDivisionError, self.Unpickler, F()) + # File with bad readline, no read + class F: + readline = bad_property + self.assertRaises(ZeroDivisionError, self.Unpickler, F()) + + # File with bad read, no readline + class F: + read = bad_property + self.assertRaises(ZeroDivisionError, self.Unpickler, F()) + # File with bad peek class F: peek = bad_property From dd343e153875f552e22042fa5208d7f8b0b23850 Mon Sep 17 00:00:00 2001 From: Oleg Iarygin Date: Fri, 21 Apr 2023 06:40:08 +0400 Subject: [PATCH 7/9] `_PyObject_LookupAttrId` -> `_PyObject_LookupAttr` --- Modules/_pickle.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Modules/_pickle.c b/Modules/_pickle.c index 9ba6f656ac3481..72e4d89872005e 100644 --- a/Modules/_pickle.c +++ b/Modules/_pickle.c @@ -1678,8 +1678,8 @@ _Unpickler_SetInputStream(UnpicklerObject *self, PyObject *file) Py_CLEAR(self->peek); return -1; } - if (_PyObject_LookupAttrId(file, &_Py_ID(read), &self->read) <= 0 || - _PyObject_LookupAttrId(file, &_Py_ID(readline), &self->readline) <= 0) + if (_PyObject_LookupAttr(file, &_Py_ID(read), &self->read) <= 0 || + _PyObject_LookupAttr(file, &_Py_ID(readline), &self->readline) <= 0) { if (!PyErr_Occurred()) { PyErr_SetString(PyExc_TypeError, From 644a1cf0d2e1e20eff6e206e6dc8f4f80f59383b Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Fri, 1 Dec 2023 14:35:51 +0200 Subject: [PATCH 8/9] Fix tests. --- Lib/test/pickletester.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/test/pickletester.py b/Lib/test/pickletester.py index 4bb93eee2ec6d5..fd446c8145850c 100644 --- a/Lib/test/pickletester.py +++ b/Lib/test/pickletester.py @@ -3557,7 +3557,7 @@ class F: # File with bad read, no readline class F: read = bad_property - self.assertRaises(ZeroDivisionError, self.Unpickler, F()) + self.assertRaises((AttributeError, ZeroDivisionError), self.Unpickler, F()) # File with bad peek class F: From 57f6095632b6454d26a2d4b29d72a317b54d6ee9 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Fri, 1 Dec 2023 14:36:12 +0200 Subject: [PATCH 9/9] Remove a NEWS entry. --- .../next/Library/2021-10-20-22-02-23.bpo-38384.7XK1_N.rst | 2 -- 1 file changed, 2 deletions(-) delete mode 100644 Misc/NEWS.d/next/Library/2021-10-20-22-02-23.bpo-38384.7XK1_N.rst diff --git a/Misc/NEWS.d/next/Library/2021-10-20-22-02-23.bpo-38384.7XK1_N.rst b/Misc/NEWS.d/next/Library/2021-10-20-22-02-23.bpo-38384.7XK1_N.rst deleted file mode 100644 index 132e945d7dfba1..00000000000000 --- a/Misc/NEWS.d/next/Library/2021-10-20-22-02-23.bpo-38384.7XK1_N.rst +++ /dev/null @@ -1,2 +0,0 @@ -Fix a crash in the C implementation of :mod:`pickle` if an error occurred -while looking up the ``read`` attribute of the file.