From 9b514d5666d0fb339670dbbb71b7aac1d4a9cb85 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Thu, 8 Jun 2023 00:21:22 +0200 Subject: [PATCH 1/5] gh-105375: Improve _pickle error handling Error handling was deferred in some cases, which could potentially lead to exceptions being overwritten. --- Modules/_pickle.c | 37 +++++++++++++++++++++++++------------ 1 file changed, 25 insertions(+), 12 deletions(-) diff --git a/Modules/_pickle.c b/Modules/_pickle.c index 418eea20206c25..5b3665d11edde3 100644 --- a/Modules/_pickle.c +++ b/Modules/_pickle.c @@ -1168,10 +1168,13 @@ _Pickler_New(PickleState *st) self->reducer_override = NULL; self->memo = PyMemoTable_New(); + if (self->memo == NULL) { + Py_DECREF(self); + return NULL; + } self->output_buffer = PyBytes_FromStringAndSize(NULL, self->max_output_len); - - if (self->memo == NULL || self->output_buffer == NULL) { + if (self->output_buffer == NULL) { Py_DECREF(self); return NULL; } @@ -1654,9 +1657,12 @@ _Unpickler_New(PyObject *module) self->memo_size = 32; self->memo_len = 0; self->memo = _Unpickler_NewMemo(self->memo_size); + if (self->memo == NULL) { + Py_DECREF(self); + return NULL; + } self->stack = (Pdata *)Pdata_New(st); - - if (self->memo == NULL || self->stack == NULL) { + if (self->stack == NULL) { Py_DECREF(self); return NULL; } @@ -4828,11 +4834,12 @@ _pickle_PicklerMemoProxy_copy_impl(PicklerMemoProxyObject *self) PyObject *key, *value; key = PyLong_FromVoidPtr(entry.me_key); + if (key == NULL) { + goto error; + } value = Py_BuildValue("nO", entry.me_value, entry.me_key); - - if (key == NULL || value == NULL) { - Py_XDECREF(key); - Py_XDECREF(value); + if (value == NULL) { + Py_DECREF(key); goto error; } status = PyDict_SetItem(new_memo, key, value); @@ -5988,12 +5995,18 @@ load_stack_global(PickleState *st, UnpicklerObject *self) PyObject *global_name; PDATA_POP(st, self->stack, global_name); + if (global_name == NULL) { + return -1; + } PDATA_POP(st, self->stack, module_name); - if (module_name == NULL || !PyUnicode_CheckExact(module_name) || - global_name == NULL || !PyUnicode_CheckExact(global_name)) { + if (module_name == NULL) { + Py_DECREF(global_name); + return -1; + } + if (!PyUnicode_CheckExact(module_name) || + !PyUnicode_CheckExact(global_name)) + { PyErr_SetString(st->UnpicklingError, "STACK_GLOBAL requires str"); - Py_XDECREF(global_name); - Py_XDECREF(module_name); return -1; } global = find_class(self, module_name, global_name); From a91713a110adf630b6f1c750739a9786e442d912 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Thu, 8 Jun 2023 00:24:51 +0200 Subject: [PATCH 2/5] Bail early from _Unpickler_SetInputStream() also --- Modules/_pickle.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/Modules/_pickle.c b/Modules/_pickle.c index 5b3665d11edde3..fd58aff69ad67e 100644 --- a/Modules/_pickle.c +++ b/Modules/_pickle.c @@ -1678,25 +1678,25 @@ _Unpickler_SetInputStream(UnpicklerObject *self, PyObject *file) { /* Optional file methods */ if (_PyObject_LookupAttr(file, &_Py_ID(peek), &self->peek) < 0) { - return -1; + goto error; } if (_PyObject_LookupAttr(file, &_Py_ID(readinto), &self->readinto) < 0) { - return -1; + goto error; } - (void)_PyObject_LookupAttr(file, &_Py_ID(read), &self->read); - (void)_PyObject_LookupAttr(file, &_Py_ID(readline), &self->readline); - if (!self->readline || !self->read) { - if (!PyErr_Occurred()) { - PyErr_SetString(PyExc_TypeError, - "file must have 'read' and 'readline' attributes"); - } - Py_CLEAR(self->read); - Py_CLEAR(self->readinto); - Py_CLEAR(self->readline); - Py_CLEAR(self->peek); - return -1; + if (_PyObject_LookupAttr(file, &_Py_ID(read), &self->read) < 0) { + goto error; + } + if (_PyObject_LookupAttr(file, &_Py_ID(readline), &self->readline) < 0) { + goto error; } return 0; + +error: + 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 32ef4684fce889366709b308d95fca0f6c024366 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Thu, 8 Jun 2023 08:41:00 +0200 Subject: [PATCH 3/5] Revert "Bail early from _Unpickler_SetInputStream() also" This reverts commit a91713a110adf630b6f1c750739a9786e442d912. --- Modules/_pickle.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/Modules/_pickle.c b/Modules/_pickle.c index fd58aff69ad67e..5b3665d11edde3 100644 --- a/Modules/_pickle.c +++ b/Modules/_pickle.c @@ -1678,25 +1678,25 @@ _Unpickler_SetInputStream(UnpicklerObject *self, PyObject *file) { /* Optional file methods */ if (_PyObject_LookupAttr(file, &_Py_ID(peek), &self->peek) < 0) { - goto error; + return -1; } if (_PyObject_LookupAttr(file, &_Py_ID(readinto), &self->readinto) < 0) { - goto error; - } - if (_PyObject_LookupAttr(file, &_Py_ID(read), &self->read) < 0) { - goto error; + return -1; } - if (_PyObject_LookupAttr(file, &_Py_ID(readline), &self->readline) < 0) { - goto error; + (void)_PyObject_LookupAttr(file, &_Py_ID(read), &self->read); + (void)_PyObject_LookupAttr(file, &_Py_ID(readline), &self->readline); + if (!self->readline || !self->read) { + if (!PyErr_Occurred()) { + PyErr_SetString(PyExc_TypeError, + "file must have 'read' and 'readline' attributes"); + } + Py_CLEAR(self->read); + Py_CLEAR(self->readinto); + Py_CLEAR(self->readline); + Py_CLEAR(self->peek); + return -1; } return 0; - -error: - 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 7d6821a569696065304775902d4f0b520c2b872f Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Thu, 8 Jun 2023 08:58:41 +0200 Subject: [PATCH 4/5] Add NEWS --- .../next/Library/2023-06-08-08-58-36.gh-issue-105375.bTcqS9.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Library/2023-06-08-08-58-36.gh-issue-105375.bTcqS9.rst diff --git a/Misc/NEWS.d/next/Library/2023-06-08-08-58-36.gh-issue-105375.bTcqS9.rst b/Misc/NEWS.d/next/Library/2023-06-08-08-58-36.gh-issue-105375.bTcqS9.rst new file mode 100644 index 00000000000000..3030477c8245b5 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2023-06-08-08-58-36.gh-issue-105375.bTcqS9.rst @@ -0,0 +1 @@ +Fix bugs in :mod:`pickle` where exceptions could be overwritten. From f6e7be7b3584df2cb717f29bb1ca2f566f0e830f Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Fri, 9 Jun 2023 14:03:48 +0200 Subject: [PATCH 5/5] Add back decrefs --- Modules/_pickle.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Modules/_pickle.c b/Modules/_pickle.c index 5b3665d11edde3..e6eb9c741e1adc 100644 --- a/Modules/_pickle.c +++ b/Modules/_pickle.c @@ -6007,6 +6007,8 @@ load_stack_global(PickleState *st, UnpicklerObject *self) !PyUnicode_CheckExact(global_name)) { PyErr_SetString(st->UnpicklingError, "STACK_GLOBAL requires str"); + Py_DECREF(global_name); + Py_DECREF(module_name); return -1; } global = find_class(self, module_name, global_name);