Repository: python/cpython
Pull request: gh-102381: don't call watcher callback with dead object
Fixes #102381, #102351 and #102385.

Ensure that `func_dealloc`, `code_dealloc`, and `dict_dealloc` temporarily incref the function/code object/dict before calling the watcher callback, so we don't pass around dead objects.

A watcher callback could take a reference to the object; check for this and allow it to be resurrected in that case, rather than going ahead and freeing it (making the taken reference a pointer to freed memory.)

Also, don't pass the object itself to `PyErr_WriteUnraisable` if the callback raises, allowing an unraisablehook to take a reference to the object and resurrect it and, in the worst case, causing an infinite loop. Safer to construct an informative debug string and pass that as context to `PyErr_WriteUnraisable`. This actually provides more debug information than before, since the new message clarifies that the error occurred in a watcher callback for the mentioned object.

<!-- gh-issue-number: gh-102381 -->
* Issue: gh-102381
<!-- /gh-issue-number -->


Problem statement:
raising an exception in a dict/code/function watcher callback will segfault on a DEALLOCATED event
Because the object we pass into the callback is dead (zero refcount), and if the callback raises an error we try to `PyErr_WriteUnraisable` with that object as context, which increfs and decrefs it, which recursively again tries to deallocate it, and we end up in infinite recursion and overflowing the stack.

There are various other ways a callback could trigger this issue. We shouldn't pass around dead objects; instead `*_dealloc` should temporarily resurrect it before calling the callback.


<!-- gh-linked-prs -->
### Linked PRs
* gh-102382
<!-- /gh-linked-prs -->

Hints from issue discussion:

Commit message: Update code-object watcher docs on return value also

Patch to review:
diff --git a/Doc/c-api/code.rst b/Doc/c-api/code.rst
index 062ef3a1fea93c..31d9f9376995fc 100644
--- a/Doc/c-api/code.rst
+++ b/Doc/c-api/code.rst
@@ -172,6 +172,12 @@ bound into a function.
    before the destruction of *co* takes place, so the prior state of *co*
    can be inspected.
 
+   If *event* is ``PY_CODE_EVENT_DESTROY``, there may already be a pending
+   exception set on entry to the callback; in this case, the callback may not
+   execute Python code or otherwise disturb the pending exception. Taking a
+   reference in the callback to an about-to-be-destroyed code object will
+   resurrect it and prevent it from being freed.
+
    Users of this API should not rely on internal runtime implementation
    details. Such details may include, but are not limited to, the exact
    order and timing of creation and destruction of code objects. While
@@ -179,9 +185,10 @@ bound into a function.
    (including whether a callback is invoked or not), it does not change
    the semantics of the Python code being executed.
 
-   If the callback returns with an exception set, it must return ``-1``; this
-   exception will be printed as an unraisable exception using
-   :c:func:`PyErr_WriteUnraisable`. Otherwise it should return ``0``.
+   If the callback sets an exception, it must return ``-1``; this exception will
+   be printed as an unraisable exception using :c:func:`PyErr_WriteUnraisable`.
+   Otherwise (including if an exception was already set on entry to the
+   callback) it should return ``0``.
 
    .. versionadded:: 3.12
 
diff --git a/Doc/c-api/dict.rst b/Doc/c-api/dict.rst
index 34106ee6b1f30d..d468fd9ae71a30 100644
--- a/Doc/c-api/dict.rst
+++ b/Doc/c-api/dict.rst
@@ -298,13 +298,21 @@ Dictionary Objects
    dictionary.
 
    The callback may inspect but must not modify *dict*; doing so could have
-   unpredictable effects, including infinite recursion.
+   unpredictable effects, including infinite recursion. Do not trigger Python
+   code execution in the callback, as it could modify the dict as a side effect.
+
+   If *event* is ``PyDict_EVENT_DEALLOCATED``, there may already be a pending
+   exception set on entry to the callback; in this case, the callback may not
+   execute Python code or otherwise disturb the pending exception. Taking a new
+   reference in the callback to an about-to-be-destroyed dictionary will
+   resurrect it and prevent it from being freed.
 
    Callbacks occur before the notified modification to *dict* takes place, so
    the prior state of *dict* can be inspected.
 
-   If the callback returns with an exception set, it must return ``-1``; this
-   exception will be printed as an unraisable exception using
-   :c:func:`PyErr_WriteUnraisable`. Otherwise it should return ``0``.
+   If the callback sets an exception, it must return ``-1``; this exception will
+   be printed as an unraisable exception using :c:func:`PyErr_WriteUnraisable`.
+   Otherwise (including if a pending exception was already set on entry to the
+   callback) it should return ``0``.
 
    .. versionadded:: 3.12
diff --git a/Doc/c-api/function.rst b/Doc/c-api/function.rst
index bc7569d0add97d..47ca3cb5831682 100644
--- a/Doc/c-api/function.rst
+++ b/Doc/c-api/function.rst
@@ -173,8 +173,15 @@ There are a few functions specific to Python functions.
    runtime behavior depending on optimization decisions, it does not change
    the semantics of the Python code being executed.
 
-   If the callback returns with an exception set, it must return ``-1``; this
-   exception will be printed as an unraisable exception using
-   :c:func:`PyErr_WriteUnraisable`. Otherwise it should return ``0``.
+   If *event* is ``PyFunction_EVENT_DESTROY``, there may already be a pending
+   exception set on entry to the callback; in this case, the callback may not
+   execute Python code or otherwise disturb the pending exception. Taking a
+   reference in the callback to an about-to-be-destroyed function will resurrect
+   it and prevent it from being freed.
+
+   If the callback sets an exception, it must return ``-1``; this exception will
+   be printed as an unraisable exception using :c:func:`PyErr_WriteUnraisable`.
+   Otherwise (including if a pending exception was already set on entry to the
+   callback) it should return ``0``.
 
    .. versionadded:: 3.12
diff --git a/Include/internal/pycore_dict.h b/Include/internal/pycore_dict.h
index c74a3437713039..1af5e59a677a9a 100644
--- a/Include/internal/pycore_dict.h
+++ b/Include/internal/pycore_dict.h
@@ -164,6 +164,7 @@ _PyDict_NotifyEvent(PyDict_WatchEvent event,
                     PyObject *key,
                     PyObject *value)
 {
+    assert(Py_REFCNT((PyObject*)mp) > 0);
     int watcher_bits = mp->ma_version_tag & DICT_VERSION_MASK;
     if (watcher_bits) {
         _PyDict_SendEvent(watcher_bits, event, mp, key, value);
diff --git a/Lib/test/test_capi/test_watchers.py b/Lib/test/test_capi/test_watchers.py
index 1922614ef60558..98611f1598eda3 100644
--- a/Lib/test/test_capi/test_watchers.py
+++ b/Lib/test/test_capi/test_watchers.py
@@ -109,10 +109,21 @@ def test_error(self):
             self.watch(wid, d)
             with catch_unraisable_exception() as cm:
                 d["foo"] = "bar"
-                self.assertIs(cm.unraisable.object, d)
+                self.assertIn(
+                    "watcher callback for <dict at",
+                    cm.unraisable.object
+                )
                 self.assertEqual(str(cm.unraisable.exc_value), "boom!")
             self.assert_events([])
 
+    def test_dealloc_error(self):
+        d = {}
+        with self.watcher(kind=self.ERROR) as wid:
+            self.watch(wid, d)
+            with catch_unraisable_exception() as cm:
+                del d
+                self.assertEqual(str(cm.unraisable.exc_value), "boom!")
+
     def test_two_watchers(self):
         d1 = {}
         d2 = {}
@@ -389,6 +400,22 @@ def test_code_object_events_dispatched(self):
         del co4
         self.assert_event_counts(0, 0, 0, 0)
 
+    def test_error(self):
+        with self.code_watcher(2):
+            with catch_unraisable_exception() as cm:
+                co = _testcapi.code_newempty("test_watchers", "dummy0", 0)
+
+                self.assertEqual(cm.unraisable.object, f"watcher callback for {co!r}")
+                self.assertEqual(str(cm.unraisable.exc_value), "boom!")
+
+    def test_dealloc_error(self):
+        co = _testcapi.code_newempty("test_watchers", "dummy0", 0)
+        with self.code_watcher(2):
+            with catch_unraisable_exception() as cm:
+                del co
+
+                self.assertEqual(str(cm.unraisable.exc_value), "boom!")
+
     def test_clear_out_of_range_watcher_id(self):
         with self.assertRaisesRegex(ValueError, r"Invalid code watcher ID -1"):
             _testcapi.clear_code_watcher(-1)
@@ -479,7 +506,25 @@ def watcher(*args):
                 def myfunc():
                     pass
 
-                self.assertIs(cm.unraisable.object, myfunc)
+                self.assertEqual(
+                    cm.unraisable.object,
+                    f"watcher callback for {myfunc!r}"
+                )
+
+    def test_dealloc_watcher_raises_error(self):
+        class MyError(Exception):
+            pass
+
+        def watcher(*args):
+            raise MyError("testing 123")
+
+        def myfunc():
+            pass
+
+        with self.add_watcher(watcher):
+            with catch_unraisable_exception() as cm:
+                del myfunc
+
                 self.assertIsInstance(cm.unraisable.exc_value, MyError)
 
     def test_clear_out_of_range_watcher_id(self):
diff --git a/Modules/_testcapi/watchers.c b/Modules/_testcapi/watchers.c
index d9ace632768ae8..0e046950e0cc61 100644
--- a/Modules/_testcapi/watchers.c
+++ b/Modules/_testcapi/watchers.c
@@ -317,6 +317,13 @@ noop_code_event_handler(PyCodeEvent event, PyCodeObject *co)
     return 0;
 }
 
+static int
+error_code_event_handler(PyCodeEvent event, PyCodeObject *co)
+{
+    PyErr_SetString(PyExc_RuntimeError, "boom!");
+    return -1;
+}
+
 static PyObject *
 add_code_watcher(PyObject *self, PyObject *which_watcher)
 {
@@ -333,7 +340,11 @@ add_code_watcher(PyObject *self, PyObject *which_watcher)
         num_code_object_created_events[1] = 0;
         num_code_object_destroyed_events[1] = 0;
     }
+    else if (which_l == 2) {
+        watcher_id = PyCode_AddWatcher(error_code_event_handler);
+    }
     else {
+        PyErr_Format(PyExc_ValueError, "invalid watcher %d", which_l);
         return NULL;
     }
     if (watcher_id < 0) {
diff --git a/Objects/codeobject.c b/Objects/codeobject.c
index 175bd57568f8f6..8417ac558945b6 100644
--- a/Objects/codeobject.c
+++ b/Objects/codeobject.c
@@ -11,9 +11,12 @@
 #include "pycore_tuple.h"         // _PyTuple_ITEMS()
 #include "clinic/codeobject.c.h"
 
+static PyObject* code_repr(PyCodeObject *co);
+
 static void
 notify_code_watchers(PyCodeEvent event, PyCodeObject *co)
 {
+    assert(Py_REFCNT(co) > 0);
     PyInterpreterState *interp = _PyInterpreterState_GET();
     assert(interp->_initialized);
     uint8_t bits = interp->active_code_watchers;
@@ -25,7 +28,20 @@ notify_code_watchers(PyCodeEvent event, PyCodeObject *co)
             // callback must be non-null if the watcher bit is set
             assert(cb != NULL);
             if (cb(event, co) < 0) {
-                PyErr_WriteUnraisable((PyObject *) co);
+                // Don't risk resurrecting the object if an unraisablehook keeps
+                // a reference; pass a string as context.
+                PyObject *context = NULL;
+                PyObject *repr = code_repr(co);
+                if (repr) {
+                    context = PyUnicode_FromFormat("watcher callback for %U", repr);
+                    Py_DECREF(repr);
+                }
+                if (context == NULL) {
+                    context = Py_None;
+                    Py_INCREF(context);
+                }
+                PyErr_WriteUnraisable(context);
+                Py_DECREF(context);
             }
         }
         i++;
@@ -1667,7 +1683,14 @@ code_new_impl(PyTypeObject *type, int argcount, int posonlyargcount,
 static void
 code_dealloc(PyCodeObject *co)
 {
+    assert(Py_REFCNT(co) == 0);
+    Py_SET_REFCNT(co, 1);
     notify_code_watchers(PY_CODE_EVENT_DESTROY, co);
+    if (Py_REFCNT(co) > 1) {
+        Py_DECREF(co);
+        return;
+    }
+    Py_SET_REFCNT(co, 0);
 
     if (co->co_extra != NULL) {
         PyInterpreterState *interp = _PyInterpreterState_GET();
diff --git a/Objects/dictobject.c b/Objects/dictobject.c
index fc658ca2f4b7f8..d08eb934766a21 100644
--- a/Objects/dictobject.c
+++ b/Objects/dictobject.c
@@ -2308,7 +2308,14 @@ _PyDict_FromKeys(PyObject *cls, PyObject *iterable, PyObject *value)
 static void
 dict_dealloc(PyDictObject *mp)
 {
+    assert(Py_REFCNT(mp) == 0);
+    Py_SET_REFCNT(mp, 1);
     _PyDict_NotifyEvent(PyDict_EVENT_DEALLOCATED, mp, NULL, NULL);
+    if(Py_REFCNT(mp) > 1) {
+        Py_DECREF(mp);
+        return;
+    }
+    Py_SET_REFCNT(mp, 0);
     PyDictValues *values = mp->ma_values;
     PyDictKeysObject *keys = mp->ma_keys;
     Py_ssize_t i, n;
@@ -5744,9 +5751,17 @@ _PyDict_SendEvent(int watcher_bits,
         if (watcher_bits & 1) {
             PyDict_WatchCallback cb = interp->dict_state.watchers[i];
             if (cb && (cb(event, (PyObject*)mp, key, value) < 0)) {
-                // some dict modification paths (e.g. PyDict_Clear) can't raise, so we
-                // can't propagate exceptions from dict watchers.
-                PyErr_WriteUnraisable((PyObject *)mp);
+                // We don't want to resurrect the dict by potentially having an
+                // unraisablehook keep a reference to it, so we don't pass the
+                // dict as context, just an informative string message.  Dict
+                // repr can call arbitrary code, so we invent a simpler version.
+                PyObject *context = PyUnicode_FromFormat("watcher callback for <dict at %p>", mp);
+                if (context == NULL) {
+                    context = Py_None;
+                    Py_INCREF(context);
+                }
+                PyErr_WriteUnraisable(context);
+                Py_DECREF(context);
             }
         }
         watcher_bits >>= 1;
diff --git a/Objects/funcobject.c b/Objects/funcobject.c
index 91a6b3dd40a232..ef7a7f490581bf 100644
--- a/Objects/funcobject.c
+++ b/Objects/funcobject.c
@@ -8,6 +8,8 @@
 #include "pycore_pyerrors.h"      // _PyErr_Occurred()
 #include "structmember.h"         // PyMemberDef
 
+static PyObject* func_repr(PyFunctionObject *op);
+
 static void
 notify_func_watchers(PyInterpreterState *interp, PyFunction_WatchEvent event,
                      PyFunctionObject *func, PyObject *new_value)
@@ -21,7 +23,20 @@ notify_func_watchers(PyInterpreterState *interp, PyFunction_WatchEvent event,
             // callback must be non-null if the watcher bit is set
             assert(cb != NULL);
             if (cb(event, func, new_value) < 0) {
-                PyErr_WriteUnraisable((PyObject *) func);
+                // Don't risk resurrecting the func if an unraisablehook keeps a
+                // reference; pass a string as context.
+                PyObject *context = NULL;
+                PyObject *repr = func_repr(func);
+                if (repr != NULL) {
+                    context = PyUnicode_FromFormat("watcher callback for %U", repr);
+                    Py_DECREF(repr);
+                }
+                if (context == NULL) {
+                    context = Py_None;
+                    Py_INCREF(context);
+                }
+                PyErr_WriteUnraisable(context);
+                Py_DECREF(context);
             }
         }
         i++;
@@ -33,6 +48,7 @@ static inline void
 handle_func_event(PyFunction_WatchEvent event, PyFunctionObject *func,
                   PyObject *new_value)
 {
+    assert(Py_REFCNT(func) > 0);
     PyInterpreterState *interp = _PyInterpreterState_GET();
     assert(interp->_initialized);
     if (interp->active_func_watchers) {
@@ -766,7 +782,14 @@ func_clear(PyFunctionObject *op)
 static void
 func_dealloc(PyFunctionObject *op)
 {
+    assert(Py_REFCNT(op) == 0);
+    Py_SET_REFCNT(op, 1);
     handle_func_event(PyFunction_EVENT_DESTROY, op, NULL);
+    if (Py_REFCNT(op) > 1) {
+        Py_DECREF(op);
+        return;
+    }
+    Py_SET_REFCNT(op, 0);
     _PyObject_GC_UNTRACK(op);
     if (op->func_weakreflist != NULL) {
         PyObject_ClearWeakRefs((PyObject *) op);


Code review comments:
diff_hunk: @@ -2308,7 +2308,14 @@ _PyDict_FromKeys(PyObject *cls, PyObject *iterable, PyObject *value)
 static void
 dict_dealloc(PyDictObject *mp)
 {
+    assert(Py_REFCNT(mp) == 0);
+    Py_SET_REFCNT(mp, 1);
     _PyDict_NotifyEvent(PyDict_EVENT_DEALLOCATED, mp, NULL, NULL);
+    if(Py_REFCNT(mp) > 1) {
+        Py_DECREF(mp);
path: Objects/dictobject.c
text: @user1:
Py_SET_REFCNT here too.

diff_hunk: @@ -173,8 +173,15 @@ There are a few functions specific to Python functions.
    runtime behavior depending on optimization decisions, it does not change
    the semantics of the Python code being executed.
 
-   If the callback returns with an exception set, it must return ``-1``; this
-   exception will be printed as an unraisable exception using
-   :c:func:`PyErr_WriteUnraisable`. Otherwise it should return ``0``.
+   If *event* is ``PyFunction_EVENT_DESTROY``, there may already be a pending
path: Doc/c-api/function.rst
text: @user1:
Does it make sense to guarantee that the other event handlers are never called with an exception set?

Is it forbidden or problematic for the callback to store the pending exception, call API functions, and restore it before returning 0? (The wording seems to imply so.)

@author:
I don't want to make this a guarantee for the callbacks, because it's quite expensive to check `PyErr_Occurred()` and then save/restore the exception. The intended use case for these callbacks is performance-sensitive, and generally should not involve the callback doing anything that even uses the CPython C API; just doing some cheap updates to internal bookkeeping of e.g. a JIT.

But you're right that if a callback really wants to save/clear/restore the exception itself and do something with the C API while it's cleared, that should be fine. I'll update the wording accordingly.

EDIT: I think I misinterpreted your comment. You said "other event handlers" -- I'm thinking now that you meant I shouldn't word things so as to sound like only the dealloc event can be called with exception set. I don't _think_ the other events will ever be called with exception set, but I don't know that for sure. I guess it's probably safer to just say callbacks generally need to be prepared to handle this.

diff_hunk: @@ -21,7 +23,20 @@ notify_func_watchers(PyInterpreterState *interp, PyFunction_WatchEvent event,
             // callback must be non-null if the watcher bit is set
             assert(cb != NULL);
             if (cb(event, func, new_value) < 0) {
-                PyErr_WriteUnraisable((PyObject *) func);
+                // Don't risk resurrecting the func if an unraisablehook keeps a
+                // reference; pass a string as context.
+                PyObject *context = NULL;
+                PyObject *repr = func_repr(func);
+                if (repr != NULL) {
+                    context = PyUnicode_FromFormat("watcher callback for %U", repr);
+                    Py_DECREF(repr);
+                }
+                if (context == NULL) {
+                    context = Py_None;
+                    Py_INCREF(context);
path: Objects/funcobject.c
text: @user1:
```suggestion
                    context = Py_NewRef(Py_None);
```

diff_hunk: @@ -5744,9 +5751,17 @@ _PyDict_SendEvent(int watcher_bits,
         if (watcher_bits & 1) {
             PyDict_WatchCallback cb = interp->dict_state.watchers[i];
             if (cb && (cb(event, (PyObject*)mp, key, value) < 0)) {
-                // some dict modification paths (e.g. PyDict_Clear) can't raise, so we
-                // can't propagate exceptions from dict watchers.
-                PyErr_WriteUnraisable((PyObject *)mp);
+                // We don't want to resurrect the dict by potentially having an
+                // unraisablehook keep a reference to it, so we don't pass the
+                // dict as context, just an informative string message.  Dict
+                // repr can call arbitrary code, so we invent a simpler version.
+                PyObject *context = PyUnicode_FromFormat("watcher callback for <dict at %p>", mp);
+                if (context == NULL) {
+                    context = Py_None;
+                    Py_INCREF(context);
path: Objects/dictobject.c
text: @user1:
```suggestion
                    context = Py_NewRef(Py_None);
```

diff_hunk: @@ -1667,7 +1683,14 @@ code_new_impl(PyTypeObject *type, int argcount, int posonlyargcount,
 static void
 code_dealloc(PyCodeObject *co)
 {
+    assert(Py_REFCNT(co) == 0);
+    Py_SET_REFCNT(co, 1);
     notify_code_watchers(PY_CODE_EVENT_DESTROY, co);
+    if (Py_REFCNT(co) > 1) {
+        Py_DECREF(co);
path: Objects/codeobject.c
text: @user1:
Don't call Py_DECREF here; when checking refcounts (i.e. a Py_DEBUG build) DECREF and INCREF need to be balanced. Since the refcount was increased by calling Py_SET_REFCNT, you need to decrease it the same way.

@author:
Ah right, you mentioned this in debugging, but clearly it didn't stick :) I'll remember now.

It would be nice if _something_ in CI would catch this problem. Even refleak buildbots didn't catch it. I guess it would just hide out until someone manually noticed a discrepancy in `gettotalrefcount()`? If so, it would surprise me if we don't already have such discrepancies hiding some places.

diff_hunk: @@ -766,7 +782,14 @@ func_clear(PyFunctionObject *op)
 static void
 func_dealloc(PyFunctionObject *op)
 {
+    assert(Py_REFCNT(op) == 0);
+    Py_SET_REFCNT(op, 1);
     handle_func_event(PyFunction_EVENT_DESTROY, op, NULL);
+    if (Py_REFCNT(op) > 1) {
+        Py_DECREF(op);
path: Objects/funcobject.c
text: @user1:
Py_SET_REFCNT here too.

diff_hunk: @@ -2308,7 +2308,14 @@ _PyDict_FromKeys(PyObject *cls, PyObject *iterable, PyObject *value)
 static void
 dict_dealloc(PyDictObject *mp)
 {
+    assert(Py_REFCNT(mp) == 0);
+    Py_SET_REFCNT(mp, 1);
     _PyDict_NotifyEvent(PyDict_EVENT_DEALLOCATED, mp, NULL, NULL);
+    if(Py_REFCNT(mp) > 1) {
path: Objects/dictobject.c
text: @user1:
```suggestion
    if (Py_REFCNT(mp) > 1) {
```

diff_hunk: @@ -25,7 +28,20 @@ notify_code_watchers(PyCodeEvent event, PyCodeObject *co)
             // callback must be non-null if the watcher bit is set
             assert(cb != NULL);
             if (cb(event, co) < 0) {
-                PyErr_WriteUnraisable((PyObject *) co);
+                // Don't risk resurrecting the object if an unraisablehook keeps
+                // a reference; pass a string as context.
+                PyObject *context = NULL;
+                PyObject *repr = code_repr(co);
+                if (repr) {
+                    context = PyUnicode_FromFormat("watcher callback for %U", repr);
+                    Py_DECREF(repr);
+                }
+                if (context == NULL) {
+                    context = Py_None;
+                    Py_INCREF(context);
path: Objects/codeobject.c
text: @user1:
```suggestion
                    context = Py_NewRef(Py_None);
```