Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-45691: Make array of small ints static to fix use-after-free error. #29366

Merged
merged 2 commits into from
Nov 3, 2021

Conversation

markshannon
Copy link
Member

@markshannon markshannon commented Nov 2, 2021

Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

Note the small ints have been in PyInterpreterState for a while. We didn't get use-after-free until gh-29274. So perhaps we could just revert that one? The general underlying problem in bpo-45691 is one we'll have to fix regardless, so there's no need to go all the way back to using a static global.

Otherwise this looks fine.

(I'm approving assuming that you'll take my feedback into consideration before merging and discuss if you disagree.)

@markshannon
Copy link
Member Author

markshannon commented Nov 2, 2021

As I stated in bpo-45691, we need to make everything static or per-interpreter. Right now that means static.

The best way to move towards multiple interpreters is to merge all the static state into a single struct. That way there is only one static value to convert to per-interpreter when we are ready.

We can start with the small ints 🙂

@ericsnowcurrently
Copy link
Member

ericsnowcurrently commented Nov 2, 2021

The best way to move towards multiple interpreters is to merge all the static state into a single struct.

FYI, that's exactly what _PyRuntimeState is (with a handful of things that will stay global). 🙂

Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

I'm good with this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants