GH-106895: Raise a ValueError when attempting to disable events that cannot be disabled. (GH-107337)

Author: markshannon

Include/internal/pycore_instruments.h

@@ -28,7 +28,8 @@ extern "C" {
 #define PY_MONITORING_EVENT_BRANCH 8
 #define PY_MONITORING_EVENT_STOP_ITERATION 9
 
-#define PY_MONITORING_INSTRUMENTED_EVENTS 10
+#define PY_MONITORING_IS_INSTRUMENTED_EVENT(ev) \
+    ((ev) <= PY_MONITORING_EVENT_STOP_ITERATION)
 
 /* Other events, mainly exceptions */
 

Lib/test/test_monitoring.py

@@ -136,20 +136,27 @@ def test_c_return_count(self):
 
 E = sys.monitoring.events
 
-SIMPLE_EVENTS = [
+INSTRUMENTED_EVENTS = [
     (E.PY_START, "start"),
     (E.PY_RESUME, "resume"),
     (E.PY_RETURN, "return"),
     (E.PY_YIELD, "yield"),
     (E.JUMP, "jump"),
     (E.BRANCH, "branch"),
+]
+
+EXCEPT_EVENTS = [
     (E.RAISE, "raise"),
     (E.PY_UNWIND, "unwind"),
     (E.EXCEPTION_HANDLED, "exception_handled"),
+]
+
+SIMPLE_EVENTS = INSTRUMENTED_EVENTS + EXCEPT_EVENTS + [
     (E.C_RAISE, "c_raise"),
     (E.C_RETURN, "c_return"),
 ]
 
+
 SIMPLE_EVENT_SET = functools.reduce(operator.or_, [ev for (ev, _) in SIMPLE_EVENTS], 0) | E.CALL
 
 
@@ -618,6 +625,49 @@ def func2():
 
         self.check_lines(func2, [1,2,3,4,5,6])
 
+class TestDisable(MonitoringTestBase, unittest.TestCase):
+
+    def gen(self, cond):
+        for i in range(10):
+            if cond:
+                yield 1
+            else:
+                yield 2
+
+    def raise_handle_reraise(self):
+        try:
+            1/0
+        except:
+            raise
+
+    def test_disable_legal_events(self):
+        for event, name in INSTRUMENTED_EVENTS:
+            try:
+                counter = CounterWithDisable()
+                counter.disable = True
+                sys.monitoring.register_callback(TEST_TOOL, event, counter)
+                sys.monitoring.set_events(TEST_TOOL, event)
+                for _ in self.gen(1):
+                    pass
+                self.assertLess(counter.count, 4)
+            finally:
+                sys.monitoring.set_events(TEST_TOOL, 0)
+                sys.monitoring.register_callback(TEST_TOOL, event, None)
+
+
+    def test_disable_illegal_events(self):
+        for event, name in EXCEPT_EVENTS:
+            try:
+                counter = CounterWithDisable()
+                counter.disable = True
+                sys.monitoring.register_callback(TEST_TOOL, event, counter)
+                sys.monitoring.set_events(TEST_TOOL, event)
+                with self.assertRaises(ValueError):
+                    self.raise_handle_reraise()
+            finally:
+                sys.monitoring.set_events(TEST_TOOL, 0)
+                sys.monitoring.register_callback(TEST_TOOL, event, None)
+
 
 class ExceptionRecorder:
 

Misc/NEWS.d/next/Core and Builtins/2023-07-26-18-53-34.gh-issue-106895.DdEwV8.rst

@@ -0,0 +1,2 @@
+Raise a ``ValueError`` when a monitoring callback funtion returns
+``DISABLE`` for events that cannot be disabled locally.

Objects/classobject.c

@@ -48,6 +48,7 @@ method_vectorcall(PyObject *method, PyObject *const *args,
     PyObject *self = PyMethod_GET_SELF(method);
     PyObject *func = PyMethod_GET_FUNCTION(method);
     Py_ssize_t nargs = PyVectorcall_NARGS(nargsf);
+    assert(nargs == 0 || args[nargs-1]);
 
     PyObject *result;
     if (nargsf & PY_VECTORCALL_ARGUMENTS_OFFSET) {
@@ -56,6 +57,7 @@ method_vectorcall(PyObject *method, PyObject *const *args,
         nargs += 1;
         PyObject *tmp = newargs[0];
         newargs[0] = self;
+        assert(newargs[nargs-1]);
         result = _PyObject_VectorcallTstate(tstate, func, newargs,
                                             nargs, kwnames);
         newargs[0] = tmp;

Python/bytecodes.c

@@ -2672,7 +2672,12 @@ dummy_func(
             assert(val && PyExceptionInstance_Check(val));
             exc = PyExceptionInstance_Class(val);
             tb = PyException_GetTraceback(val);
-            Py_XDECREF(tb);
+            if (tb == NULL) {
+                tb = Py_None;
+            }
+            else {
+                Py_DECREF(tb);
+            }
             assert(PyLong_Check(lasti));
             (void)lasti; // Shut up compiler warning if asserts are off
             PyObject *stack[4] = {NULL, exc, val, tb};

Python/ceval.c

@@ -193,7 +193,7 @@ static int monitor_stop_iteration(PyThreadState *tstate,
 static void monitor_unwind(PyThreadState *tstate,
                  _PyInterpreterFrame *frame,
                  _Py_CODEUNIT *instr);
-static void monitor_handled(PyThreadState *tstate,
+static int monitor_handled(PyThreadState *tstate,
                  _PyInterpreterFrame *frame,
                  _Py_CODEUNIT *instr, PyObject *exc);
 static void monitor_throw(PyThreadState *tstate,
@@ -886,7 +886,9 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, _PyInterpreterFrame *frame, int
             PyObject *exc = _PyErr_GetRaisedException(tstate);
             PUSH(exc);
             JUMPTO(handler);
-            monitor_handled(tstate, frame, next_instr, exc);
+            if (monitor_handled(tstate, frame, next_instr, exc) < 0) {
+                goto exception_unwind;
+            }
             /* Resume normal execution */
             DISPATCH();
         }
@@ -1924,6 +1926,7 @@ do_monitor_exc(PyThreadState *tstate, _PyInterpreterFrame *frame,
         PyErr_SetRaisedException(exc);
     }
     else {
+        assert(PyErr_Occurred());
         Py_DECREF(exc);
     }
     return err;
@@ -1988,15 +1991,15 @@ monitor_unwind(PyThreadState *tstate,
 }
 
 
-static void
+static int
 monitor_handled(PyThreadState *tstate,
                 _PyInterpreterFrame *frame,
                 _Py_CODEUNIT *instr, PyObject *exc)
 {
     if (no_tools_for_event(tstate, frame, PY_MONITORING_EVENT_EXCEPTION_HANDLED)) {
-        return;
+        return 0;
     }
-    _Py_call_instrumentation_arg(tstate, PY_MONITORING_EVENT_EXCEPTION_HANDLED, frame, instr, exc);
+    return _Py_call_instrumentation_arg(tstate, PY_MONITORING_EVENT_EXCEPTION_HANDLED, frame, instr, exc);
 }
 
 static void

Python/executor_cases.c.h

@@ -702,29 +702,13 @@ instrument_per_instruction(PyCodeObject *code, int i)
     *opcode_ptr = INSTRUMENTED_INSTRUCTION;
 }
 
-#ifndef NDEBUG
-static bool
-instruction_has_event(PyCodeObject *code, int offset)
-{
-    _Py_CODEUNIT instr = _PyCode_CODE(code)[offset];
-    int opcode = instr.op.code;
-    if (opcode == INSTRUMENTED_LINE) {
-        opcode = code->_co_monitoring->lines[offset].original_opcode;
-    }
-    if (opcode == INSTRUMENTED_INSTRUCTION) {
-        opcode = code->_co_monitoring->per_instruction_opcodes[offset];
-    }
-    return opcode_has_event(opcode);
-}
-#endif
-
 static void
 remove_tools(PyCodeObject * code, int offset, int event, int tools)
 {
     assert(event != PY_MONITORING_EVENT_LINE);
     assert(event != PY_MONITORING_EVENT_INSTRUCTION);
-    assert(event < PY_MONITORING_INSTRUMENTED_EVENTS);
-    assert(instruction_has_event(code, offset));
+    assert(PY_MONITORING_IS_INSTRUMENTED_EVENT(event));
+    assert(opcode_has_event(_Py_GetBaseOpcode(code, offset)));
     _PyCoMonitoringData *monitoring = code->_co_monitoring;
     if (monitoring && monitoring->tools) {
         monitoring->tools[offset] &= ~tools;
@@ -779,7 +763,7 @@ add_tools(PyCodeObject * code, int offset, int event, int tools)
 {
     assert(event != PY_MONITORING_EVENT_LINE);
     assert(event != PY_MONITORING_EVENT_INSTRUCTION);
-    assert(event < PY_MONITORING_INSTRUMENTED_EVENTS);
+    assert(PY_MONITORING_IS_INSTRUMENTED_EVENT(event));
     assert(code->_co_monitoring);
     if (code->_co_monitoring &&
         code->_co_monitoring->tools
@@ -919,7 +903,7 @@ get_tools_for_instruction(PyCodeObject *code, PyInterpreterState *interp, int i,
                 event == PY_MONITORING_EVENT_C_RETURN);
         event = PY_MONITORING_EVENT_CALL;
     }
-    if (event < PY_MONITORING_INSTRUMENTED_EVENTS) {
+    if (PY_MONITORING_IS_INSTRUMENTED_EVENT(event)) {
         CHECK(is_version_up_to_date(code, interp));
         CHECK(instrumentation_cross_checks(interp, code));
         if (code->_co_monitoring->tools) {
@@ -940,6 +924,26 @@ get_tools_for_instruction(PyCodeObject *code, PyInterpreterState *interp, int i,
     return tools;
 }
 
+static const char *const event_names [] = {
+    [PY_MONITORING_EVENT_PY_START] = "PY_START",
+    [PY_MONITORING_EVENT_PY_RESUME] = "PY_RESUME",
+    [PY_MONITORING_EVENT_PY_RETURN] = "PY_RETURN",
+    [PY_MONITORING_EVENT_PY_YIELD] = "PY_YIELD",
+    [PY_MONITORING_EVENT_CALL] = "CALL",
+    [PY_MONITORING_EVENT_LINE] = "LINE",
+    [PY_MONITORING_EVENT_INSTRUCTION] = "INSTRUCTION",
+    [PY_MONITORING_EVENT_JUMP] = "JUMP",
+    [PY_MONITORING_EVENT_BRANCH] = "BRANCH",
+    [PY_MONITORING_EVENT_C_RETURN] = "C_RETURN",
+    [PY_MONITORING_EVENT_PY_THROW] = "PY_THROW",
+    [PY_MONITORING_EVENT_RAISE] = "RAISE",
+    [PY_MONITORING_EVENT_RERAISE] = "RERAISE",
+    [PY_MONITORING_EVENT_EXCEPTION_HANDLED] = "EXCEPTION_HANDLED",
+    [PY_MONITORING_EVENT_C_RAISE] = "C_RAISE",
+    [PY_MONITORING_EVENT_PY_UNWIND] = "PY_UNWIND",
+    [PY_MONITORING_EVENT_STOP_ITERATION] = "STOP_ITERATION",
+};
+
 static int
 call_instrumentation_vector(
     PyThreadState *tstate, int event,
@@ -984,7 +988,18 @@ call_instrumentation_vector(
         }
         else {
             /* DISABLE */
-            remove_tools(code, offset, event, 1 << tool);
+            if (!PY_MONITORING_IS_INSTRUMENTED_EVENT(event)) {
+                PyErr_Format(PyExc_ValueError,
+                              "Cannot disable %s events. Callback removed.",
+                             event_names[event]);
+                /* Clear tool to prevent infinite loop */
+                Py_CLEAR(interp->monitoring_callables[tool][event]);
+                err = -1;
+                break;
+            }
+            else {
+                remove_tools(code, offset, event, 1 << tool);
+            }
         }
     }
     Py_DECREF(offset_obj);
@@ -1262,7 +1277,7 @@ initialize_tools(PyCodeObject *code)
                     assert(event > 0);
                 }
                 assert(event >= 0);
-                assert(event < PY_MONITORING_INSTRUMENTED_EVENTS);
+                assert(PY_MONITORING_IS_INSTRUMENTED_EVENT(event));
                 tools[i] = code->_co_monitoring->active_monitors.tools[event];
                 CHECK(tools[i] != 0);
             }
@@ -2017,26 +2032,6 @@ add_power2_constant(PyObject *obj, const char *name, int i)
     return err;
 }
 
-static const char *const event_names [] = {
-    [PY_MONITORING_EVENT_PY_START] = "PY_START",
-    [PY_MONITORING_EVENT_PY_RESUME] = "PY_RESUME",
-    [PY_MONITORING_EVENT_PY_RETURN] = "PY_RETURN",
-    [PY_MONITORING_EVENT_PY_YIELD] = "PY_YIELD",
-    [PY_MONITORING_EVENT_CALL] = "CALL",
-    [PY_MONITORING_EVENT_LINE] = "LINE",
-    [PY_MONITORING_EVENT_INSTRUCTION] = "INSTRUCTION",
-    [PY_MONITORING_EVENT_JUMP] = "JUMP",
-    [PY_MONITORING_EVENT_BRANCH] = "BRANCH",
-    [PY_MONITORING_EVENT_C_RETURN] = "C_RETURN",
-    [PY_MONITORING_EVENT_PY_THROW] = "PY_THROW",
-    [PY_MONITORING_EVENT_RAISE] = "RAISE",
-    [PY_MONITORING_EVENT_RERAISE] = "RERAISE",
-    [PY_MONITORING_EVENT_EXCEPTION_HANDLED] = "EXCEPTION_HANDLED",
-    [PY_MONITORING_EVENT_C_RAISE] = "C_RAISE",
-    [PY_MONITORING_EVENT_PY_UNWIND] = "PY_UNWIND",
-    [PY_MONITORING_EVENT_STOP_ITERATION] = "STOP_ITERATION",
-};
-
 /*[clinic input]
 monitoring._all_events
 [clinic start generated code]*/