gh-120289: Disallow disable() and clear() in external timer to prevent use-after-free (#120297)

gaogaotiantian · web-flow · commit 1ab17782832b · 2024-07-18T12:47:22.000-07:00
diff --git a/Lib/test/test_cprofile.py b/Lib/test/test_cprofile.py
@@ -30,6 +30,43 @@ def test_bad_counter_during_dealloc(self):
 
             self.assertEqual(cm.unraisable.exc_type, TypeError)
 
+    def test_evil_external_timer(self):
+        # gh-120289
+        # Disabling profiler in external timer should not crash
+        import _lsprof
+        class EvilTimer():
+            def __init__(self, disable_count):
+                self.count = 0
+                self.disable_count = disable_count
+
+            def __call__(self):
+                self.count += 1
+                if self.count == self.disable_count:
+                    profiler_with_evil_timer.disable()
+                return self.count
+
+        # this will trigger external timer to disable profiler at
+        # call event - in initContext in _lsprof.c
+        with support.catch_unraisable_exception() as cm:
+            profiler_with_evil_timer = _lsprof.Profiler(EvilTimer(1))
+            profiler_with_evil_timer.enable()
+            # Make a call to trigger timer
+            (lambda: None)()
+            profiler_with_evil_timer.disable()
+            profiler_with_evil_timer.clear()
+            self.assertEqual(cm.unraisable.exc_type, RuntimeError)
+
+        # this will trigger external timer to disable profiler at
+        # return event - in Stop in _lsprof.c
+        with support.catch_unraisable_exception() as cm:
+            profiler_with_evil_timer = _lsprof.Profiler(EvilTimer(2))
+            profiler_with_evil_timer.enable()
+            # Make a call to trigger timer
+            (lambda: None)()
+            profiler_with_evil_timer.disable()
+            profiler_with_evil_timer.clear()
+            self.assertEqual(cm.unraisable.exc_type, RuntimeError)
+
     def test_profile_enable_disable(self):
         prof = self.profilerclass()
         # Make sure we clean ourselves up if the test fails for some reason.
diff --git a/Misc/NEWS.d/next/Library/2024-06-09-19-53-11.gh-issue-120289.s4HXR0.rst b/Misc/NEWS.d/next/Library/2024-06-09-19-53-11.gh-issue-120289.s4HXR0.rst
@@ -0,0 +1,2 @@
+Fixed the use-after-free issue in :mod:`cProfile` by disallowing
+``disable()`` and ``clear()`` in external timers.
diff --git a/Modules/_lsprof.c b/Modules/_lsprof.c
@@ -59,6 +59,7 @@ typedef struct {
 #define POF_ENABLED     0x001
 #define POF_SUBCALLS    0x002
 #define POF_BUILTINS    0x004
+#define POF_EXT_TIMER   0x008
 #define POF_NOMEMORY    0x100
 
 /*[clinic input]
@@ -87,7 +88,14 @@ _lsprof_get_state(PyObject *module)
 
 static PyTime_t CallExternalTimer(ProfilerObject *pObj)
 {
-    PyObject *o = _PyObject_CallNoArgs(pObj->externalTimer);
+    PyObject *o = NULL;
+
+    // External timer can do arbitrary things so we need a flag to prevent
+    // horrible things to happen
+    pObj->flags |= POF_EXT_TIMER;
+    o = _PyObject_CallNoArgs(pObj->externalTimer);
+    pObj->flags &= ~POF_EXT_TIMER;
+
     if (o == NULL) {
         PyErr_WriteUnraisable(pObj->externalTimer);
         return 0;
@@ -777,6 +785,11 @@ Stop collecting profiling information.\n\
 static PyObject*
 profiler_disable(ProfilerObject *self, PyObject* noarg)
 {
+    if (self->flags & POF_EXT_TIMER) {
+        PyErr_SetString(PyExc_RuntimeError,
+                        "cannot disable profiler in external timer");
+        return NULL;
+    }
     if (self->flags & POF_ENABLED) {
         PyObject* result = NULL;
         PyObject* monitoring = _PyImport_GetModuleAttrString("sys", "monitoring");
@@ -830,6 +843,11 @@ Clear all profiling information collected so far.\n\
 static PyObject*
 profiler_clear(ProfilerObject *pObj, PyObject* noarg)
 {
+    if (pObj->flags & POF_EXT_TIMER) {
+        PyErr_SetString(PyExc_RuntimeError,
+                        "cannot clear profiler in external timer");
+        return NULL;
+    }
     clearEntries(pObj);
     Py_RETURN_NONE;
 }

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,2 @@`
	`1`	+Fixed the use-after-free issue in :mod:`cProfile` by disallowing
	`2`	+``disable()`` and ``clear()`` in external timers.