GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-158031: Keep the iter sentinel alive during comparison by lul-cas · Pull Request #158046 · python/cpython

Conversation

@lul-cas

@lul-cas lul-cas commented Sep 24, 2026 •
edited by bedevere-app Bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

iter(callable, sentinel) compared the sentinel through a borrowed reference. A reentrant eq can clear that sentinel with setstate and free it while the comparison is still using it.

Hold a strong reference for the duration of PyObject_RichCompareBool, as aiter() already does.

Fixes #158031.

  • Issue: uaf of it_sentinel in callable_iterator.__next__ #158031

Sorry, something went wrong.

Uh oh!

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

All reactions
@lul-cas
pythongh-158031: Keep the iter sentinel alive during comparison
846fbbf
@bedevere-app bedevere-app Bot mentioned this pull request Sep 24, 2026
uaf of it_sentinel in callable_iterator.__next__ #158031
Open
@bedevere-app bedevere-app Bot added the awaiting review label Sep 24, 2026
@lul-cas
pythongh-158031: Add the NEWS entry for the iter sentinel fix
b01ea1f
@picnixz

picnixz commented Sep 24, 2026

Copy link
Copy Markdown
Member

This introduces contention on the FT build. How does it affect performance? PRs touching builtins are delicate and should not just be generated through an LLM. Anyone submitting such PRs should have a deeper understanding of the interpreter and the consequences so please keep in mind that we rather have core devs tackle such issues.

All reactions

Sorry, something went wrong.

Uh oh!

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

@lul-cas

lul-cas commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

Fair point on the free threaded build. I dont have a benchmark so I cant tell you how the extra reference behaves under contention

I've checked the bug itself. The comparison was using a borrowed sentinel and a reentrant "eq" could free it before the comparison finished. With this extra reference, the tests for that case passed and the object stays alive until the comparison is done

I agree that builtins are delicate and that this is the kind of change you'd rather have a core dev to handle. I'm happy to adjust it, or leave it, whichever you prefer. I may have underestimated that..

All reactions

Sorry, something went wrong.

Uh oh!

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

@picnixz

picnixz commented Sep 24, 2026

Copy link
Copy Markdown
Member

I dont have a benchmark so I cant tell you how the extra reference behaves under contention

Please do so then. We usually expect a PR that changes performance critical code to justify the change (the change is correct but the impact is the hard question)

lul-cas 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.

@lul-cas

lul-cas commented Sep 24, 2026 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

I ran next() on iter(callable, sentinel) on the free-threaded build, macOS 26.5 arm64, with --disable-gil --enable-optimizations --with-lto. The sentinel is a fresh object(), so the refcount actually changes. Just one thread.

main: 26.4 ns +/- 0.3 ns
this PR: 29.1 ns +/- 0.8 ns

Little slower. Around 2.7 ns per "next()". The callable only returns 0, so this is pretty much the worst case. The extra incref/decref is a big chunk of a tiny loop. I didnt try several threads sharing the same sentinel..

Maybe worth looking for another way to fix this?

main-iter.json
pr-iter.json

All reactions

Sorry, something went wrong.

Uh oh!

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

@picnixz

picnixz commented Sep 24, 2026 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

The other thing we can do is: do nothing. Sometimes it is not worth it just for purity as such cases are rare. It slows down every code path for theoretical issues. OTOH, the sentinel form is also rarer. We can document this as a real crasher.

cc @kumaraditya303

All reactions

Sorry, something went wrong.

Uh oh!

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

@lul-cas

lul-cas commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

Ok, that makes sense.
Happy to help if there's anything useful I can do

All reactions

Sorry, something went wrong.

Uh oh!

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

@nascheme nascheme mentioned this pull request Nov 11, 2026
gh-161792: Fix crashes in the callable iterator #161793
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

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

Assignees

No one assigned

Labels

awaiting review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

uaf of it_sentinel in callable_iterator.__next__

Uh oh!

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

2 participants

@lul-cas @picnixz
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.