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-107080: Fix Py_TRACE_REFS Crashes Under Isolated Subinterpreters #107567

Merged
merged 6 commits into from
Aug 3, 2023

Conversation

ericsnowcurrently
Copy link
Member

@ericsnowcurrently ericsnowcurrently commented Aug 2, 2023

The linked list of objects was a global variable, which broke isolation between interpreters, causing crashes. To solve this, we've moved the linked list to each interpreter.

Objects/object.c Show resolved Hide resolved
}

PyObject refchain = { 0 };
_Py_StealRefchain(tstate->interp, &refchain);
Copy link
Member

Choose a reason for hiding this comment

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

Why is this necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

The interpreter will be deallocated before we later call _Py_PrintReferenceAddresses() to walk the linked list.

That said, I think there's a related problem that I still need to address. Namely, every remaining object in the linked list will be (effectively) freed when the interpreter's allocator is finalized, so the _ob_next and _ob_prev accesses might segfault. It likely isn't a big problem during runtime finalization (except perhaps for embedders) but I'll fix that regardless.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I'm pretty sure that's not actually a problem. We only call _Py_PrintReferenceAddresses() in Py_FinalizeEx() and only for the main interpreter, which is statically allocated. Objects in the linked list might still cause a segfault but only if they would have before this change, so I'm not going to worry about that here. I will leave a note in the code though.

@ericsnowcurrently ericsnowcurrently enabled auto-merge (squash) August 3, 2023 19:43
@ericsnowcurrently ericsnowcurrently merged commit 58ef741 into python:main Aug 3, 2023
19 checks passed
@miss-islington
Copy link
Contributor

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

@bedevere-bot
Copy link

GH-107599 is a backport of this pull request to the 3.12 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.12 bug and security fixes label Aug 3, 2023
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 3, 2023
…ters (pythongh-107567)

The linked list of objects was a global variable, which broke isolation between interpreters, causing crashes. To solve this, we've moved the linked list to each interpreter.
(cherry picked from commit 58ef741)

Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>
@ericsnowcurrently ericsnowcurrently deleted the fix-tracerefs branch August 3, 2023 20:02
Yhg1s pushed a commit that referenced this pull request Aug 3, 2023
…eters (gh-107567) (#107599)

gh-107080: Fix Py_TRACE_REFS Crashes Under Isolated Subinterpreters (gh-107567)

The linked list of objects was a global variable, which broke isolation between interpreters, causing crashes. To solve this, we've moved the linked list to each interpreter.
(cherry picked from commit 58ef741)

Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>
ericsnowcurrently added a commit to ericsnowcurrently/cpython that referenced this pull request Aug 4, 2023
Yhg1s pushed a commit that referenced this pull request Aug 5, 2023
… Under Isolated Subinterpreters (gh-107567) (#107599)" (#107648)

Revert "[3.12] gh-107080: Fix Py_TRACE_REFS Crashes Under Isolated Subinterpreters (gh-107567) (#107599)"

This reverts commit 58af229.
ericsnowcurrently added a commit to ericsnowcurrently/cpython that referenced this pull request Aug 7, 2023
ericsnowcurrently added a commit that referenced this pull request Aug 7, 2023
This finishes fixing the crashes in Py_TRACE_REFS builds.  We missed this part in gh-107567.
ericsnowcurrently added a commit that referenced this pull request Aug 7, 2023
…lds (gh-107750)

This is a follow-up to gh-107567 and gh-107733.

We skip test_basic_multiple_interpreters_deleted_no_reset on tracerefs builds.  The test breaks interpreter isolation a little, which doesn't work well with Py_TRACE_REFS builds, so I feel fine about skipping the test.
Yhg1s pushed a commit that referenced this pull request Aug 16, 2023
…eters (#107751)

* Unrevert "[3.12] gh-107080: Fix Py_TRACE_REFS Crashes Under Isolated Subinterpreters (gh-107567) (#107599)".

This reverts commit 6e4eec7 (gh-107648).

* Initialize each interpreter's refchain properly.

* Skip test_basic_multiple_interpreters_deleted_no_reset on tracerefs builds.
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.

4 participants