Skip to content

Commit

Permalink
pythongh-115832: Fix instrumentation version mismatch during interpre…
Browse files Browse the repository at this point in the history
…ter shutdown (python#115856)

A previous commit introduced a bug to `interpreter_clear()`: it set
`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 the version check in bytecodes.c will see a different result
than the one in instrumentation.c causing an infinite loop.

The fix itself is straightforward: clear `tstate->eval_breaker` when clearing
`interp->ceval.instrumentation_version`.
  • Loading branch information
swtaarrs authored and adorilson committed Mar 25, 2024
1 parent 15f78e2 commit beb1957
Show file tree
Hide file tree
Showing 4 changed files with 53 additions and 2 deletions.
30 changes: 30 additions & 0 deletions Lib/test/_test_monitoring_shutdown.py
Original file line number Diff line number Diff line change
@@ -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())
12 changes: 11 additions & 1 deletion Lib/test/test_monitoring.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@
import types
import unittest
import asyncio
from test.support import requires_specialization
from test import support
from test.support import requires_specialization, script_helper

PAIR = (0,1)

Expand Down Expand Up @@ -1858,3 +1859,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)
10 changes: 9 additions & 1 deletion Python/instrumentation.c
Original file line number Diff line number Diff line change
Expand Up @@ -891,8 +891,16 @@ static inline int most_significant_bit(uint8_t bits) {
static uint32_t
global_version(PyInterpreterState *interp)
{
return (uint32_t)_Py_atomic_load_uintptr_relaxed(
uint32_t version = (uint32_t)_Py_atomic_load_uintptr_relaxed(
&interp->ceval.instrumentation_version);
#ifdef Py_DEBUG
PyThreadState *tstate = _PyThreadState_GET();
uint32_t thread_version =
(uint32_t)(_Py_atomic_load_uintptr_relaxed(&tstate->eval_breaker) &
~_PY_EVAL_EVENTS_MASK);
assert(thread_version == version);
#endif
return version;
}

/* Atomically set the given version in the given location, without touching
Expand Down
3 changes: 3 additions & 0 deletions Python/pystate.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down

0 comments on commit beb1957

Please sign in to comment.