pythongh-115832: Fix instrumentation version mismatch during interpre…

…ter shutdown

In python/cpython@bc613e4ecb3c, I introduced a bug to `interpreter_clear()`: it
sets `interp->ceval.instrumentation_version` to 0, without making the
corresponding change to `tstate->eval_breaker` (which holds a thread-local copy
of the version). After this happens, Python code can still run due to object
finalizers during a GC, and [this version check in
bytecodes.c](https://github.com/python/cpython/blob/4ee6bdfbaa792a3aa93c65c2022a89bd2d1e0894/Python/bytecodes.c#L147-L152)
will see a different result than [this one in
instrumentation.c](https://github.com/python/cpython/blob/4ee6bdfbaa792a3aa93c65c2022a89bd2d1e0894/Python/instrumentation.c#L894-L895),
causing an infinite loop.

The fix itself is straightforward, and is what I should've done in
`interpreter_clear()` in the first place: also clear `tstate->eval_breaker`
when clearing `interp->ceval.instrumentation_version`. I also restored a
comment that I'm not sure why I deleted in the original commit.

To make bugs of this type less likely in the future, I changed
`instrumentation.c:global_version()` to read the version from a
`PyThreadState*` rather than a `PyInterpreterState*`, so it's reading the
version from the same location as the interpreter loop. This had some fan-out
effects on callers, although a lot of them were passing `tstate->interp`
already, so this made them (very) slighty shorter.

- Issue: pythongh-115832

Include/internal/pycore_code.h

@@ -516,7 +516,7 @@ adaptive_counter_backoff(uint16_t counter) {
 
 #define COMPARISON_NOT_EQUALS (COMPARISON_UNORDERED | COMPARISON_LESS_THAN | COMPARISON_GREATER_THAN)
 
-extern int _Py_Instrument(PyCodeObject *co, PyInterpreterState *interp);
+extern int _Py_Instrument(PyCodeObject *co, PyThreadState *tstate);
 
 extern int _Py_GetBaseOpcode(PyCodeObject *code, int offset);
 

Lib/test/_test_monitoring_shutdown.py

@@ -0,0 +1,30 @@
+#!/usr/bin/env python3
+
+# gh-115832: An object destructor running during the final GC of interpreter
+# shutdown triggered an infinite loop in the instrumentation code.
+
+import sys
+
+class CallableCycle:
+    def __init__(self):
+        self._cycle = self
+
+    def __del__(self):
+        pass
+
+    def __call__(self, code, instruction_offset):
+        pass
+
+def tracefunc(frame, event, arg):
+    pass
+
+def main():
+    tool_id = sys.monitoring.PROFILER_ID
+    event_id = sys.monitoring.events.PY_START
+
+    sys.monitoring.use_tool_id(tool_id, "test profiler")
+    sys.monitoring.set_events(tool_id, event_id)
+    sys.monitoring.register_callback(tool_id, event_id, CallableCycle())
+
+if __name__ == "__main__":
+    sys.exit(main())

Lib/test/test_monitoring.py

@@ -9,6 +9,8 @@
 import types
 import unittest
 import asyncio
+from test import support
+from test.support import script_helper
 
 PAIR = (0,1)
 
@@ -1853,3 +1855,12 @@ def test_func(recorder):
             sys.monitoring.register_callback(TEST_TOOL, E.LINE, None)
             sys.monitoring.set_events(TEST_TOOL, 0)
         self.assertGreater(len(events), 250)
+
+class TestMonitoringAtShutdown(unittest.TestCase):
+
+    def test_monitoring_live_at_shutdown(self):
+        # gh-115832: An object destructor running during the final GC of
+        # interpreter shutdown triggered an infinite loop in the
+        # instrumentation code.
+        script = support.findfile("_test_monitoring_shutdown.py")
+        script_helper.run_test_script(script)

Python/bytecodes.c

@@ -150,7 +150,7 @@ dummy_func(
             uintptr_t code_version = _PyFrame_GetCode(frame)->_co_instrumentation_version;
             assert((code_version & 255) == 0);
             if (code_version != global_version) {
-                int err = _Py_Instrument(_PyFrame_GetCode(frame), tstate->interp);
+                int err = _Py_Instrument(_PyFrame_GetCode(frame), tstate);
                 ERROR_IF(err, error);
                 next_instr = this_instr;
             }
@@ -177,7 +177,7 @@ dummy_func(
             uintptr_t global_version = _Py_atomic_load_uintptr_relaxed(&tstate->eval_breaker) & ~_PY_EVAL_EVENTS_MASK;
             uintptr_t code_version = _PyFrame_GetCode(frame)->_co_instrumentation_version;
             if (code_version != global_version) {
-                if (_Py_Instrument(_PyFrame_GetCode(frame), tstate->interp)) {
+                if (_Py_Instrument(_PyFrame_GetCode(frame), tstate)) {
                     GOTO_ERROR(error);
                 }
                 next_instr = this_instr;

Python/ceval.c

@@ -736,7 +736,7 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, _PyInterpreterFrame *frame, int
         }
         /* Because this avoids the RESUME,
          * we need to update instrumentation */
-        _Py_Instrument(_PyFrame_GetCode(frame), tstate->interp);
+        _Py_Instrument(_PyFrame_GetCode(frame), tstate);
         monitor_throw(tstate, frame, frame->instr_ptr);
         /* TO DO -- Monitor throw entry. */
         goto resume_with_error;

Python/instrumentation.c

@@ -889,10 +889,10 @@ static inline int most_significant_bit(uint8_t bits) {
 }
 
 static uint32_t
-global_version(PyInterpreterState *interp)
+global_version(PyThreadState *tstate)
 {
-    return (uint32_t)_Py_atomic_load_uintptr_relaxed(
-        &interp->ceval.instrumentation_version);
+    return (uint32_t)(_Py_atomic_load_uintptr_relaxed(&tstate->eval_breaker) &
+                      ~_PY_EVAL_EVENTS_MASK);
 }
 
 /* Atomically set the given version in the given location, without touching
@@ -931,9 +931,9 @@ set_global_version(PyThreadState *tstate, uint32_t version)
 }
 
 static bool
-is_version_up_to_date(PyCodeObject *code, PyInterpreterState *interp)
+is_version_up_to_date(PyCodeObject *code, PyThreadState *tstate)
 {
-    return global_version(interp) == code->_co_instrumentation_version;
+    return global_version(tstate) == code->_co_instrumentation_version;
 }
 
 #ifndef NDEBUG
@@ -948,8 +948,9 @@ instrumentation_cross_checks(PyInterpreterState *interp, PyCodeObject *code)
 #endif
 
 static inline uint8_t
-get_tools_for_instruction(PyCodeObject *code, PyInterpreterState *interp, int i, int event)
+get_tools_for_instruction(PyCodeObject *code, PyThreadState *tstate, int i, int event)
 {
+    PyInterpreterState *interp = tstate->interp;
     uint8_t tools;
     assert(event != PY_MONITORING_EVENT_LINE);
     assert(event != PY_MONITORING_EVENT_INSTRUCTION);
@@ -959,7 +960,7 @@ get_tools_for_instruction(PyCodeObject *code, PyInterpreterState *interp, int i,
         event = PY_MONITORING_EVENT_CALL;
     }
     if (PY_MONITORING_IS_INSTRUMENTED_EVENT(event)) {
-        CHECK(is_version_up_to_date(code, interp));
+        CHECK(is_version_up_to_date(code, tstate));
         CHECK(instrumentation_cross_checks(interp, code));
         if (code->_co_monitoring->tools) {
             tools = code->_co_monitoring->tools[i];
@@ -1018,7 +1019,7 @@ call_instrumentation_vector(
     assert(args[2] == NULL);
     args[2] = offset_obj;
     PyInterpreterState *interp = tstate->interp;
-    uint8_t tools = get_tools_for_instruction(code, interp, offset, event);
+    uint8_t tools = get_tools_for_instruction(code, tstate, offset, event);
     Py_ssize_t nargsf = nargs | PY_VECTORCALL_ARGUMENTS_OFFSET;
     PyObject **callargs = &args[1];
     int err = 0;
@@ -1156,7 +1157,7 @@ int
 _Py_call_instrumentation_line(PyThreadState *tstate, _PyInterpreterFrame* frame, _Py_CODEUNIT *instr, _Py_CODEUNIT *prev)
 {
     PyCodeObject *code = _PyFrame_GetCode(frame);
-    assert(is_version_up_to_date(code, tstate->interp));
+    assert(is_version_up_to_date(code, tstate));
     assert(instrumentation_cross_checks(tstate->interp, code));
     int i = (int)(instr - _PyCode_CODE(code));
 
@@ -1258,7 +1259,7 @@ int
 _Py_call_instrumentation_instruction(PyThreadState *tstate, _PyInterpreterFrame* frame, _Py_CODEUNIT *instr)
 {
     PyCodeObject *code = _PyFrame_GetCode(frame);
-    assert(is_version_up_to_date(code, tstate->interp));
+    assert(is_version_up_to_date(code, tstate));
     assert(instrumentation_cross_checks(tstate->interp, code));
     int offset = (int)(instr - _PyCode_CODE(code));
     _PyCoMonitoringData *instrumentation_data = code->_co_monitoring;
@@ -1587,11 +1588,12 @@ update_instrumentation_data(PyCodeObject *code, PyInterpreterState *interp)
 }
 
 int
-_Py_Instrument(PyCodeObject *code, PyInterpreterState *interp)
+_Py_Instrument(PyCodeObject *code, PyThreadState *tstate)
 {
-    if (is_version_up_to_date(code, interp)) {
+    PyInterpreterState *interp = tstate->interp;
+    if (is_version_up_to_date(code, tstate)) {
         assert(
-            interp->ceval.instrumentation_version == 0 ||
+            global_version(tstate) == 0 ||
             instrumentation_cross_checks(interp, code)
         );
         return 0;
@@ -1628,7 +1630,7 @@ _Py_Instrument(PyCodeObject *code, PyInterpreterState *interp)
         assert(monitors_are_empty(monitors_and(new_events, removed_events)));
     }
     code->_co_monitoring->active_monitors = active_events;
-    code->_co_instrumentation_version = global_version(interp);
+    code->_co_instrumentation_version = global_version(tstate);
     if (monitors_are_empty(new_events) && monitors_are_empty(removed_events)) {
 #ifdef INSTRUMENT_DEBUG
         sanity_check_instrumentation(code);
@@ -1746,7 +1748,7 @@ instrument_all_executing_code_objects(PyInterpreterState *interp) {
         _PyInterpreterFrame *frame = ts->current_frame;
         while (frame) {
             if (frame->owner != FRAME_OWNED_BY_CSTACK) {
-                if (_Py_Instrument(_PyFrame_GetCode(frame), interp)) {
+                if (_Py_Instrument(_PyFrame_GetCode(frame), ts)) {
                     return -1;
                 }
             }
@@ -1814,7 +1816,7 @@ _PyMonitoring_SetEvents(int tool_id, _PyMonitoringEventSet events)
         return 0;
     }
     set_events(&interp->monitors, tool_id, events);
-    uint32_t new_version = global_version(interp) + MONITORING_VERSION_INCREMENT;
+    uint32_t new_version = global_version(tstate) + MONITORING_VERSION_INCREMENT;
     if (new_version == 0) {
         PyErr_Format(PyExc_OverflowError, "events set too many times");
         return -1;
@@ -1828,7 +1830,8 @@ int
 _PyMonitoring_SetLocalEvents(PyCodeObject *code, int tool_id, _PyMonitoringEventSet events)
 {
     assert(0 <= tool_id && tool_id < PY_MONITORING_TOOL_IDS);
-    PyInterpreterState *interp = _PyInterpreterState_GET();
+    PyThreadState *tstate = _PyThreadState_GET();
+    PyInterpreterState *interp = tstate->interp;
     assert(events < (1 << _PY_MONITORING_LOCAL_EVENTS));
     if (code->_co_firsttraceable >= Py_SIZE(code)) {
         PyErr_Format(PyExc_SystemError, "cannot instrument shim code object '%U'", code->co_name);
@@ -1846,12 +1849,12 @@ _PyMonitoring_SetLocalEvents(PyCodeObject *code, int tool_id, _PyMonitoringEvent
         return 0;
     }
     set_local_events(local, tool_id, events);
-    if (is_version_up_to_date(code, interp)) {
+    if (is_version_up_to_date(code, tstate)) {
         /* Force instrumentation update */
         code->_co_instrumentation_version -= MONITORING_VERSION_INCREMENT;
     }
     _Py_Executors_InvalidateDependency(interp, code);
-    if (_Py_Instrument(code, interp)) {
+    if (_Py_Instrument(code, tstate)) {
         return -1;
     }
     return 0;
@@ -2150,7 +2153,7 @@ monitoring_restart_events_impl(PyObject *module)
      */
     PyThreadState *tstate = _PyThreadState_GET();
     PyInterpreterState *interp = tstate->interp;
-    uint32_t restart_version = global_version(interp) + MONITORING_VERSION_INCREMENT;
+    uint32_t restart_version = global_version(tstate) + MONITORING_VERSION_INCREMENT;
     uint32_t new_version = restart_version + MONITORING_VERSION_INCREMENT;
     if (new_version <= MONITORING_VERSION_INCREMENT) {
         PyErr_Format(PyExc_OverflowError, "events set too many times");

Python/pystate.c

@@ -795,7 +795,10 @@ interpreter_clear(PyInterpreterState *interp, PyThreadState *tstate)
 
     Py_CLEAR(interp->audit_hooks);
 
+    // At this time, all the threads should be cleared so we don't need atomic
+    // operations for instrumentation_version or eval_breaker.
     interp->ceval.instrumentation_version = 0;
+    tstate->eval_breaker = 0;
 
     for (int i = 0; i < _PY_MONITORING_UNGROUPED_EVENTS; i++) {
         interp->monitors.tools[i] = 0;