From 372eabc5c8e1c84f12fbad9c068b067449183c5e Mon Sep 17 00:00:00 2001 From: Mark Shannon Date: Mon, 5 May 2025 12:01:32 +0100 Subject: [PATCH 1/5] Make sure that the GC doesn't untrack objects in trashcan --- Objects/object.c | 44 ++++++++++++++++++++++++++++++++++++++------ Python/gc.c | 1 + 2 files changed, 39 insertions(+), 6 deletions(-) diff --git a/Objects/object.c b/Objects/object.c index 0974a231ec101a..326a6da7eb0982 100644 --- a/Objects/object.c +++ b/Objects/object.c @@ -2930,6 +2930,39 @@ Py_ReprLeave(PyObject *obj) /* Trashcan support. */ +/* We need to store a pointer in the refcount field of + * an object. It is important that we never store 0 (NULL). +* It is also important to not make the object appear immortal, +* or it might be untracked by the cycle GC. */ +uintptr_t +pointer_to_safe_refcount(void *ptr) +{ + uintptr_t full = (uintptr_t)ptr; + assert((full & 3) == 0); +#if defined(Py_GIL_DISABLED) + return full + 1; +#else + uint32_t refcnt = (uint32_t)full; + if (refcnt >= (uint32_t)_Py_IMMORTAL_MINIMUM_REFCNT) { + full = full - ((uintptr_t)_Py_IMMORTAL_MINIMUM_REFCNT) + 1; + } + return full + 2; +#endif +} + +void * +safe_refcount_to_pointer(uintptr_t refcnt) +{ +#if defined(Py_GIL_DISABLED) + return (void *)(refcnt - 1); +#else + if (refcnt & 1) { + refcnt += _Py_IMMORTAL_MINIMUM_REFCNT - 1; + } + return (void *)(refcnt - 2); +#endif +} + /* Add op to the gcstate->trash_delete_later list. Called when the current * call-stack depth gets large. op must be a currently untracked gc'ed * object, with refcount 0. Py_DECREF must already have been called on it. @@ -2941,11 +2974,10 @@ _PyTrash_thread_deposit_object(PyThreadState *tstate, PyObject *op) #ifdef Py_GIL_DISABLED op->ob_tid = (uintptr_t)tstate->delete_later; #else - /* Store the delete_later pointer in the refcnt field. - * As this object may still be tracked by the GC, - * it is important that we never store 0 (NULL). */ - uintptr_t refcnt = (uintptr_t)tstate->delete_later; - *((uintptr_t*)op) = refcnt+1; + /* Store the delete_later pointer in the refcnt field. */ + uintptr_t refcnt = pointer_to_safe_refcount(tstate->delete_later); + *((uintptr_t*)op) = refcnt; + assert(!_Py_IsImmortal(op)); #endif tstate->delete_later = op; } @@ -2967,7 +2999,7 @@ _PyTrash_thread_destroy_chain(PyThreadState *tstate) /* Get the delete_later pointer from the refcnt field. * See _PyTrash_thread_deposit_object(). */ uintptr_t refcnt = *((uintptr_t*)op); - tstate->delete_later = (PyObject *)(refcnt - 1); + tstate->delete_later = safe_refcount_to_pointer(refcnt); op->ob_refcnt = 0; #endif diff --git a/Python/gc.c b/Python/gc.c index b7b48c8af39c4a..7b0e6d6e803504 100644 --- a/Python/gc.c +++ b/Python/gc.c @@ -493,6 +493,7 @@ update_refs(PyGC_Head *containers) next = GC_NEXT(gc); PyObject *op = FROM_GC(gc); if (_Py_IsImmortal(op)) { + assert(!_Py_IsStaticImmortal(op)); _PyObject_GC_UNTRACK(op); gc = next; continue; From c18073c453b53a566136863b092d8d13ee544eaf Mon Sep 17 00:00:00 2001 From: Mark Shannon Date: Mon, 5 May 2025 12:03:54 +0100 Subject: [PATCH 2/5] Add news --- .../2025-05-05-12-03-46.gh-issue-133261.bL1gqz.rst | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 Misc/NEWS.d/next/Core_and_Builtins/2025-05-05-12-03-46.gh-issue-133261.bL1gqz.rst diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2025-05-05-12-03-46.gh-issue-133261.bL1gqz.rst b/Misc/NEWS.d/next/Core_and_Builtins/2025-05-05-12-03-46.gh-issue-133261.bL1gqz.rst new file mode 100644 index 00000000000000..1f40ea7eb8843b --- /dev/null +++ b/Misc/NEWS.d/next/Core_and_Builtins/2025-05-05-12-03-46.gh-issue-133261.bL1gqz.rst @@ -0,0 +1,3 @@ +Fix bug where the cycle GC could untrack objects in the trashcan because +they looked like they were immortal. When objects are added to the trashcan, +we take care to ensure they keep a mortal reference count. From ee9580327ba9e845373184ab7ec6f5154e12169c Mon Sep 17 00:00:00 2001 From: Mark Shannon Date: Mon, 5 May 2025 12:15:23 +0100 Subject: [PATCH 3/5] Make functions static --- Objects/object.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Objects/object.c b/Objects/object.c index 326a6da7eb0982..17b9cb4512f21a 100644 --- a/Objects/object.c +++ b/Objects/object.c @@ -2934,7 +2934,7 @@ Py_ReprLeave(PyObject *obj) * an object. It is important that we never store 0 (NULL). * It is also important to not make the object appear immortal, * or it might be untracked by the cycle GC. */ -uintptr_t +static uintptr_t pointer_to_safe_refcount(void *ptr) { uintptr_t full = (uintptr_t)ptr; @@ -2950,7 +2950,7 @@ pointer_to_safe_refcount(void *ptr) #endif } -void * +static void * safe_refcount_to_pointer(uintptr_t refcnt) { #if defined(Py_GIL_DISABLED) From cb357f9169be4fde62621f4f0b440ea2c30d5c6a Mon Sep 17 00:00:00 2001 From: Mark Shannon Date: Mon, 5 May 2025 12:46:15 +0100 Subject: [PATCH 4/5] Helpers are needed for no-gil build --- Objects/object.c | 8 -------- 1 file changed, 8 deletions(-) diff --git a/Objects/object.c b/Objects/object.c index 17b9cb4512f21a..60a01d6623b6ed 100644 --- a/Objects/object.c +++ b/Objects/object.c @@ -2939,28 +2939,20 @@ pointer_to_safe_refcount(void *ptr) { uintptr_t full = (uintptr_t)ptr; assert((full & 3) == 0); -#if defined(Py_GIL_DISABLED) - return full + 1; -#else uint32_t refcnt = (uint32_t)full; if (refcnt >= (uint32_t)_Py_IMMORTAL_MINIMUM_REFCNT) { full = full - ((uintptr_t)_Py_IMMORTAL_MINIMUM_REFCNT) + 1; } return full + 2; -#endif } static void * safe_refcount_to_pointer(uintptr_t refcnt) { -#if defined(Py_GIL_DISABLED) - return (void *)(refcnt - 1); -#else if (refcnt & 1) { refcnt += _Py_IMMORTAL_MINIMUM_REFCNT - 1; } return (void *)(refcnt - 2); -#endif } /* Add op to the gcstate->trash_delete_later list. Called when the current From 48a244dc776725954a54be75b9481d8948514b42 Mon Sep 17 00:00:00 2001 From: Mark Shannon Date: Mon, 5 May 2025 13:06:07 +0100 Subject: [PATCH 5/5] Functions aren't needed for no-gil build --- Objects/object.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Objects/object.c b/Objects/object.c index 60a01d6623b6ed..f5fe25eb5aaf8f 100644 --- a/Objects/object.c +++ b/Objects/object.c @@ -2930,6 +2930,7 @@ Py_ReprLeave(PyObject *obj) /* Trashcan support. */ +#ifndef Py_GIL_DISABLED /* We need to store a pointer in the refcount field of * an object. It is important that we never store 0 (NULL). * It is also important to not make the object appear immortal, @@ -2954,6 +2955,7 @@ safe_refcount_to_pointer(uintptr_t refcnt) } return (void *)(refcnt - 2); } +#endif /* Add op to the gcstate->trash_delete_later list. Called when the current * call-stack depth gets large. op must be a currently untracked gc'ed