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

gh-92036: Fix gc_fini_untrack() #92037

Merged
merged 1 commit into from
May 4, 2022
Merged

gh-92036: Fix gc_fini_untrack() #92037

merged 1 commit into from
May 4, 2022

Conversation

vstinner
Copy link
Member

Fix a crash in subinterpreters related to the garbage collector. When
a subinterpreter is deleted, untrack all objects tracked by its GC.
To prevent a crash in deallocator functions expecting objects to be
tracked by the GC, leak a strong reference to these objects on
purpose, so they are never deleted and their deallocator functions
are not called.

Fix a crash in subinterpreters related to the garbage collector. When
a subinterpreter is deleted, untrack all objects tracked by its GC.
To prevent a crash in deallocator functions expecting objects to be
tracked by the GC, leak a strong reference to these objects on
purpose, so they are never deleted and their deallocator functions
are not called.
@vstinner
Copy link
Member Author

@pablogsal @tim-one @pitrou @ericsnowcurrently: Would you mind to review the follow-up of the #30577 fix (1a4d1c1)?

My previous fix was incomplete and Kodi still crash when using the_sqlite3 extension with subintepreters:

Pablo proposed a different approach: "move all these objects into the main interpreter GC state":
#30577

It seems like the root problem is when an object is created in an interpreter A and destroyed in interpreter B, previously, the chain of objects could be broken: _gc_prev and _gc_next could become dangling pointers. My first fix prevents this case. I'm not convinced that moving objects to the main interpreter GC state prevents dangling pointers, but I didn't try to implement this idea.

My plan for sub-interpreters is to ensure that objects never travel between interpreters. The problem is that existing C extensions ("incompatible" with subinterpreters: don't implement PEP 489 multiphase init) can still share mutable containers (like a dict in this case) which exposes indirectly objects from different interpreters.

In this case, the interpreter B deletes an object tracked by the GC of interpreter A, but the interpreter A was deleted and so the object was untracked. Problem: the deallocator function, func_dealloc(), requires the object to be tracked by "a" GC (which one? that's unclear to me).

I'm not really excited by the overall gc_fini_untrack() solution, but for me, it's a practical solution for a practical problem. Fixing all C extensions to prevent leaking Python objects in Py_EndInterpreter() is a very complex problem. For example, it took 15 years to fix https://bugs.python.org/issue1635741 which only fixed a subset of the stdlib (only extensions used at Python startup).

@comrade-meowski comrade-meowski mentioned this pull request May 3, 2022
@vstinner vstinner merged commit 1424336 into python:main May 4, 2022
@vstinner vstinner deleted the gc_fini_untrack branch May 4, 2022 09:59
@vstinner vstinner added the needs backport to 3.10 only security fixes label May 4, 2022
@miss-islington
Copy link
Contributor

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

@vstinner vstinner added the needs backport to 3.9 only security fixes label May 4, 2022
@miss-islington
Copy link
Contributor

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label May 4, 2022
@bedevere-bot
Copy link

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

@bedevere-bot
Copy link

GH-92297 is a backport of this pull request to the 3.9 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.9 only security fixes label May 4, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 4, 2022
Fix a crash in subinterpreters related to the garbage collector. When
a subinterpreter is deleted, untrack all objects tracked by its GC.
To prevent a crash in deallocator functions expecting objects to be
tracked by the GC, leak a strong reference to these objects on
purpose, so they are never deleted and their deallocator functions
are not called.
(cherry picked from commit 1424336)

Co-authored-by: Victor Stinner <[email protected]>
miss-islington added a commit that referenced this pull request May 4, 2022
Fix a crash in subinterpreters related to the garbage collector. When
a subinterpreter is deleted, untrack all objects tracked by its GC.
To prevent a crash in deallocator functions expecting objects to be
tracked by the GC, leak a strong reference to these objects on
purpose, so they are never deleted and their deallocator functions
are not called.
(cherry picked from commit 1424336)

Co-authored-by: Victor Stinner <[email protected]>
miss-islington added a commit that referenced this pull request May 4, 2022
Fix a crash in subinterpreters related to the garbage collector. When
a subinterpreter is deleted, untrack all objects tracked by its GC.
To prevent a crash in deallocator functions expecting objects to be
tracked by the GC, leak a strong reference to these objects on
purpose, so they are never deleted and their deallocator functions
are not called.
(cherry picked from commit 1424336)

Co-authored-by: Victor Stinner <[email protected]>
hello-adam pushed a commit to hello-adam/cpython that referenced this pull request Jun 2, 2022
Fix a crash in subinterpreters related to the garbage collector. When
a subinterpreter is deleted, untrack all objects tracked by its GC.
To prevent a crash in deallocator functions expecting objects to be
tracked by the GC, leak a strong reference to these objects on
purpose, so they are never deleted and their deallocator functions
are not called.
(cherry picked from commit 1424336)

Co-authored-by: Victor Stinner <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants