gh-158711: Ensure legacy tracing ignores PyMonitoring_Fire* by godlygeek · Pull Request #158894 · python/cpython

godlygeek · 2026-10-06T04:27:19Z

sys.setprofile and sys.settrace callbacks are no longer called for sys.monitoring events that extension modules fire for their own code objects through the monitoring C API.

Until now, the sys.setprofile or sys.settrace function would get run, but it would be passed the top frame on the Python stack rather than anything related to the code object passed to the monitoring C API, and this would manifest to the legacy tracing function as nonsensical duplicate events rather than anything that could be attributed to the code object passed to the monitoring API.

Issue: PyMonitoring_FirePyStartEvent results in incorrect duplicate calls to sys.setprofile profile functions #158711

`sys.setprofile` and `sys.settrace` callbacks are no longer called for `sys.monitoring` events that extension modules fire for their own code objects through the monitoring C API. Until now, the `sys.setprofile` or `sys.settrace` function would get run, but it would be passed the top frame on the Python stack rather than anything related to the code object passed to the monitoring C API, and this would manifest to the legacy tracing function as nonsensical duplicate events rather than anything that could be attributed to the code object passed to the monitoring API.

godlygeek · 2026-10-06T04:30:32Z

-            ( 1, E.PY_RESUME, capi.fire_event_py_resume),
-            ( 1, E.PY_YIELD, capi.fire_event_py_yield, 10),
-            ( 1, E.PY_RETURN, capi.fire_event_py_return, 20),
-            ( 2, E.CALL, capi.fire_event_call, callable, 40),


Note: The removed 1st tuple element (never reflected in the above comment!) was the expected number of calls that would be seen. It was 1 for everything except E.CALL, where it was 2 instead. It was 2 for E.CALL because we'd see both a call to the monitoring API and a call to the Python function that triggered the call to the monitoring API. Instead of having this count of expected calls, the test now uses a Counter class that looks only for calls for a particular CodeLike and ignores all others, which makes it so that the expected count is 1 for everything instead of for everything but CALL.

godlygeek · 2026-10-06T04:37:51Z

            sys.monitoring.register_callback(TEST_TOOL, event, counter)
            if event == E.C_RETURN or event == E.C_RAISE:
                sys.monitoring.set_events(TEST_TOOL, E.CALL)
+                event_value = int(math.log2(E.CALL))


Note for reviewer: previously this would call

self.Scope(self.codelike, math.log2(E.C_RETURN))

which results in an out-of-bounds read inside PyMonitoring_EnterScope. It accesses m->tools[event], and m->tools is an array of 16 elements, and PY_MONITORING_EVENT_C_RETURN is 16 and so one past the end of that array.

This was wrong; you get the C_RETURN event by subscribing to C_CALL events. That's what this fix is.

godlygeek · 2026-10-06T04:40:39Z

+    if (PyVectorcall_NARGS(nargsf) != 4) {
+        // PyMonitoring_FireCReturnEvent and PyMonitoring_FireCRaiseEvent
+        // don't provide the callable.
+        assert(PyVectorcall_NARGS(nargsf) == 3);


Note for reviewer: this fixes an assertion error that would be hit on debug builds whenever you call PyMonitoring_FireCReturnEvent or PyMonitoring_FireCRaiseEvent with a sys.setprofile legacy profile function installed.

godlygeek · 2026-10-06T04:41:47Z

    assert(!PyErr_Occurred());
    assert(kwnames == NULL);
    assert(PyVectorcall_NARGS(nargsf) == 3);
-    assert(PyCode_Check(args[0]));


Note for reviewer: this would trigger an assertion error in the tests, where we pass a CodeLike instead of a code object. It only didn't fail up until now because the path wasn't being exercised.

godlygeek · 2026-10-06T04:43:59Z

+    PyFrameObject *frame = get_top_frame_if_running_code(args[0]);
+    if (frame == NULL) {
+        Py_RETURN_NONE;
+    }


Note for reviewer: this check is moved up so that we know that the code object we received is actually a PyCodeObject before we cast it to one.

pablogsal · 2026-10-11T18:18:44Z

            return NULL;
        }
-        PyObject *res =  call_profile_func(self, meth);
+        PyObject *res =  call_profile_func(self, args[0], meth);


Can we check the code mismatch before tp_descr_get? Reporting list.append with arg0=1 still raises TypeError before reaching this guard, preventing delivery to sys.monitoring.

pablogsal · 2026-10-11T18:18:44Z

            return NULL;
        }
-        PyObject *res =  call_profile_func(self, meth);
+        PyObject *res =  call_profile_func(self, args[0], meth);


This still does the tp_descr_get before the frame check, so a PyMonitoring_FireCallEvent with a method descriptor and an arg0 of another type fails with a TypeError under sys.setprofile instead of being ignored (and otherwise we create a bound method just to throw it away). I think we should do the frame check at the top of sys_profile_call_or_return. Can you add a test for this case as well?

pablogsal · 2026-10-11T18:18:44Z


+/* Return the top frame (borrowed) if it's running `code`, else NULL. */
+static PyFrameObject *
+get_top_frame_if_running_code(PyObject *code)


Why not mask the legacy tools out in PyMonitoring_EnterScope (or in capi_call_instrumentation) instead? Tool ids 6 and 7 are only used by sys.setprofile/sys.settrace, so something like

state_array[i].active = m->tools[event] & ~LEGACY_TOOLS;

would keep all these handlers untouched, and extensions would not even pay for the call (and the PyEval_GetFrame()) when only legacy tracing is active. Am I missing a case where we want these handlers to run for C API events?

Huh, that approach didn't occur to me, but I do think it's cleaner.

Am I missing a case where we want these handlers to run for C API events?

The only case I can think of is perhaps if the user has called _PyInterpreterState_SetEvalFrameFunc, and is calling the monitoring C API from their own frame evaluation function? In that case they would have a real frame on the PyEval_GetFrame() stack, and the existing behavior would be right for them.

I've updated the implementation to that. I left the tests alone. Let me know if you'd like to pare down the tests as well, now that all of these events are being ignored in one common place, instead of in many separate places.

Instead of making the callbacks associated with the `sys.setprofile` and `sys.settrace` legacy trace functions detect when they were called with something that's not a code object or not the code object held by the top frame on the Python stack, sidestep the problem entirely by making sure that the legacy profile functions are masked out of the set of tools to fire events for when using the monitoring C API.

godlygeek · 2026-10-11T23:04:22Z

The failed test from the latest run seems unrelated to my changes...

bedevere-app Bot added the awaiting review label Oct 6, 2026

bedevere-app Bot mentioned this pull request Oct 6, 2026

PyMonitoring_FirePyStartEvent results in incorrect duplicate calls to sys.setprofile profile functions #158711

Open

godlygeek commented Oct 6, 2026

View reviewed changes

pablogsal self-requested a review October 11, 2026 17:59

pablogsal reviewed Oct 11, 2026

View reviewed changes

Navigation Menu

Repository navigation

gh-158711: Ensure legacy tracing ignores PyMonitoring_Fire* - #158894

gh-158711: Ensure legacy tracing ignores PyMonitoring_Fire*#158894
godlygeek wants to merge 2 commits into
python:mainfrom
godlygeek:ensure_legacy_tracing_ignores_pymonitoring_fire

godlygeek commented Oct 6, 2026 •

edited by bedevere-app Bot

Loading

Uh oh!

godlygeek Oct 6, 2026

Uh oh!

godlygeek Oct 6, 2026 •

edited

Loading

Uh oh!

godlygeek Oct 6, 2026 •

edited

Loading

Uh oh!

godlygeek Oct 6, 2026 •

edited

Loading

Uh oh!

godlygeek Oct 6, 2026 •

edited

Loading

Uh oh!

pablogsal Oct 11, 2026

Uh oh!

pablogsal Oct 11, 2026

Uh oh!

pablogsal Oct 11, 2026

Uh oh!

godlygeek Oct 11, 2026

Uh oh!

godlygeek Oct 11, 2026

Uh oh!

godlygeek commented Oct 11, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

godlygeek commented Oct 6, 2026 • edited by bedevere-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

godlygeek Oct 6, 2026

Choose a reason for hiding this comment

Uh oh!

godlygeek Oct 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

godlygeek Oct 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

godlygeek Oct 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

godlygeek Oct 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pablogsal Oct 11, 2026

Choose a reason for hiding this comment

Uh oh!

pablogsal Oct 11, 2026

Choose a reason for hiding this comment

Uh oh!

pablogsal Oct 11, 2026

Choose a reason for hiding this comment

Uh oh!

godlygeek Oct 11, 2026

Choose a reason for hiding this comment

Uh oh!

godlygeek Oct 11, 2026

Choose a reason for hiding this comment

Uh oh!

godlygeek commented Oct 11, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

godlygeek commented Oct 6, 2026 •

edited by bedevere-app Bot

Loading

godlygeek Oct 6, 2026 •

edited

Loading

godlygeek Oct 6, 2026 •

edited

Loading

godlygeek Oct 6, 2026 •

edited

Loading

godlygeek Oct 6, 2026 •

edited

Loading