gh-101765: Fix SystemError / segmentation fault in iter __reduce__ when internal access of builtins.__dict__ exhausts the iterator

[ionite34]

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

[JelleZijlstra]

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?

[miss-islington]

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]

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

[miss-islington]

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

[JelleZijlstra]

@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]

@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]

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.

[bedevere-bot]

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

[bedevere-bot]

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

[kumaraditya303]

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

[JelleZijlstra]

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

[JelleZijlstra]

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