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-135106: Restrict trashcan to GC'ed objects by markshannon · Pull Request #135682 · python/cpython

Conversation

@markshannon

@markshannon markshannon commented Jun 18, 2025 •
edited by bedevere-app Bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

… and thread the trashcan list through the GC header

  • Issue: test_weakref.test_threaded_weak_key_dict_deepcopy crash: merged objects should have ob_tid == 0 #135106

Sorry, something went wrong.

Uh oh!

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

All reactions
@markshannon
Restrict trashcan to GC'ed objects and thread the trashcan list throu…
2d81b6b
…gh the GC header
@bedevere-app bedevere-app Bot mentioned this pull request Jun 18, 2025
test_weakref.test_threaded_weak_key_dict_deepcopy crash: merged objects should have ob_tid == 0 #135106
Closed
colesbury
colesbury reviewed Jun 18, 2025
View reviewed changes
Comment thread Objects/object.c Outdated
PyThreadState *tstate = _PyThreadState_GET();
intptr_t margin = _Py_RecursionLimit_GetMargin(tstate);
if (margin < 2) {
if (margin < 2 && PyObject_IS_GC(op)) {

@colesbury colesbury Jun 18, 2025

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I think you also need to guard the _PyTrash_thread_destroy_chain() below; otherwise destructors for simple types (like PyLongObject) can trigger arbitrary code execution.

That checks seems a bit more difficult to add efficiently.

Sorry, something went wrong.

Uh oh!

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

All reactions

@markshannon markshannon Jun 18, 2025

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

otherwise destructors for simple types (like PyLongObject) can trigger arbitrary code execution.

Does that matter?
We only care that PyStackRef_CLOSE_SPECIALIZED doesn't escape and that doesn't (at least, shouldn't) call Py_DECREF.

Sorry, something went wrong.

Uh oh!

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

All reactions

@colesbury colesbury Jun 18, 2025

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

  1. PyStackRef_CLOSE_SPECIALIZED calls Py_DECREF in the free threaded build. This is fixable, but requires more changes.
  2. obj.attr += 1 wouldn't be atomic in the GIL-enabled build when using a lot of stack (already not atomic in free threaded build).
  3. I think there are places in C code where we assume that Py_DECREF() on certain types of objects doesn't escape and won't invalidate borrowed references. One I encountered is:

    cpython/Objects/typeobject.c

    Line 9345 in 46c60e0

    if (key != NULL && PyDict_DelItem(subclasses, key)) {
    where key is a PyLongObject

Sorry, something went wrong.

Uh oh!

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

All reactions

@markshannon markshannon Jun 19, 2025

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

We can do it reasonably efficiently with a read of tp_flags. Since we are reading tp_dealloc anyway, the latency is likely hidden.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Objects/object.c Outdated
}
/* It is possible that the object has been accessed through
* a weak ref, so only free if refcount == 0) */
if (Py_REFCNT(op) == 0) {

@colesbury colesbury Jun 18, 2025

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I think you already brought this up in the meeting, but I don't think this should be necessary. Weakrefs won't resurrect an object with refcount of zero, which _PyTrash_thread_deposit_object() ensures.

Sorry, something went wrong.

Uh oh!

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

All reactions

@markshannon markshannon Jun 18, 2025

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I'll put the assert back.

Sorry, something went wrong.

Uh oh!

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

All reactions
markshannon added 4 commits June 18, 2025 18:49
@markshannon
Go back to asserting refcnt == 0
f89ddfd
@markshannon
Don't empty trashcan unless object is GC'd
7101e8f
@markshannon
Handle tp_is_gc in _PyTrash_thread_deposit_object
a81bb72
@markshannon
Tidy a bit
98c9efd
@markshannon
markshannon marked this pull request as ready for review June 19, 2025 13:59
@markshannon
markshannon requested a review from ZeroIntensity as a code owner June 19, 2025 13:59
@bedevere-app bedevere-app Bot added the awaiting core review label Jun 19, 2025
@markshannon markshannon added the needs backport to 3.14 bugs and security fixes label Jun 19, 2025
@markshannon
markshannon requested a review from nascheme June 20, 2025 15:30
@nascheme

nascheme commented Jun 20, 2025

Copy link
Copy Markdown
Member

LGTM.

All reactions

Sorry, something went wrong.

Uh oh!

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

nascheme
nascheme approved these changes Jun 23, 2025
View reviewed changes
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Jun 23, 2025
@markshannon
markshannon merged commit 2060089 into python:main Jun 24, 2025
@miss-islington-app

miss-islington-app Bot commented Jun 24, 2025

Copy link
Copy Markdown

Thanks @markshannon for the PR 🌮🎉.. I'm working now to backport this PR to: 3.14.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-app bedevere-app Bot removed the awaiting merge label Jun 24, 2025
@markshannon
markshannon deleted the fix-trashcan branch June 24, 2025 08:49
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 24, 2025
@markshannon @miss-islington
pythonGH-135106: Restrict trashcan to GC'ed objects (pythonGH-135682)
ada81dd
(cherry picked from commit 2060089)

Co-authored-by: Mark Shannon <[email protected]>
@bedevere-app

bedevere-app Bot commented Jun 24, 2025

Copy link
Copy Markdown

GH-135876 is a backport of this pull request to the 3.14 branch.

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-app bedevere-app Bot removed the needs backport to 3.14 bugs and security fixes label Jun 24, 2025
hugovk pushed a commit that referenced this pull request Jul 7, 2025
@miss-islington @markshannon
[3.14] GH-135106: Restrict trashcan to GC'ed objects (GH-135682) (#13…
46a7981
…5876)

Co-authored-by: Mark Shannon <[email protected]>
AndPuQing pushed a commit to AndPuQing/cpython that referenced this pull request Jul 11, 2025
@markshannon @AndPuQing
pythonGH-135106: Restrict trashcan to GC'ed objects (pythonGH-135682)
9831101
Pranjal095 pushed a commit to Pranjal095/cpython that referenced this pull request Jul 12, 2025
@markshannon @Pranjal095
pythonGH-135106: Restrict trashcan to GC'ed objects (pythonGH-135682)
153fe08
taegyunkim pushed a commit to taegyunkim/cpython that referenced this pull request Aug 4, 2025
@markshannon @taegyunkim
pythonGH-135106: Restrict trashcan to GC'ed objects (pythonGH-135682)
d46977b
Agent-Hellboy pushed a commit to Agent-Hellboy/cpython that referenced this pull request Aug 19, 2025
@markshannon @Agent-Hellboy
pythonGH-135106: Restrict trashcan to GC'ed objects (pythonGH-135682)
ccdc965
mandolaerik added a commit to mandolaerik/cpython that referenced this pull request Jan 23, 2026
@mandolaerik
Revert "[3.14] pythonGH-135106: Restrict trashcan to GC'ed objects (p…
a47da5f
…ythonGH-135682) (python#135876)"

This reverts commit 46a7981.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@colesbury colesbury colesbury left review comments
@nascheme nascheme nascheme approved these changes
@ZeroIntensity ZeroIntensity Awaiting requested review from ZeroIntensity ZeroIntensity is a code owner

Uh oh!

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

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

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

3 participants

@markshannon @nascheme @colesbury
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.