GeezerVison

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.

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.

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.

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.

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.

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.

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?

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.