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

Non-thread-safe use of Py_SET_REFCNT in destructors #127582

Closed
colesbury opened this issue Dec 3, 2024 · 0 comments
Closed

Non-thread-safe use of Py_SET_REFCNT in destructors #127582

colesbury opened this issue Dec 3, 2024 · 0 comments
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 Dec 3, 2024

Bug report

When calling finalizers from dealloc, we temporarily increase the refcount with Py_SET_REFCNT and then decrease it again (also with Py_SET_REFCNT). This isn't thread-safe in the free threading build because some other thread may concurrently try to increase the refcount during a racy dictionary or list access.

cpython/Objects/object.c

Lines 552 to 566 in dabcecf

/* Temporarily resurrect the object. */
Py_SET_REFCNT(self, 1);
PyObject_CallFinalizer(self);
_PyObject_ASSERT_WITH_MSG(self,
Py_REFCNT(self) > 0,
"refcount is too small");
/* Undo the temporary resurrection; can't use DECREF here, it would
* cause a recursive call. */
Py_SET_REFCNT(self, Py_REFCNT(self) - 1);
if (Py_REFCNT(self) == 0) {
return 0; /* this is the normal path out */
}

We have similar issues with some watcher events:

cpython/Objects/funcobject.c

Lines 1095 to 1102 in dabcecf

assert(Py_REFCNT(op) == 0);
Py_SET_REFCNT(op, 1);
handle_func_event(PyFunction_EVENT_DESTROY, op, NULL);
if (Py_REFCNT(op) > 1) {
Py_SET_REFCNT(op, Py_REFCNT(op) - 1);
return;
}
Py_SET_REFCNT(op, 0);

This can lead to crashes when we have racy accesses to objects that may be concurrently finalized. For example:

./python -m test test_free_threading -m test_racing_load_super_attr -v -F

Test (un)specialization of LOAD_SUPER_ATTR opcode. ... ./Include/internal/pycore_object.h:593: _Py_NegativeRefcount: Assertion failed: object has negative ref count
Enable tracemalloc to get the memory block allocation traceback

<object at 0x200041800f0 is freed>

Fatal Python error: _PyObject_AssertFailed: _PyObject_AssertFailed
Python runtime state: initialized

(Observed more frequently on macOS, but also occurs on Linux)

In macOS buildbot: https://buildbot.python.org/#/builders/1368/builds/2251/steps/6/logs/stdio

Linked PRs

@colesbury colesbury added type-bug An unexpected behavior, bug, or error topic-free-threading type-crash A hard crash of the interpreter, possibly with a core dump labels Dec 3, 2024
colesbury added a commit to colesbury/cpython that referenced this issue Dec 4, 2024
…ing.

Objects may be temporarily "resurrected" in destructors when calling
finalizers or watcher callbacks. We previously undid the resurrection
by decrementing the reference count using `Py_SET_REFCNT`. This was not
thread-safe because other threads might be accessing the object
(modifying its reference count) if it was exposed by the finalizer,
watcher callback, or temporarily accessed by a racy dictionary or list
access.

This adds internal-only thread-safe functions for temporary object
resurrection during destructors.
colesbury added a commit that referenced this issue Dec 5, 2024
…H-127612)

Objects may be temporarily "resurrected" in destructors when calling
finalizers or watcher callbacks. We previously undid the resurrection
by decrementing the reference count using `Py_SET_REFCNT`. This was not
thread-safe because other threads might be accessing the object
(modifying its reference count) if it was exposed by the finalizer,
watcher callback, or temporarily accessed by a racy dictionary or list
access.

This adds internal-only thread-safe functions for temporary object
resurrection during destructors.
colesbury added a commit to colesbury/cpython that referenced this issue Dec 5, 2024
… threading. (pythonGH-127612)

Objects may be temporarily "resurrected" in destructors when calling
finalizers or watcher callbacks. We previously undid the resurrection
by decrementing the reference count using `Py_SET_REFCNT`. This was not
thread-safe because other threads might be accessing the object
(modifying its reference count) if it was exposed by the finalizer,
watcher callback, or temporarily accessed by a racy dictionary or list
access.

This adds internal-only thread-safe functions for temporary object
resurrection during destructors.
(cherry picked from commit f4f5308)

Co-authored-by: Sam Gross <[email protected]>
colesbury added a commit that referenced this issue Dec 5, 2024
…ding. (GH-127612) (GH-127659)

Objects may be temporarily "resurrected" in destructors when calling
finalizers or watcher callbacks. We previously undid the resurrection
by decrementing the reference count using `Py_SET_REFCNT`. This was not
thread-safe because other threads might be accessing the object
(modifying its reference count) if it was exposed by the finalizer,
watcher callback, or temporarily accessed by a racy dictionary or list
access.

This adds internal-only thread-safe functions for temporary object
resurrection during destructors.
(cherry picked from commit f4f5308)
srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this issue Jan 8, 2025
…ing. (pythonGH-127612)

Objects may be temporarily "resurrected" in destructors when calling
finalizers or watcher callbacks. We previously undid the resurrection
by decrementing the reference count using `Py_SET_REFCNT`. This was not
thread-safe because other threads might be accessing the object
(modifying its reference count) if it was exposed by the finalizer,
watcher callback, or temporarily accessed by a racy dictionary or list
access.

This adds internal-only thread-safe functions for temporary object
resurrection during destructors.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

No branches or pull requests

1 participant