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

[3.13] gh-124785: alternative interned string clear fix #124796

Closed
wants to merge 2 commits into from

Conversation

nascheme
Copy link
Member

@nascheme nascheme commented Sep 30, 2024

The gh-124646 fix has the issue that it breaks the trace-refs debug build. This is a simpler fix that avoids the use-after-free crashes by just leaking immortal interned strings allocated by legacy sub-interpreters.

The pythonGH-124646 fix has the issue that it breaks the trace-refs debug
build.  This is a simpler fix that avoids the use-after-free crashes by
just leaking immortal interned strings allocated by legacy
subinterpreters.
@nascheme nascheme added the 3.13 bugs and security fixes label Sep 30, 2024
@AA-Turner AA-Turner changed the title gh-124785: alternative interned string clear fix [3.13] gh-124785: alternative interned string clear fix Sep 30, 2024
@Yhg1s
Copy link
Member

Yhg1s commented Sep 30, 2024

Since this is a rollback of the original fix plus a new, debatably dodgy fix on top, can we just do a regular rollback of #124648 first? I think we need more time for the proper fix and I'd rather release 3.13 (and 3.12, really) with known issues than introduce wholly new ones.

@nascheme
Copy link
Member Author

nascheme commented Sep 30, 2024

You should rollback GH-124648, it was not as safe a change as I thought it was. I thought it only kicks into action of you are using "basic single-phase init" extensions and at that point you are already on shaky ground. However it actually kicks in if your sub-interpreter is allowed to use those extensions, i.e Py_RTFLAGS_MULTI_INTERP_EXTENSIONS is set. I didn't realize that detail until investigating the gh-124785.

With GH-124648 rolled back, you can make a separate decision if you want to merge this PR into a RC. It fixes a use-after-free segfault bug. However, those bugs only get triggered if you are on the shaky ground as explained above (embedding case with legacy sub-interpreters). So, I think you would be well justified to say you don't want to merge this given we are already into RC2. If the ABI change is a problem, I think it can be re-worked to avoid that. Then it could go into 3.13.1 as just a regular bugfix.

@nascheme
Copy link
Member Author

Closing, for the 'main' branch I think GH-124808 is a better fix. For 3.13 (and 3.12) I'll work on either a backported version of GH-124808 or potentially a re-worked version of this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.13 bugs and security fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants