Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-101765: Fix SystemError / segmentation fault in iter__reduce__ when internal access ofbuiltins.__dict__ exhausts the iterator#101769

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.

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

Already on GitHub?Sign in to your account

Merged
JelleZijlstra merged 24 commits intopython:mainfromionite34:fix-reduce
Feb 24, 2023

Conversation

@ionite34
Copy link
Contributor

@ionite34ionite34 commentedFeb 10, 2023•
edited
Loading

Summary

  • Fixes potential segmentation fault or SystemError when__reduce__ is called oniter objects when the key of the hash of"iter" within__builtins__.__dict__ is a custom object that executes arbitrary code within__eq__, mutating theiter object and causing illegal memory access or SystemError (based on C argument evaluation order, which is undefined behavior).

Affected methods

  • iterobject.c
    • iter_reduce
    • calliter_reduce
  • listobject.c
    • listiter_reduce_general
  • bytearrayobject.c
    • bytearrayiter_reduce
  • bytesobject.c
    • striter_reduce
  • tupleobject.c
    • tupleiter_reduce
  • genericaliasobject.c
    • ga_iter_reduce

This PR also fixes a compounded issue where currentlygenericaliasobject.ga_iter_reduce does not handle empty iterators at all and has no NULL check

Python3.11.0 (main,Oct262022,10:14:06) [GCC9.4.0]onlinuxType"help","copyright","credits"or"license"formoreinformation.>>>x=tuple[int]>>>>>>it=iter(x)>>> _=list(it)>>>>>>it.__reduce__()Traceback (mostrecentcalllast):File"<stdin>",line1,in<module>SystemError:NULLobjectpassedtoPy_BuildValue

Along with moving the evaluation of_PyEval_GetBuiltin for potential side effects, this also adds handling of the NULL case (like the otheriter_reduce functions have:

Python3.12.0a5+ (heads/fix-reduce-dirty:93854e172e,Feb102023,13:41:45) [GCC9.4.0]onlinuxType"help","copyright","credits"or"license"formoreinformation.>>>x=tuple[int]>>>>>>it=iter(x)>>> _=list(it)>>>>>>it.__reduce__()(<built-infunctioniter>, ((),))

Linked Issue

@arhadthedevarhadthedev added interpreter-core(Objects, Python, Grammar, and Parser dirs) type-crashA hard crash of the interpreter, possibly with a core dump labelsFeb 10, 2023
@JelleZijlstraJelleZijlstra self-requested a reviewFebruary 10, 2023 05:43
@ionite34ionite34 marked this pull request as ready for reviewFebruary 10, 2023 07:29
@JelleZijlstraJelleZijlstra self-requested a reviewFebruary 10, 2023 17:29
- Added some comments for dict del usages- Switched to `__builtin__` instead of conditional `__dict__` access- Use kwargs for improved readability
@JelleZijlstra
Copy link
Member

Since this could cause a segfault, it should perhaps be considered a security issue to be backported to the security branches. However, the way to trigger it is so obscure that I'm not sure it's worth backporting.@ambv what do you think?

@JelleZijlstraJelleZijlstra merged commit54dfa14 intopython:mainFeb 24, 2023
@miss-islington
Copy link
Contributor

Thanks@ionite34 for the PR, and@JelleZijlstra for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry,@ionite34 and@JelleZijlstra, I could not cleanly backport this to3.11 due to a conflict.
Please backport usingcherry_picker on command line.
cherry_picker 54dfa14c5a94b893b67a4d9e9e403ff538ce9023 3.11

@miss-islington
Copy link
Contributor

Sorry@ionite34 and@JelleZijlstra, I had trouble checking out the3.10 backport branch.
Please retry by removing and re-adding the "needs backport to 3.10" label.
Alternatively, you can backport usingcherry_picker on the command line.
cherry_picker 54dfa14c5a94b893b67a4d9e9e403ff538ce9023 3.10

@JelleZijlstra
Copy link
Member

@ionite34 are you interested in doing the 3.10/3.11 backports (and potentially 3.7-3.9 if we decide this is important enough)? It's probably a matter of installing cherry-picker locally and fixing a few conflicts.

@ionite34
Copy link
ContributorAuthor

@ionite34 are you interested in doing the 3.10/3.11 backports (and potentially 3.7-3.9 if we decide this is important enough)? It's probably a matter of installing cherry-picker locally and fixing a few conflicts.

Yeah I could try 👀, though uh, I don't think I saw anything on the dev guide regarding backports. Am I essentially doing the stuff mentioned here?https://pypi.org/project/cherry_picker/#example

@JelleZijlstra
Copy link
Member

Thanks! Yes, cherrry-picker will do most of the work of actually creating the PR, you just have to do the manual work to fix the merge.

Just as general background, we develop every 3.x version in a separate branch. PRs are usually first against the main branch (which will be 3.12). If the PR fixes a bug (as opposed to a new feature), we'll backport the change into the bugfix branches, which are currently for 3.10 and 3.11.

ionite34 reacted with thumbs up emoji

ionite34 added a commit to ionite34/cpython that referenced this pull requestFeb 25, 2023
…`__reduce__` when internal access of `builtins.__dict__` exhausts the iterator (pythonGH-101769).(cherry picked from commit54dfa14)Co-authored-by: Ionite <dev@ionite.io>
ionite34 added a commit to ionite34/cpython that referenced this pull requestFeb 25, 2023
…`__reduce__` when internal access of `builtins.__dict__` exhausts the iterator (pythonGH-101769).(cherry picked from commit54dfa14)Co-authored-by: Ionite <dev@ionite.io>
@bedevere-bot
Copy link

GH-102228 is a backport of this pull request to the3.11 branch.

@bedevere-botbedevere-bot removed the needs backport to 3.11only security fixes labelFeb 25, 2023
ionite34 added a commit to ionite34/cpython that referenced this pull requestFeb 25, 2023
…`__reduce__` when internal access of `builtins.__dict__` exhausts the iterator (pythonGH-101769).(cherry picked from commit54dfa14)Co-authored-by: Ionite <dev@ionite.io>
@bedevere-bot
Copy link

GH-102229 is a backport of this pull request to the3.10 branch.

@bedevere-botbedevere-bot removed the needs backport to 3.10only security fixes labelFeb 25, 2023
JelleZijlstra pushed a commit that referenced this pull requestFeb 25, 2023
…uce__` when internal access of `builtins.__dict__` exhausts the iterator (GH-101769) (#102228)(cherry picked from commit54dfa14)
JelleZijlstra pushed a commit that referenced this pull requestFeb 25, 2023
…uce__` when internal access of `builtins.__dict__` exhausts the iterator (GH-101769) (#102229)(cherry picked from commit54dfa14)
}else {
PyObject*reversed=_PyEval_GetBuiltin(&_Py_ID(reversed));
listreviterobject*it= (listreviterobject*)_it;
if (it->it_seq) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ifit_seq is NULL then reversed is leaked here,_PyEval_GetBuiltin returns a strong reference.

Choose a reason for hiding this comment

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

Good point, but doesn't that mean the old code also leaked references? I'll prepare a PR to fix this now.

Choose a reason for hiding this comment

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

Oh wait, the N code to Py_BuildValue steals a reference, so the previous code was right in terms of refcounting.

JelleZijlstra added a commit to JelleZijlstra/cpython that referenced this pull requestFeb 25, 2023
JelleZijlstra added a commit to JelleZijlstra/cpython that referenced this pull requestFeb 26, 2023
…ing (pythonGH-102265)Followup frompythonGH-101769..(cherry picked from commitd71edbd)Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
JelleZijlstra added a commit to JelleZijlstra/cpython that referenced this pull requestFeb 26, 2023
…ing (pythonGH-102265)Followup frompythonGH-101769..(cherry picked from commitd71edbd)Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
JelleZijlstra pushed a commit to JelleZijlstra/cpython that referenced this pull requestSep 10, 2024
…ce__` when internal access of `builtins.__dict__` exhausts the iterator (python#101769)
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@JelleZijlstraJelleZijlstraJelleZijlstra approved these changes

@kumaraditya303kumaraditya303kumaraditya303 left review comments

@carljmcarljmcarljm approved these changes

Assignees

@JelleZijlstraJelleZijlstra

Labels

interpreter-core(Objects, Python, Grammar, and Parser dirs)type-crashA hard crash of the interpreter, possibly with a core dump

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@ionite34@JelleZijlstra@miss-islington@bedevere-bot@carljm@kumaraditya303@arhadthedev
[8]ページ先頭
©2009-2025 Movatter.jp