Skip to content

gh-82565: Fix a possible assertion failure in _pickle#16606

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
serhiy-storchaka merged 13 commits into from
Dec 1, 2023
Merged

gh-82565: Fix a possible assertion failure in _pickle #16606

serhiy-storchaka merged 13 commits into from
Dec 1, 2023

Conversation

@ZackerySpytz
Copy link
Contributor

@ZackerySpytzZackerySpytz commented Oct 6, 2019•
edited by bedevere-bot
Loading

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.

https://bugs.python.org/issue38384

@ZackerySpytz
bpo-38384: Fix a possible assertion failure in _pickle
afdb094
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.
serhiy-storchaka
serhiy-storchaka requested changes Oct 6, 2019
View reviewed changes
Copy link
Member

@serhiy-storchakaserhiy-storchaka left a comment

Choose a reason for hiding this comment

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

Please add a test.

@bedevere-bot
Copy link

bedevere-bot commented Oct 6, 2019

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Oct 21, 2019

Minimal reproducer (but you can add readline and readinto):

classF: @propertydefread(self): 1/0importpicklepickle.load(F())

@taleinat
Copy link
Contributor

taleinat commented Nov 13, 2019

@ZackerySpytz Ping?

@taleinat
Copy link
Contributor

taleinat commented Nov 22, 2019

Hi @ZackerySpytz! This is looking good!

Make sure to also add a NEWS entry (you can use blurb-it).

@iritkatriel
Copy link
Member

iritkatriel commented Oct 17, 2021

Please add a test.

@ZackerySpytz has added a test. Shall we merge this?

serhiy-storchaka
serhiy-storchaka reviewed Oct 19, 2021
View reviewed changes
@ZackerySpytzZackerySpytzmannequin mentioned this pull request Apr 10, 2022
Closed
@furkanonder
Copy link
Contributor

furkanonder commented Dec 22, 2022

LGTM. @serhiy-storchaka

serhiy-storchaka
serhiy-storchaka reviewed Dec 22, 2022
View reviewed changes
Copy link
Member

@serhiy-storchakaserhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM.

Please add few more test cases:

  • no read, no readline
  • read = bad_property, no readline
  • readline = bad_property, no read

@SahillMulaniSahillMulani mentioned this pull request Apr 3, 2023
Closed
@hauntsaninjahauntsaninja changed the title bpo-38384: Fix a possible assertion failure in _picklegh-82565: Fix a possible assertion failure in _pickleApr 3, 2023
@arhadthedevarhadthedev added extension-modules C modules in the Modules dir 3.11 only security fixes labels Apr 20, 2023
@arhadthedevarhadthedev added the 3.12 only security fixes label Apr 20, 2023
@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Dec 1, 2023

Sorry, I forget to merge this PR at its time, so the original issue was fixed in #105667.

But the tests may still be useful.

@serhiy-storchakaserhiy-storchaka enabled auto-merge (squash) December 1, 2023 12:39
@serhiy-storchakaserhiy-storchaka added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes tests Tests in the Lib/test dir skip news and removed 3.11 only security fixes 3.12 only security fixes extension-modules C modules in the Modules dir labels Dec 1, 2023
@serhiy-storchakaserhiy-storchaka merged commit 0584443 into python:mainDec 1, 2023
@miss-islington-app
Copy link

miss-islington-appbot commented Dec 1, 2023

Thanks @ZackerySpytz for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 1, 2023
@ZackerySpytz@miss-islington
pythongh-82565: Add tests for pickle and unpickle with bad files (pyt…
c3caf2b
…honGH-16606) (cherry picked from commit 0584443) Co-authored-by: Zackery Spytz <zspytz@gmail.com>
@bedevere-app
Copy link

bedevere-appbot commented Dec 1, 2023

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

@bedevere-appbedevere-appbot removed the needs backport to 3.12 only security fixes label Dec 1, 2023
@bedevere-app
Copy link

bedevere-appbot commented Dec 1, 2023

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 1, 2023
@ZackerySpytz@miss-islington
pythongh-82565: Add tests for pickle and unpickle with bad files (pyt…
f860fd7
…honGH-16606) (cherry picked from commit 0584443) Co-authored-by: Zackery Spytz <zspytz@gmail.com>
@bedevere-appbedevere-appbot removed the needs backport to 3.11 only security fixes label Dec 1, 2023
serhiy-storchaka pushed a commit that referenced this pull request Dec 1, 2023
@miss-islington@ZackerySpytz
[3.12]gh-82565: Add tests for pickle and unpickle with bad files (GH…
edce0c4
…-16606) (GH-112591) (cherry picked from commit 0584443) Co-authored-by: Zackery Spytz <zspytz@gmail.com>
serhiy-storchaka pushed a commit that referenced this pull request Dec 1, 2023
@miss-islington@ZackerySpytz
[3.11]gh-82565: Add tests for pickle and unpickle with bad files (GH…
d838c7a
…-16606) (GH-112592) (cherry picked from commit 0584443) Co-authored-by: Zackery Spytz <zspytz@gmail.com>
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
@ZackerySpytz@aisk
pythongh-82565: Add tests for pickle and unpickle with bad files (pyt…
1fd2c8a
…honGH-16606)
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
@ZackerySpytz@Glyphack
pythongh-82565: Add tests for pickle and unpickle with bad files (pyt…
ffe1825
…honGH-16606)
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

@furkanonderfurkanonderfurkanonder approved these changes

+1 more reviewer

@bumper-furkan-onderbumper-furkan-onderbumper-furkan-onder approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

skip newstestsTests in the Lib/test dir

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@ZackerySpytz@bedevere-bot@serhiy-storchaka@taleinat@iritkatriel@furkanonder@bumper-furkan-onder@arhadthedev@the-knights-who-say-ni@ezio-melotti