From f1f1a0abe19d78a080789744b417e6cc995af113 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Sun, 11 Jun 2023 22:28:42 +0200 Subject: [PATCH 1/7] gh-105375: Improve error handling in _Unpickler_SetInputStream() Prevent exceptions from possibly being overwritten in case of multiple failures. --- Modules/_pickle.c | 41 ++++++++++++++++++++++++++++------------- 1 file changed, 28 insertions(+), 13 deletions(-) diff --git a/Modules/_pickle.c b/Modules/_pickle.c index e6eb9c741e1adc..3c2dca5d490752 100644 --- a/Modules/_pickle.c +++ b/Modules/_pickle.c @@ -1678,25 +1678,40 @@ _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"); + int no_read_attr = 0; + if (_PyObject_LookupAttr(file, &_Py_ID(read), &self->read) < 0) { + if (!PyErr_ExceptionMatches(PyExc_AttributeError)) { + goto error; } - Py_CLEAR(self->read); - Py_CLEAR(self->readinto); - Py_CLEAR(self->readline); - Py_CLEAR(self->peek); - return -1; + PyErr_Clear(); + no_read_attr = 1; + } + int no_readline_attr = 0; + if (_PyObject_LookupAttr(file, &_Py_ID(readline), &self->readline) < 0) { + if (!PyErr_ExceptionMatches(PyExc_AttributeError)) { + goto error; + } + PyErr_Clear(); + no_readline_attr = 1; + } + if (no_read_attr || no_readline_attr) { + PyErr_SetString(PyExc_TypeError, + "file must have 'read' and 'readline' attributes"); + 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 620a9876a1e28f2aa176d8df7e67737ebdb4f45c Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Sun, 11 Jun 2023 22:46:15 +0200 Subject: [PATCH 2/7] Add NEWS --- .../next/Library/2023-06-11-22-46-06.gh-issue-105375.YkhSNt.rst | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2023-06-11-22-46-06.gh-issue-105375.YkhSNt.rst diff --git a/Misc/NEWS.d/next/Library/2023-06-11-22-46-06.gh-issue-105375.YkhSNt.rst b/Misc/NEWS.d/next/Library/2023-06-11-22-46-06.gh-issue-105375.YkhSNt.rst new file mode 100644 index 00000000000000..dda8f428760ba1 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2023-06-11-22-46-06.gh-issue-105375.YkhSNt.rst @@ -0,0 +1,2 @@ +Fix a bug in :c:func:`!_Unpickler_SetInputStream` where an exception could +end up being overwritten in case of failure. From 2d6a7b5e693e4ba0cbe42e0847510454318c2062 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Mon, 12 Jun 2023 13:17:05 +0200 Subject: [PATCH 3/7] Address Serhiy's review - There's no need to match AttributeError, since _PyObject_LookupAttr() converts that exception into a return value of 0. - We can use the result output variable instead of adding temporary boolean indicators. --- Modules/_pickle.c | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/Modules/_pickle.c b/Modules/_pickle.c index 3c2dca5d490752..2286d1c807908d 100644 --- a/Modules/_pickle.c +++ b/Modules/_pickle.c @@ -1683,23 +1683,13 @@ _Unpickler_SetInputStream(UnpicklerObject *self, PyObject *file) if (_PyObject_LookupAttr(file, &_Py_ID(readinto), &self->readinto) < 0) { goto error; } - int no_read_attr = 0; if (_PyObject_LookupAttr(file, &_Py_ID(read), &self->read) < 0) { - if (!PyErr_ExceptionMatches(PyExc_AttributeError)) { - goto error; - } PyErr_Clear(); - no_read_attr = 1; } - int no_readline_attr = 0; if (_PyObject_LookupAttr(file, &_Py_ID(readline), &self->readline) < 0) { - if (!PyErr_ExceptionMatches(PyExc_AttributeError)) { - goto error; - } PyErr_Clear(); - no_readline_attr = 1; } - if (no_read_attr || no_readline_attr) { + if (self->read == NULL || self->readline == NULL) { PyErr_SetString(PyExc_TypeError, "file must have 'read' and 'readline' attributes"); goto error; From 666bd74d9b672622cee40937b8c8d07da475d1c7 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Mon, 12 Jun 2023 13:29:46 +0200 Subject: [PATCH 4/7] Slightly reduce diff --- Modules/_pickle.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/_pickle.c b/Modules/_pickle.c index 2286d1c807908d..872b6ef088a434 100644 --- a/Modules/_pickle.c +++ b/Modules/_pickle.c @@ -1689,7 +1689,7 @@ _Unpickler_SetInputStream(UnpicklerObject *self, PyObject *file) if (_PyObject_LookupAttr(file, &_Py_ID(readline), &self->readline) < 0) { PyErr_Clear(); } - if (self->read == NULL || self->readline == NULL) { + if (!self->read || !self->readline) { PyErr_SetString(PyExc_TypeError, "file must have 'read' and 'readline' attributes"); goto error; From 078149bdab0f3a7c683bfe8f72097f068f00addc Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Mon, 12 Jun 2023 13:30:30 +0200 Subject: [PATCH 5/7] Slightly reduce diff --- Modules/_pickle.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/_pickle.c b/Modules/_pickle.c index 872b6ef088a434..3eabbe06ff33d3 100644 --- a/Modules/_pickle.c +++ b/Modules/_pickle.c @@ -1689,7 +1689,7 @@ _Unpickler_SetInputStream(UnpicklerObject *self, PyObject *file) if (_PyObject_LookupAttr(file, &_Py_ID(readline), &self->readline) < 0) { PyErr_Clear(); } - if (!self->read || !self->readline) { + if (!self->readline || !self->read) { PyErr_SetString(PyExc_TypeError, "file must have 'read' and 'readline' attributes"); goto error; From ab8606ab782d7ca3e10834322c164ff023dcf021 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Mon, 12 Jun 2023 19:05:03 +0200 Subject: [PATCH 6/7] Address Serhiy's review Don't clear arbitrary exceptions --- Modules/_pickle.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/Modules/_pickle.c b/Modules/_pickle.c index 3eabbe06ff33d3..8481106ba700ba 100644 --- a/Modules/_pickle.c +++ b/Modules/_pickle.c @@ -1684,15 +1684,17 @@ _Unpickler_SetInputStream(UnpicklerObject *self, PyObject *file) goto error; } if (_PyObject_LookupAttr(file, &_Py_ID(read), &self->read) < 0) { - PyErr_Clear(); + goto error; } if (_PyObject_LookupAttr(file, &_Py_ID(readline), &self->readline) < 0) { - PyErr_Clear(); + goto error; } if (!self->readline || !self->read) { - PyErr_SetString(PyExc_TypeError, - "file must have 'read' and 'readline' attributes"); - goto error; + if (!PyErr_Occurred()) { + PyErr_SetString(PyExc_TypeError, + "file must have 'read' and 'readline' attributes"); + goto error; + } } return 0; From b6a8a17e48ef4d69432ed0065b19fb5867933730 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Mon, 12 Jun 2023 23:40:39 +0200 Subject: [PATCH 7/7] Address Serhiy's review: There's no need for PyErr_Occurred() anymore --- Modules/_pickle.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/Modules/_pickle.c b/Modules/_pickle.c index 15045b08e18709..4913a8dfee589e 100644 --- a/Modules/_pickle.c +++ b/Modules/_pickle.c @@ -1706,11 +1706,9 @@ _Unpickler_SetInputStream(UnpicklerObject *self, PyObject *file) goto error; } if (!self->readline || !self->read) { - if (!PyErr_Occurred()) { - PyErr_SetString(PyExc_TypeError, - "file must have 'read' and 'readline' attributes"); - goto error; - } + PyErr_SetString(PyExc_TypeError, + "file must have 'read' and 'readline' attributes"); + goto error; } return 0;