Skip to content

gh-105375: Improve error handling in _Unpickler_SetInputStream() - #105667

Merged
erlend-aasland merged 10 commits into
python:mainfrom
erlend-aasland:improve-pickle-error-handling
Jun 13, 2023
Merged

gh-105375: Improve error handling in _Unpickler_SetInputStream()#105667
erlend-aasland merged 10 commits into
python:mainfrom
erlend-aasland:improve-pickle-error-handling

Conversation

@erlend-aasland

@erlend-aaslanderlend-aasland commented Jun 11, 2023•
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

Prevent exceptions from possibly being overwritten in case of multiple
failures.

@erlend-aasland
pythongh-105375: Improve error handling in _Unpickler_SetInputStream()
f1f1a0a
Prevent exceptions from possibly being overwritten in case of multiple
failures.
@erlend-aaslanderlend-aasland added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Jun 11, 2023
@bedevere-botbedevere-bot mentioned this pull request Jun 11, 2023
Closed
18 tasks
@erlend-aasland

erlend-aasland commented Jun 11, 2023

Copy link
Copy Markdown
ContributorAuthor

@serhiy-storchaka, would you have time to look at this bugfix?

serhiy-storchaka
serhiy-storchaka reviewed Jun 12, 2023
View reviewed changes

@serhiy-storchakaserhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you missed the purpose _PyObject_LookupAttr(). It was designed to get rid of PyErr_ExceptionMatches(PyExc_AttributeError) checks and the overhead of raising and catching an exception.

Comment threadModules/_pickle.c Outdated
Comment threadModules/_pickle.c Outdated
@erlend-aasland
Pull in main
09e678b
@erlend-aasland
Address Serhiy's review
2d6a7b5
- 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.
@erlend-aasland
Slightly reduce diff
666bd74
@erlend-aasland
Slightly reduce diff
078149b
serhiy-storchaka
serhiy-storchaka reviewed Jun 12, 2023
View reviewed changes
Comment threadModules/_pickle.c Outdated
Comment threadModules/_pickle.c Outdated
serhiy-storchaka
serhiy-storchaka reviewed Jun 12, 2023
View reviewed changes
Comment threadModules/_pickle.c Outdated
@erlend-aasland
Pull in main
ba9ceb0
@erlend-aasland
Address Serhiy's review:
b6a8a17
There's no need for PyErr_Occurred() anymore
@erlend-aasland

erlend-aasland commented Jun 13, 2023

Copy link
Copy Markdown
ContributorAuthor

Thanks for the very helpful review and historical insight, Serhiy.

@erlend-aasland
erlend-aasland merged commit 217589d into python:mainJun 13, 2023
@miss-islington

miss-islington commented Jun 13, 2023

Copy link
Copy Markdown
Contributor

Thanks @erlend-aasland for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

@erlend-aasland
erlend-aasland deleted the improve-pickle-error-handling branch June 13, 2023 08:38
@bedevere-bot

bedevere-bot commented Jun 13, 2023

Copy link
Copy Markdown

GH-105720 is a backport of this pull request to the 3.12 branch.

@bedevere-botbedevere-bot removed the needs backport to 3.12 only security fixes label Jun 13, 2023
@bedevere-bot

bedevere-bot commented Jun 13, 2023

Copy link
Copy Markdown

GH-105721 is a backport of this pull request to the 3.11 branch.

erlend-aasland added a commit to erlend-aasland/cpython that referenced this pull request Jun 13, 2023
@erlend-aasland
[3.12]pythongh-105375: Improve error handling in _Unpickler_SetInput…
3cfb1cf
…Stream() (python#105667)
Prevent exceptions from possibly being overwritten in case of multiple
failures.
(cherry picked from commit 217589d)
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
erlend-aasland added a commit to erlend-aasland/cpython that referenced this pull request Jun 13, 2023
@erlend-aasland
[3.11]pythongh-105375: Improve error handling in _Unpickler_SetInput…
c43310d
…Stream() (python#105667)
Prevent exceptions from possibly being overwritten in case of multiple
failures.
(cherry picked from commit 217589d)
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
@bedevere-botbedevere-bot removed the needs backport to 3.11 only security fixes label Jun 13, 2023
erlend-aasland added a commit that referenced this pull request Jun 13, 2023
@erlend-aasland
[3.11]gh-105375: Improve error handling in _Unpickler_SetInputStream…
2b1745f
…() (#105667) (#105721)
Prevent exceptions from possibly being overwritten in case of multiple
failures.
(cherry picked from commit 217589d)
erlend-aasland added a commit that referenced this pull request Jun 13, 2023
@erlend-aasland
[3.12]gh-105375: Improve error handling in _Unpickler_SetInputStream…
56877e4
…() (#105667) (#105720)
Prevent exceptions from possibly being overwritten in case of multiple
failures.
(cherry picked from commit 217589d)
This was referenced Dec 1, 2023
Merged
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchakaserhiy-storchakaserhiy-storchaka approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@erlend-aasland@miss-islington@bedevere-bot@serhiy-storchaka