Skip to content

_PyObject_ResurrectEnd may deallocate object in the "resurrection" case #130202

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.

Sign up for GitHub

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

Jump to bottom
Closed
colesbury opened this issue Feb 16, 2025 · 0 comments
Closed

_PyObject_ResurrectEnd may deallocate object in the "resurrection" case #130202

colesbury opened this issue Feb 16, 2025 · 0 comments
Assignees
@colesbury
Labels
topic-free-threading type-bug An unexpected behavior, bug, or error type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@colesbury
Copy link
Contributor

colesbury commented Feb 16, 2025 •
edited by bedevere-app bot
Loading

Bug report

See #130163 (comment) for reproducer.

Yeah, I think that's another bug with _PyObject_ResurrectEnd. In the resurrection case, the object may still be deallocated. In particular, _Py_brc_queue_object() may immediately free the object or lead to it being freed by another thread. The calls below _PyObject_ResurrectEnd() are not safe:

cpython/Objects/object.c

Lines 594 to 604 in 655fc8a

if (!_PyObject_ResurrectEnd(self)) {
return 0; /* this is the normal path out */
}
/* tp_finalize resurrected it! Make it look like the original Py_DECREF
* never happened. */
_Py_ResurrectReference(self);
_PyObject_ASSERT(self,
(!_PyType_IS_GC(Py_TYPE(self))
|| _PyObject_GC_IS_TRACKED(self)));

We either need to ensure that the object remains alive after _PyObject_ResurrectEnd() returns 1, or rearrange the resurrection code.

Originally posted by @colesbury in #130163

I am not yet sure how we want to fix this.

Linked PRs
@colesbury colesbury added topic-free-threading type-bug An unexpected behavior, bug, or error type-crash A hard crash of the interpreter, possibly with a core dump labels Feb 16, 2025
@colesbury colesbury mentioned this issue Feb 16, 2025
Closed
@colesbury colesbury self-assigned this Feb 16, 2025
colesbury added a commit to colesbury/cpython that referenced this issue Feb 18, 2025
@colesbury
pythongh-130202: Fix bug in _PyObject_ResurrectEnd in free threaded…
a6dc1d7 
… build

This fixes a fairly subtle bug involving finalizers and resurrection in
debug free threaded builds: if `_PyObject_ResurrectEnd` returns `1`
(i.e., the object was resurrected by a finalizer), it's not safe to
access the object because it might still be deallocated. For example:

 * The finalizer may have exposed the object to another thread. That
   thread may hold the last reference and concurrently deallocate it any
   time after `_PyObject_ResurrectEnd()` returns `1`.
 * `_PyObject_ResurrectEnd()` may call `_Py_brc_queue_object()`, which
   may internally deallocate the object immediately if the owning thread
   is dead.

Therefore, it's important not to access the object after it's
resurrected. We only violate this in two cases, and only in debug
builds:

 * We assert that the object is tracked appropriately. This is now moved
   up betewen the finalizer and the `_PyObject_ResurrectEnd()` call.

 * The `--with-trace-refs` builds may need to remember the object if
   it's resurrected. This is now handled by `_PyObject_ResurrectStart()`
   and `_PyObject_ResurrectEnd()`.

Note that `--with-trace-refs` is currently disabled in `--disable-gil`
builds because the refchain hash table isn't thread-safe, but this
refactoring avoids an additional thread-safety issue.
@bedevere-app bedevere-app bot mentioned this issue Feb 18, 2025
Merged
colesbury added a commit to colesbury/cpython that referenced this issue Feb 18, 2025
@colesbury
pythongh-130202: Fix bug in _PyObject_ResurrectEnd in free threaded…
bfac5b2 
… build

This fixes a fairly subtle bug involving finalizers and resurrection in
debug free threaded builds: if `_PyObject_ResurrectEnd` returns `1`
(i.e., the object was resurrected by a finalizer), it's not safe to
access the object because it might still be deallocated. For example:

 * The finalizer may have exposed the object to another thread. That
   thread may hold the last reference and concurrently deallocate it any
   time after `_PyObject_ResurrectEnd()` returns `1`.
 * `_PyObject_ResurrectEnd()` may call `_Py_brc_queue_object()`, which
   may internally deallocate the object immediately if the owning thread
   is dead.

Therefore, it's important not to access the object after it's
resurrected. We only violate this in two cases, and only in debug
builds:

 * We assert that the object is tracked appropriately. This is now moved
   up betewen the finalizer and the `_PyObject_ResurrectEnd()` call.

 * The `--with-trace-refs` builds may need to remember the object if
   it's resurrected. This is now handled by `_PyObject_ResurrectStart()`
   and `_PyObject_ResurrectEnd()`.

Note that `--with-trace-refs` is currently disabled in `--disable-gil`
builds because the refchain hash table isn't thread-safe, but this
refactoring avoids an additional thread-safety issue.
colesbury added a commit to colesbury/cpython that referenced this issue Feb 18, 2025
@colesbury
Merge branch 'main' into pythongh-130202-resurrect
95d9dc8
colesbury added a commit that referenced this issue Feb 25, 2025
@colesbury
gh-130202: Fix bug in _PyObject_ResurrectEnd in free threaded build (…
f963239 
…gh-130281)

This fixes a fairly subtle bug involving finalizers and resurrection in
debug free threaded builds: if `_PyObject_ResurrectEnd` returns `1`
(i.e., the object was resurrected by a finalizer), it's not safe to
access the object because it might still be deallocated. For example:

 * The finalizer may have exposed the object to another thread. That
   thread may hold the last reference and concurrently deallocate it any
   time after `_PyObject_ResurrectEnd()` returns `1`.
 * `_PyObject_ResurrectEnd()` may call `_Py_brc_queue_object()`, which
   may internally deallocate the object immediately if the owning thread
   is dead.

Therefore, it's important not to access the object after it's
resurrected. We only violate this in two cases, and only in debug
builds:

 * We assert that the object is tracked appropriately. This is now moved
   up betewen the finalizer and the `_PyObject_ResurrectEnd()` call.

 * The `--with-trace-refs` builds may need to remember the object if
   it's resurrected. This is now handled by `_PyObject_ResurrectStart()`
   and `_PyObject_ResurrectEnd()`.

Note that `--with-trace-refs` is currently disabled in `--disable-gil`
builds because the refchain hash table isn't thread-safe, but this
refactoring avoids an additional thread-safety issue.
seehwan pushed a commit to seehwan/cpython that referenced this issue Apr 16, 2025
@colesbury @seehwan80
pythongh-130202: Fix bug in _PyObject_ResurrectEnd in free threaded…
5b962df 
… build (pythongh-130281)

This fixes a fairly subtle bug involving finalizers and resurrection in
debug free threaded builds: if `_PyObject_ResurrectEnd` returns `1`
(i.e., the object was resurrected by a finalizer), it's not safe to
access the object because it might still be deallocated. For example:

 * The finalizer may have exposed the object to another thread. That
   thread may hold the last reference and concurrently deallocate it any
   time after `_PyObject_ResurrectEnd()` returns `1`.
 * `_PyObject_ResurrectEnd()` may call `_Py_brc_queue_object()`, which
   may internally deallocate the object immediately if the owning thread
   is dead.

Therefore, it's important not to access the object after it's
resurrected. We only violate this in two cases, and only in debug
builds:

 * We assert that the object is tracked appropriately. This is now moved
   up betewen the finalizer and the `_PyObject_ResurrectEnd()` call.

 * The `--with-trace-refs` builds may need to remember the object if
   it's resurrected. This is now handled by `_PyObject_ResurrectStart()`
   and `_PyObject_ResurrectEnd()`.

Note that `--with-trace-refs` is currently disabled in `--disable-gil`
builds because the refchain hash table isn't thread-safe, but this
refactoring avoids an additional thread-safety issue.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@colesbury colesbury

Labels
topic-free-threading type-bug An unexpected behavior, bug, or error type-crash A hard crash of the interpreter, possibly with a core dump
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@colesbury