GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-101765: Fix SystemError / segmentation fault in iter `__reduce__` when internal access of `builtins.__dict__` exhausts the iterator by ionite34 · Pull Request #101769 · python/cpython

Conversation

ionite34 commented Feb 10, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

Summary

Fixes potential segmentation fault or SystemError when __reduce__ is called on iter objects when the key of the hash of "iter" within __builtins__.__dict__ is a custom object that executes arbitrary code within __eq__, mutating the iter 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 currently genericaliasobject.ga_iter_reduce does not handle empty iterators at all and has no NULL check

Python 3.11.0 (main, Oct 26 2022, 10:14:06) [GCC 9.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> x = tuple[int]
>>>
>>> it = iter(x)
>>> _ = list(it)
>>>
>>> it.__reduce__()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
SystemError: NULL object passed to Py_BuildValue

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

Python 3.12.0a5+ (heads/fix-reduce-dirty:93854e172e, Feb 10 2023, 13:41:45) [GCC 9.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> x = tuple[int]
>>>
>>> it = iter(x)
>>> _ = list(it)
>>>
>>> it.__reduce__()
(<built-in function iter>, ((),))

Linked Issue

Issue: iter __reduce__ can segfault if accessing __builtins__.__dict__['iter'] mutates the iter object #101765

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Fix undefined behavior in listiter_reduce from _PyEval_GetBuiltin sid…

f256afe

…e effects

bedevere-bot added the awaiting review label

Feb 10, 2023

bedevere-bot mentioned this pull request

Feb 10, 2023

iter __reduce__ can segfault if accessing __builtins__.__dict__['iter'] mutates the iter object #101765

Closed

ionite34 added 4 commits

February 9, 2023 20:34


          Update comment to not mention __eq__

6b4faad


          Fix undefined behavior in iter_reduce and calliter_reduce

a6d6211


          Update listiter_reduce_general comment

4ccf427


          Fix undefined behavior in bytearrayiter_reduce from _PyEval_GetBuilti…

c2c9cfb

…n side effects

arhadthedev added interpreter-core

(Objects, Python, Grammar, and Parser dirs)

type-crash A hard crash of the interpreter, possibly with a core dump labels

Feb 10, 2023

ionite34 added 2 commits

February 10, 2023 00:26


          Fix undefined behavior in striter_reduce from _PyEval_GetBuiltin side…

e2989d9

… effects


          Fix undefined behavior in tupleiter_reduce from _PyEval_GetBuiltin si…

71960a8

…de effects

JelleZijlstra self-requested a review

February 10, 2023 05:43

JelleZijlstra reviewed

Feb 10, 2023

View reviewed changes

Comment thread

Objects/bytearrayobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

JelleZijlstra reviewed

Feb 10, 2023

View reviewed changes

Comment thread

Objects/listobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

ionite34 and others added 9 commits

February 10, 2023 01:04


          Move iter call in unicodeiter_reduce before it pointer access due t…

efa0540

…o potential side effects


          Add iter reduce tests for issue python#101765

c5abb14


          Remove C++ reference in comments

45522c6


          Remove C++ reference in comments

4f5fc19


          Move builtin declarations inside if blocks

049a8dd


          Move _PyEval_GetBuiltin before gi checks, add gi NULL check in ga_ite…

ef4f955

…r_reduce


          Update iter reduce mutating tests for generic alias

7d4afb0


          📜🤖 Added by blurb_it.

8e4418d


          Fix backticks format for news

d8ced8e

ionite34 marked this pull request as ready for review

February 10, 2023 07:29

carljm reviewed

Feb 10, 2023

View reviewed changes

Comment thread

Lib/test/test_iter.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Lib/test/test_iter.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Objects/listobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Objects/unicodeobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

JelleZijlstra self-requested a review

February 10, 2023 17:29

ionite34 added 4 commits

February 10, 2023 12:46


          Refactor iter reduce builtins mutation tests

178b8ea

- Added some comments for dict del usages
- Switched to `__builtin__` instead of conditional `__dict__` access
- Use kwargs for improved readability


          Update iter mutating builtins test to include reversed iterator for l…

49ba8c3

…ist conditional branch


          Add comment in unicodeiter_reduce for moving iter call before it poin…

93854e1

…ters


          Change test __builtins__ to builtins import

98ec3c6

JelleZijlstra commented Feb 24, 2023

Copy link

Copy Markdown

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

JelleZijlstra merged commit 54dfa14 into python:main

Feb 24, 2023

miss-islington commented Feb 24, 2023

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot removed the awaiting merge label

Feb 24, 2023

miss-islington commented Feb 24, 2023

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

miss-islington commented Feb 24, 2023

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

JelleZijlstra commented Feb 24, 2023

Copy link

Copy Markdown

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ionite34 commented Feb 25, 2023

Copy link

Copy Markdown

Contributor Author

@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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

JelleZijlstra commented Feb 25, 2023

Copy link

Copy Markdown

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

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ionite34 added a commit to ionite34/cpython that referenced this pull request

Feb 25, 2023


          [3.11] pythongh-101765: Fix SystemError / segmentation fault in iter …

a9cd908

…`__reduce__` when internal access of `builtins.__dict__` exhausts the iterator (pythonGH-101769).

(cherry picked from commit 54dfa14)

Co-authored-by: Ionite <[email protected]>

ionite34 added a commit to ionite34/cpython that referenced this pull request

Feb 25, 2023


          [3.11] pythongh-101765: Fix SystemError / segmentation fault in iter …

297d01e

…`__reduce__` when internal access of `builtins.__dict__` exhausts the iterator (pythonGH-101769).

(cherry picked from commit 54dfa14)

Co-authored-by: Ionite <[email protected]>

ionite34 mentioned this pull request

Feb 25, 2023

[3.11] gh-101765: Fix SystemError / segmentation fault in iter __reduce__ when internal access of builtins.__dict__ exhausts the iterator (GH-101769) #102228

Merged

bedevere-bot commented Feb 25, 2023

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot removed the needs backport to 3.11 only security fixes label

Feb 25, 2023

ionite34 added a commit to ionite34/cpython that referenced this pull request

Feb 25, 2023


          [3.10] pythongh-101765: Fix SystemError / segmentation fault in iter …

031382c

…`__reduce__` when internal access of `builtins.__dict__` exhausts the iterator (pythonGH-101769).

(cherry picked from commit 54dfa14)

Co-authored-by: Ionite <[email protected]>

ionite34 mentioned this pull request

Feb 25, 2023

[3.10] gh-101765: Fix SystemError / segmentation fault in iter __reduce__ when internal access of builtins.__dict__ exhausts the iterator (GH-101769) #102229

Merged

bedevere-bot commented Feb 25, 2023

Copy link

Copy Markdown

GH-102229 is a backport of this pull request to the 3.10 branch.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot removed the needs backport to 3.10 label

Feb 25, 2023

JelleZijlstra pushed a commit that referenced this pull request

Feb 25, 2023


          [3.11] gh-101765: Fix SystemError / segmentation fault in iter `__red…

5d46122

…uce__` when internal access of `builtins.__dict__` exhausts the iterator (GH-101769) (#102228)

(cherry picked from commit 54dfa14)

JelleZijlstra pushed a commit that referenced this pull request

Feb 25, 2023


          [3.10] gh-101765: Fix SystemError / segmentation fault in iter `__red…

9f472f8

…uce__` when internal access of `builtins.__dict__` exhausts the iterator (GH-101769) (#102229)

(cherry picked from commit 54dfa14)

kumaraditya303 reviewed

Feb 25, 2023

View reviewed changes

Comment thread

Objects/listobject.c

		} else {
		PyObject *reversed = _PyEval_GetBuiltin(&_Py_ID(reversed));
		listreviterobject it = (listreviterobject )_it;
		if (it->it_seq) {

kumaraditya303 Feb 25, 2023

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

JelleZijlstra Feb 25, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

JelleZijlstra Feb 25, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

JelleZijlstra mentioned this pull request

Feb 25, 2023

gh-101765: Fix refcount issues in list and unicode pickling #102265

Merged

JelleZijlstra added a commit to JelleZijlstra/cpython that referenced this pull request

Feb 25, 2023


          pythongh-101765: Fix refcount issues in list and unicode pickling

b8ad59e

Followup from python#101769.

JelleZijlstra added a commit that referenced this pull request

Feb 26, 2023


          gh-101765: Fix refcount issues in list and unicode pickling (#102265)

d71edbd

Followup from #101769.

This was referenced Feb 26, 2023

[3.11] gh-101765: Fix refcount issues in list and unicode pickling (G… #102268

Merged

[3.10] gh-101765: Fix refcount issues in list and unicode pickling (G… #102269

Merged

JelleZijlstra added a commit to JelleZijlstra/cpython that referenced this pull request

Feb 26, 2023


          [3.11] pythongh-101765: Fix refcount issues in list and unicode pickl…

e97ddc6

…ing (pythonGH-102265)

Followup from pythonGH-101769..
(cherry picked from commit d71edbd)

Co-authored-by: Jelle Zijlstra <[email protected]>

JelleZijlstra added a commit to JelleZijlstra/cpython that referenced this pull request

Feb 26, 2023


          [3.10] pythongh-101765: Fix refcount issues in list and unicode pickl…

fca504e

…ing (pythonGH-102265)

Followup from pythonGH-101769..
(cherry picked from commit d71edbd)

Co-authored-by: Jelle Zijlstra <[email protected]>

corona10 mentioned this pull request

Feb 8, 2024

gh-112087: Make __sizeof__ and listiter_{len, next} to be threadsafe #114843

Merged

JelleZijlstra pushed a commit to JelleZijlstra/cpython that referenced this pull request

Sep 10, 2024


          pythongh-101765: Fix SystemError / segmentation fault in iter `__redu…

473a891

…ce__` when internal access of `builtins.__dict__` exhausts the iterator (python#101769)

Reviewers

JelleZijlstra

JelleZijlstra approved these changes

kumaraditya303

kumaraditya303 left review comments

carljm

carljm approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

JelleZijlstra

Labels

interpreter-core

(Objects, Python, Grammar, and Parser dirs)

type-crash

A 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.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

ionite34 commented Feb 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Affected methods

Linked Issue

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JelleZijlstra commented Feb 24, 2023

Uh oh!

miss-islington commented Feb 24, 2023

Uh oh!

miss-islington commented Feb 24, 2023

Uh oh!

miss-islington commented Feb 24, 2023

Uh oh!

JelleZijlstra commented Feb 24, 2023

Uh oh!

ionite34 commented Feb 25, 2023

Uh oh!

JelleZijlstra commented Feb 25, 2023

Uh oh!

bedevere-bot commented Feb 25, 2023

Uh oh!

bedevere-bot commented Feb 25, 2023

Uh oh!

kumaraditya303 Feb 25, 2023

Choose a reason for hiding this comment

Uh oh!

JelleZijlstra Feb 25, 2023

Choose a reason for hiding this comment

Uh oh!

JelleZijlstra Feb 25, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ionite34 commented Feb 10, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.