From 5c53261e1a78d6b203bb5bff8778f714fbb3776c Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Fri, 11 Nov 2022 14:32:49 +0000 Subject: [PATCH 1/6] gh-99377: Add audit events for thread creation and clear --- Doc/c-api/init.rst | 12 ++++++ Doc/library/_thread.rst | 2 + Lib/test/audit-tests.py | 42 +++++++++++++++++++ Lib/test/test_audit.py | 25 +++++++++++ ...2-11-11-14-04-01.gh-issue-99377.-CJvWn.rst | 1 + Modules/_threadmodule.c | 10 ++++- Python/pystate.c | 22 +++++++--- 7 files changed, 108 insertions(+), 6 deletions(-) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2022-11-11-14-04-01.gh-issue-99377.-CJvWn.rst diff --git a/Doc/c-api/init.rst b/Doc/c-api/init.rst index afb17719a77ab2..4e07872914a387 100644 --- a/Doc/c-api/init.rst +++ b/Doc/c-api/init.rst @@ -1239,12 +1239,24 @@ All of the following functions must be called after :c:func:`Py_Initialize`. The global interpreter lock need not be held, but may be held if it is necessary to serialize calls to this function. + .. audit-event:: cpython.PyThreadState_New "" c.PyThreadState_New + + Raise an auditing event ``cpython.PyThreadState_New`` with no arguments. + The event will be raised from the creating thread, not the new thread. + .. c:function:: void PyThreadState_Clear(PyThreadState *tstate) Reset all information in a thread state object. The global interpreter lock must be held. + .. audit-event:: cpython.PyThreadState_Clear "" c.PyThreadState_Clear + + Raise an auditing event ``cpython.PyThreadState_Clear`` with no arguments. + The event may be raised from a different thread than the one being + cleared. Exceptions raised from a hook will be treated as unraisable and + will not abort the operation. + .. versionchanged:: 3.9 This function now calls the :c:member:`PyThreadState.on_delete` callback. Previously, that happened in :c:func:`PyThreadState_Delete`. diff --git a/Doc/library/_thread.rst b/Doc/library/_thread.rst index 9df9e7914e093b..122692a428594f 100644 --- a/Doc/library/_thread.rst +++ b/Doc/library/_thread.rst @@ -57,6 +57,8 @@ This module defines the following constants and functions: When the function raises a :exc:`SystemExit` exception, it is silently ignored. + .. audit-event:: _thread.start_new_thread function,args,kwargs start_new_thread + .. versionchanged:: 3.8 :func:`sys.unraisablehook` is now used to handle unhandled exceptions. diff --git a/Lib/test/audit-tests.py b/Lib/test/audit-tests.py index 4abf33d7cad1b6..f4449c7c3a1e99 100644 --- a/Lib/test/audit-tests.py +++ b/Lib/test/audit-tests.py @@ -419,6 +419,48 @@ def hook(event, args): sys._getframe() +def test_threading(): + import _thread + + def hook(event, args): + if event.startswith(("_thread.", "cpython.PyThreadState", "test.")): + print(event, args) + + sys.addaudithook(hook) + + lock = _thread.allocate_lock() + lock.acquire() + + class test_func: + def __repr__(self): return "" + def __call__(self): + sys.audit("test.test_func") + lock.release() + + i = _thread.start_new_thread(test_func(), ()) + lock.acquire() + + +def test_threading_abort(): + # Ensures that aborting PyThreadState_New raises the correct exception + import _thread + + class ThreadNewAbortError(Exception): + pass + + def hook(event, args): + if event == "cpython.PyThreadState_New": + raise ThreadNewAbortError() + + sys.addaudithook(hook) + + try: + _thread.start_new_thread(lambda: None, ()) + except ThreadNewAbortError: + # Other exceptions are raised and the test will fail + pass + + def test_wmi_exec_query(): import _wmi diff --git a/Lib/test/test_audit.py b/Lib/test/test_audit.py index 50f78f2a6b8a46..f217719548d1e4 100644 --- a/Lib/test/test_audit.py +++ b/Lib/test/test_audit.py @@ -186,6 +186,31 @@ def test_sys_getframe(self): self.assertEqual(actual, expected) + + def test_threading(self): + returncode, events, stderr = self.run_python("test_threading") + if returncode: + self.fail(stderr) + + if support.verbose: + print(*events, sep='\n') + actual = [(ev[0], ev[2]) for ev in events] + expected = [ + ("_thread.start_new_thread", "(, (), None)"), + ("cpython.PyThreadState_New", "()"), + ("test.test_func", "()"), + ("cpython.PyThreadState_Clear", "()"), + ] + + self.assertEqual(actual, expected) + + def test_threading_abort(self): + # Ensures that aborting PyThreadState_New raises the correct exception + returncode, events, stderr = self.run_python("test_threading_abort") + if returncode: + self.fail(stderr) + + def test_wmi_exec_query(self): import_helper.import_module("_wmi") returncode, events, stderr = self.run_python("test_wmi_exec_query") diff --git a/Misc/NEWS.d/next/Core and Builtins/2022-11-11-14-04-01.gh-issue-99377.-CJvWn.rst b/Misc/NEWS.d/next/Core and Builtins/2022-11-11-14-04-01.gh-issue-99377.-CJvWn.rst new file mode 100644 index 00000000000000..631b9ca23044a7 --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2022-11-11-14-04-01.gh-issue-99377.-CJvWn.rst @@ -0,0 +1 @@ +Add audit events for thread creation and clear operations. diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 5968d4e2e0ee15..ec8b6d881124da 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -1145,6 +1145,11 @@ thread_PyThread_start_new_thread(PyObject *self, PyObject *fargs) return NULL; } + if (PySys_Audit("_thread.start_new_thread", "OOO", + func, args, kwargs ? kwargs : Py_None) < 0) { + return NULL; + } + PyInterpreterState *interp = _PyInterpreterState_GET(); if (!_PyInterpreterState_HasFeature(interp, Py_RTFLAGS_THREADS)) { PyErr_SetString(PyExc_RuntimeError, @@ -1160,7 +1165,10 @@ thread_PyThread_start_new_thread(PyObject *self, PyObject *fargs) boot->tstate = _PyThreadState_Prealloc(boot->interp); if (boot->tstate == NULL) { PyMem_Free(boot); - return PyErr_NoMemory(); + if (!PyErr_Occurred()) { + return PyErr_NoMemory(); + } + return NULL; } boot->runtime = runtime; boot->func = Py_NewRef(func); diff --git a/Python/pystate.c b/Python/pystate.c index 04db1fb419af62..198184affe0591 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -820,6 +820,11 @@ new_threadstate(PyInterpreterState *interp) { PyThreadState *tstate; _PyRuntimeState *runtime = interp->runtime; + + if (PySys_Audit("cpython.PyThreadState_New", NULL) < 0) { + return NULL; + } + // We don't need to allocate a thread state for the main interpreter // (the common case), but doing it later for the other case revealed a // reentrancy problem (deadlock). So for now we always allocate before @@ -867,6 +872,7 @@ new_threadstate(PyInterpreterState *interp) // Must be called with lock unlocked to avoid re-entrancy deadlock. PyMem_RawFree(new_tstate); } + return tstate; } @@ -874,7 +880,9 @@ PyThreadState * PyThreadState_New(PyInterpreterState *interp) { PyThreadState *tstate = new_threadstate(interp); - _PyThreadState_SetCurrent(tstate); + if (tstate) { + _PyThreadState_SetCurrent(tstate); + } return tstate; } @@ -1029,6 +1037,10 @@ _PyInterpreterState_ClearModules(PyInterpreterState *interp) void PyThreadState_Clear(PyThreadState *tstate) { + if (PySys_Audit("cpython.PyThreadState_Clear", NULL) < 0) { + PyErr_WriteUnraisable(NULL); + } + int verbose = _PyInterpreterState_GetConfig(tstate->interp)->verbose; if (verbose && tstate->cframe->current_frame != NULL) { @@ -1546,16 +1558,16 @@ _PyGILState_Init(_PyRuntimeState *runtime) PyStatus _PyGILState_SetTstate(PyThreadState *tstate) { + /* must init with valid states */ + assert(tstate != NULL); + assert(tstate->interp != NULL); + if (!_Py_IsMainInterpreter(tstate->interp)) { /* Currently, PyGILState is shared by all interpreters. The main * interpreter is responsible to initialize it. */ return _PyStatus_OK(); } - /* must init with valid states */ - assert(tstate != NULL); - assert(tstate->interp != NULL); - struct _gilstate_runtime_state *gilstate = &tstate->interp->runtime->gilstate; gilstate->autoInterpreterState = tstate->interp; From 09f0fdf0d6882573961a3de6d3ccf27924182e8f Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Fri, 11 Nov 2022 16:06:50 +0000 Subject: [PATCH 2/6] Only call Python hooks when GIL is held --- Python/pystate.c | 3 ++- Python/sysmodule.c | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/Python/pystate.c b/Python/pystate.c index 198184affe0591..6b4f7544d4c9b1 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -821,7 +821,8 @@ new_threadstate(PyInterpreterState *interp) PyThreadState *tstate; _PyRuntimeState *runtime = interp->runtime; - if (PySys_Audit("cpython.PyThreadState_New", NULL) < 0) { + PyThreadState *ts = _PyThreadState_GET(); + if (ts && _PySys_Audit(ts, "cpython.PyThreadState_New", NULL) < 0) { return NULL; } diff --git a/Python/sysmodule.c b/Python/sysmodule.c index 5de684e7195bff..1f8c30d12a5313 100644 --- a/Python/sysmodule.c +++ b/Python/sysmodule.c @@ -222,8 +222,8 @@ sys_audit_tstate(PyThreadState *ts, const char *event, PyDTrace_AUDIT(event, (void *)eventArgs); } - /* Call interpreter hooks */ - if (is->audit_hooks) { + /* Call interpreter hooks if the GIL is held */ + if (PyGILState_Check() && is->audit_hooks) { eventName = PyUnicode_FromString(event); if (!eventName) { goto exit; From 9d69a0e774708908af03adb3aa65e99c9fd1047d Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Tue, 15 Nov 2022 00:33:32 +0000 Subject: [PATCH 3/6] Tries raising the event from a different place, and adds id --- Doc/c-api/init.rst | 17 +++++++++-------- Python/pystate.c | 22 +++++++++++++++------- 2 files changed, 24 insertions(+), 15 deletions(-) diff --git a/Doc/c-api/init.rst b/Doc/c-api/init.rst index 4e07872914a387..273838c1db28ea 100644 --- a/Doc/c-api/init.rst +++ b/Doc/c-api/init.rst @@ -1239,10 +1239,11 @@ All of the following functions must be called after :c:func:`Py_Initialize`. The global interpreter lock need not be held, but may be held if it is necessary to serialize calls to this function. - .. audit-event:: cpython.PyThreadState_New "" c.PyThreadState_New + .. audit-event:: cpython.PyThreadState_New id c.PyThreadState_New - Raise an auditing event ``cpython.PyThreadState_New`` with no arguments. - The event will be raised from the creating thread, not the new thread. + Raise an auditing event ``cpython.PyThreadState_New`` with Python's thread + id as the argument. The event will be raised from the thread creating the new + ``PyThreadState``, which may not be the new thread. .. c:function:: void PyThreadState_Clear(PyThreadState *tstate) @@ -1250,12 +1251,12 @@ All of the following functions must be called after :c:func:`Py_Initialize`. Reset all information in a thread state object. The global interpreter lock must be held. - .. audit-event:: cpython.PyThreadState_Clear "" c.PyThreadState_Clear + .. audit-event:: cpython.PyThreadState_Clear id c.PyThreadState_Clear - Raise an auditing event ``cpython.PyThreadState_Clear`` with no arguments. - The event may be raised from a different thread than the one being - cleared. Exceptions raised from a hook will be treated as unraisable and - will not abort the operation. + Raise an auditing event ``cpython.PyThreadState_Clear`` with Python's + thread id as the argument. The event may be raised from a different thread + than the one being cleared. Exceptions raised from a hook will be treated + as unraisable and will not abort the operation. .. versionchanged:: 3.9 This function now calls the :c:member:`PyThreadState.on_delete` callback. diff --git a/Python/pystate.c b/Python/pystate.c index 6b4f7544d4c9b1..8dd595c567d4f4 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -821,11 +821,6 @@ new_threadstate(PyInterpreterState *interp) PyThreadState *tstate; _PyRuntimeState *runtime = interp->runtime; - PyThreadState *ts = _PyThreadState_GET(); - if (ts && _PySys_Audit(ts, "cpython.PyThreadState_New", NULL) < 0) { - return NULL; - } - // We don't need to allocate a thread state for the main interpreter // (the common case), but doing it later for the other case revealed a // reentrancy problem (deadlock). So for now we always allocate before @@ -883,6 +878,11 @@ PyThreadState_New(PyInterpreterState *interp) PyThreadState *tstate = new_threadstate(interp); if (tstate) { _PyThreadState_SetCurrent(tstate); + if (PySys_Audit("cpython.PyThreadState_New", "K", tstate->id) < 0) { + PyThreadState_Clear(tstate); + _PyThreadState_DeleteCurrent(tstate); + return NULL; + } } return tstate; } @@ -890,7 +890,15 @@ PyThreadState_New(PyInterpreterState *interp) PyThreadState * _PyThreadState_Prealloc(PyInterpreterState *interp) { - return new_threadstate(interp); + PyThreadState *tstate = new_threadstate(interp); + if (tstate) { + if (PySys_Audit("cpython.PyThreadState_New", "K", tstate->id) < 0) { + PyThreadState_Clear(tstate); + _PyThreadState_DeleteCurrent(tstate); + return NULL; + } + } + return tstate; } // We keep this around for (accidental) stable ABI compatibility. @@ -1038,7 +1046,7 @@ _PyInterpreterState_ClearModules(PyInterpreterState *interp) void PyThreadState_Clear(PyThreadState *tstate) { - if (PySys_Audit("cpython.PyThreadState_Clear", NULL) < 0) { + if (PySys_Audit("cpython.PyThreadState_Clear", "K", tstate->id) < 0) { PyErr_WriteUnraisable(NULL); } From 76ef62196f774c730d399a753405f2f645f7c8c5 Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Tue, 15 Nov 2022 00:52:44 +0000 Subject: [PATCH 4/6] Fix tests --- Lib/test/test_audit.py | 4 ++-- Python/pystate.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_audit.py b/Lib/test/test_audit.py index f217719548d1e4..d6a481daa7a569 100644 --- a/Lib/test/test_audit.py +++ b/Lib/test/test_audit.py @@ -197,9 +197,9 @@ def test_threading(self): actual = [(ev[0], ev[2]) for ev in events] expected = [ ("_thread.start_new_thread", "(, (), None)"), - ("cpython.PyThreadState_New", "()"), + ("cpython.PyThreadState_New", "(2,)"), ("test.test_func", "()"), - ("cpython.PyThreadState_Clear", "()"), + ("cpython.PyThreadState_Clear", "(2,)"), ] self.assertEqual(actual, expected) diff --git a/Python/pystate.c b/Python/pystate.c index 8dd595c567d4f4..94dbd77d9b9fd4 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -894,7 +894,7 @@ _PyThreadState_Prealloc(PyInterpreterState *interp) if (tstate) { if (PySys_Audit("cpython.PyThreadState_New", "K", tstate->id) < 0) { PyThreadState_Clear(tstate); - _PyThreadState_DeleteCurrent(tstate); + _PyThreadState_Delete(tstate, 0); return NULL; } } From 1ff0cdef84fcfb9debf79d176524c7910766bcbe Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Tue, 15 Nov 2022 21:43:28 +0000 Subject: [PATCH 5/6] Revert whitespace changes --- Python/pystate.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/Python/pystate.c b/Python/pystate.c index 94dbd77d9b9fd4..4e7fa9ad8b5f13 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -820,7 +820,6 @@ new_threadstate(PyInterpreterState *interp) { PyThreadState *tstate; _PyRuntimeState *runtime = interp->runtime; - // We don't need to allocate a thread state for the main interpreter // (the common case), but doing it later for the other case revealed a // reentrancy problem (deadlock). So for now we always allocate before @@ -868,7 +867,6 @@ new_threadstate(PyInterpreterState *interp) // Must be called with lock unlocked to avoid re-entrancy deadlock. PyMem_RawFree(new_tstate); } - return tstate; } From e67161cc35c476e37490a16049759f3111709169 Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Wed, 16 Nov 2022 00:19:42 +0000 Subject: [PATCH 6/6] Revert GIL check --- Python/sysmodule.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Python/sysmodule.c b/Python/sysmodule.c index 1f8c30d12a5313..5de684e7195bff 100644 --- a/Python/sysmodule.c +++ b/Python/sysmodule.c @@ -222,8 +222,8 @@ sys_audit_tstate(PyThreadState *ts, const char *event, PyDTrace_AUDIT(event, (void *)eventArgs); } - /* Call interpreter hooks if the GIL is held */ - if (PyGILState_Check() && is->audit_hooks) { + /* Call interpreter hooks */ + if (is->audit_hooks) { eventName = PyUnicode_FromString(event); if (!eventName) { goto exit;