From f868923d087825fb2092432160cdcb03b2c30073 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 6 Jun 2023 18:12:18 -0600 Subject: [PATCH 1/4] Factor out LOCKS_INIT(). --- Python/pystate.c | 32 +++++++++++--------------------- 1 file changed, 11 insertions(+), 21 deletions(-) diff --git a/Python/pystate.c b/Python/pystate.c index 5b7a6c86ade4d7..0a316a82fcb916 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -381,6 +381,14 @@ static const _PyRuntimeState initial = _PyRuntimeState_INIT(_PyRuntime); _Py_COMP_DIAG_POP #define NUMLOCKS 5 +#define LOCKS_INIT(runtime) \ + { \ + &(runtime)->interpreters.mutex, \ + &(runtime)->xidregistry.mutex, \ + &(runtime)->getargs.mutex, \ + &(runtime)->unicode_state.ids.lock, \ + &(runtime)->imports.extensions.mutex, \ + } static int alloc_for_runtime(PyThread_type_lock locks[NUMLOCKS]) @@ -427,13 +435,7 @@ init_runtime(_PyRuntimeState *runtime, PyPreConfig_InitPythonConfig(&runtime->preconfig); - PyThread_type_lock *lockptrs[NUMLOCKS] = { - &runtime->interpreters.mutex, - &runtime->xidregistry.mutex, - &runtime->getargs.mutex, - &runtime->unicode_state.ids.lock, - &runtime->imports.extensions.mutex, - }; + PyThread_type_lock *lockptrs[NUMLOCKS] = LOCKS_INIT(runtime); for (int i = 0; i < NUMLOCKS; i++) { assert(locks[i] != NULL); *lockptrs[i] = locks[i]; @@ -512,13 +514,7 @@ _PyRuntimeState_Fini(_PyRuntimeState *runtime) LOCK = NULL; \ } - PyThread_type_lock *lockptrs[NUMLOCKS] = { - &runtime->interpreters.mutex, - &runtime->xidregistry.mutex, - &runtime->getargs.mutex, - &runtime->unicode_state.ids.lock, - &runtime->imports.extensions.mutex, - }; + PyThread_type_lock *lockptrs[NUMLOCKS] = LOCKS_INIT(runtime); for (int i = 0; i < NUMLOCKS; i++) { FREE_LOCK(*lockptrs[i]); } @@ -541,13 +537,7 @@ _PyRuntimeState_ReInitThreads(_PyRuntimeState *runtime) PyMemAllocatorEx old_alloc; _PyMem_SetDefaultAllocator(PYMEM_DOMAIN_RAW, &old_alloc); - PyThread_type_lock *lockptrs[NUMLOCKS] = { - &runtime->interpreters.mutex, - &runtime->xidregistry.mutex, - &runtime->getargs.mutex, - &runtime->unicode_state.ids.lock, - &runtime->imports.extensions.mutex, - }; + PyThread_type_lock *lockptrs[NUMLOCKS] = LOCKS_INIT(runtime); int reinit_err = 0; for (int i = 0; i < NUMLOCKS; i++) { reinit_err += _PyThread_at_fork_reinit(lockptrs[i]); From 3f0dcb47a867839bd73af99a296f6a99dc7b3284 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 6 Jun 2023 18:17:51 -0600 Subject: [PATCH 2/4] Factor out _PyRuntimeState.audit_hooks. --- Include/internal/pycore_runtime.h | 4 +++- Python/pystate.c | 4 ++-- Python/sysmodule.c | 12 ++++++------ 3 files changed, 11 insertions(+), 9 deletions(-) diff --git a/Include/internal/pycore_runtime.h b/Include/internal/pycore_runtime.h index 6e8f16a611d75e..222cb9a498be22 100644 --- a/Include/internal/pycore_runtime.h +++ b/Include/internal/pycore_runtime.h @@ -147,7 +147,9 @@ typedef struct pyruntimestate { // is called multiple times. Py_OpenCodeHookFunction open_code_hook; void *open_code_userdata; - _Py_AuditHookEntry *audit_hook_head; + struct { + _Py_AuditHookEntry *head; + } audit_hooks; struct _py_object_runtime_state object_state; struct _Py_float_runtime_state float_state; diff --git a/Python/pystate.c b/Python/pystate.c index 0a316a82fcb916..a3ac002b138eb8 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -431,7 +431,7 @@ init_runtime(_PyRuntimeState *runtime, runtime->open_code_hook = open_code_hook; runtime->open_code_userdata = open_code_userdata; - runtime->audit_hook_head = audit_hook_head; + runtime->audit_hooks.head = audit_hook_head; PyPreConfig_InitPythonConfig(&runtime->preconfig); @@ -457,7 +457,7 @@ _PyRuntimeState_Init(_PyRuntimeState *runtime) initialization and interpreter initialization. */ void *open_code_hook = runtime->open_code_hook; void *open_code_userdata = runtime->open_code_userdata; - _Py_AuditHookEntry *audit_hook_head = runtime->audit_hook_head; + _Py_AuditHookEntry *audit_hook_head = runtime->audit_hooks.head; // bpo-42882: Preserve next_index value if Py_Initialize()/Py_Finalize() // is called multiple times. Py_ssize_t unicode_next_index = runtime->unicode_state.ids.next_index; diff --git a/Python/sysmodule.c b/Python/sysmodule.c index 4427e73e584e30..6560835cab09c2 100644 --- a/Python/sysmodule.c +++ b/Python/sysmodule.c @@ -168,7 +168,7 @@ should_audit(PyInterpreterState *interp) if (!interp) { return 0; } - return (interp->runtime->audit_hook_head + return (interp->runtime->audit_hooks.head || interp->audit_hooks || PyDTrace_AUDIT_ENABLED()); } @@ -225,7 +225,7 @@ sys_audit_tstate(PyThreadState *ts, const char *event, } /* Call global hooks */ - _Py_AuditHookEntry *e = is->runtime->audit_hook_head; + _Py_AuditHookEntry *e = is->runtime->audit_hooks.head; for (; e; e = e->next) { if (e->hookCFunction(event, eventArgs, e->userData) < 0) { goto exit; @@ -353,8 +353,8 @@ _PySys_ClearAuditHooks(PyThreadState *ts) _PySys_Audit(ts, "cpython._PySys_ClearAuditHooks", NULL); _PyErr_Clear(ts); - _Py_AuditHookEntry *e = runtime->audit_hook_head, *n; - runtime->audit_hook_head = NULL; + _Py_AuditHookEntry *e = runtime->audit_hooks.head, *n; + runtime->audit_hooks.head = NULL; while (e) { n = e->next; PyMem_RawFree(e); @@ -389,10 +389,10 @@ PySys_AddAuditHook(Py_AuditHookFunction hook, void *userData) } } - _Py_AuditHookEntry *e = runtime->audit_hook_head; + _Py_AuditHookEntry *e = runtime->audit_hooks.head; if (!e) { e = (_Py_AuditHookEntry*)PyMem_RawMalloc(sizeof(_Py_AuditHookEntry)); - runtime->audit_hook_head = e; + runtime->audit_hooks.head = e; } else { while (e->next) { e = e->next; From f01297a477b2c4811733d02bff02b85087e02b1e Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Tue, 6 Jun 2023 18:29:50 -0600 Subject: [PATCH 3/4] Add a granular lock for global audit hooks state. --- Include/internal/pycore_runtime.h | 1 + Python/pystate.c | 3 ++- Python/sysmodule.c | 11 ++++++++++- 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/Include/internal/pycore_runtime.h b/Include/internal/pycore_runtime.h index 222cb9a498be22..ab03c07587fe72 100644 --- a/Include/internal/pycore_runtime.h +++ b/Include/internal/pycore_runtime.h @@ -148,6 +148,7 @@ typedef struct pyruntimestate { Py_OpenCodeHookFunction open_code_hook; void *open_code_userdata; struct { + PyThread_type_lock mutex; _Py_AuditHookEntry *head; } audit_hooks; diff --git a/Python/pystate.c b/Python/pystate.c index a3ac002b138eb8..0b9cef7de35b2f 100644 --- a/Python/pystate.c +++ b/Python/pystate.c @@ -380,7 +380,7 @@ _Py_COMP_DIAG_IGNORE_DEPR_DECLS static const _PyRuntimeState initial = _PyRuntimeState_INIT(_PyRuntime); _Py_COMP_DIAG_POP -#define NUMLOCKS 5 +#define NUMLOCKS 6 #define LOCKS_INIT(runtime) \ { \ &(runtime)->interpreters.mutex, \ @@ -388,6 +388,7 @@ _Py_COMP_DIAG_POP &(runtime)->getargs.mutex, \ &(runtime)->unicode_state.ids.lock, \ &(runtime)->imports.extensions.mutex, \ + &(runtime)->audit_hooks.mutex, \ } static int diff --git a/Python/sysmodule.c b/Python/sysmodule.c index 6560835cab09c2..60aee8f7f4ad1c 100644 --- a/Python/sysmodule.c +++ b/Python/sysmodule.c @@ -168,7 +168,10 @@ should_audit(PyInterpreterState *interp) if (!interp) { return 0; } - return (interp->runtime->audit_hooks.head + PyThread_acquire_lock(interp->runtime->audit_hooks.mutex, WAIT_LOCK); + int has_global_hooks = (interp->runtime->audit_hooks.head != NULL); + PyThread_release_lock(interp->runtime->audit_hooks.mutex); + return (has_global_hooks || interp->audit_hooks || PyDTrace_AUDIT_ENABLED()); } @@ -225,7 +228,9 @@ sys_audit_tstate(PyThreadState *ts, const char *event, } /* Call global hooks */ + PyThread_acquire_lock(is->runtime->audit_hooks.mutex, WAIT_LOCK); _Py_AuditHookEntry *e = is->runtime->audit_hooks.head; + PyThread_release_lock(is->runtime->audit_hooks.mutex); for (; e; e = e->next) { if (e->hookCFunction(event, eventArgs, e->userData) < 0) { goto exit; @@ -353,8 +358,10 @@ _PySys_ClearAuditHooks(PyThreadState *ts) _PySys_Audit(ts, "cpython._PySys_ClearAuditHooks", NULL); _PyErr_Clear(ts); + PyThread_acquire_lock(runtime->audit_hooks.mutex, WAIT_LOCK); _Py_AuditHookEntry *e = runtime->audit_hooks.head, *n; runtime->audit_hooks.head = NULL; + PyThread_release_lock(runtime->audit_hooks.mutex); while (e) { n = e->next; PyMem_RawFree(e); @@ -389,6 +396,7 @@ PySys_AddAuditHook(Py_AuditHookFunction hook, void *userData) } } + PyThread_acquire_lock(runtime->audit_hooks.mutex, WAIT_LOCK); _Py_AuditHookEntry *e = runtime->audit_hooks.head; if (!e) { e = (_Py_AuditHookEntry*)PyMem_RawMalloc(sizeof(_Py_AuditHookEntry)); @@ -400,6 +408,7 @@ PySys_AddAuditHook(Py_AuditHookFunction hook, void *userData) e = e->next = (_Py_AuditHookEntry*)PyMem_RawMalloc( sizeof(_Py_AuditHookEntry)); } + PyThread_release_lock(runtime->audit_hooks.mutex); if (!e) { if (tstate != NULL) { From 8b83d9afd94f72c703ef256ffdb29a152c9ca7c2 Mon Sep 17 00:00:00 2001 From: Eric Snow Date: Wed, 7 Jun 2023 15:44:47 -0600 Subject: [PATCH 4/4] Fix usage of the audit hooks lock. --- Python/sysmodule.c | 59 ++++++++++++++++++++++++++++------------------ 1 file changed, 36 insertions(+), 23 deletions(-) diff --git a/Python/sysmodule.c b/Python/sysmodule.c index 60aee8f7f4ad1c..70274108a97cdb 100644 --- a/Python/sysmodule.c +++ b/Python/sysmodule.c @@ -168,10 +168,7 @@ should_audit(PyInterpreterState *interp) if (!interp) { return 0; } - PyThread_acquire_lock(interp->runtime->audit_hooks.mutex, WAIT_LOCK); - int has_global_hooks = (interp->runtime->audit_hooks.head != NULL); - PyThread_release_lock(interp->runtime->audit_hooks.mutex); - return (has_global_hooks + return (interp->runtime->audit_hooks.head || interp->audit_hooks || PyDTrace_AUDIT_ENABLED()); } @@ -227,10 +224,11 @@ sys_audit_tstate(PyThreadState *ts, const char *event, goto exit; } - /* Call global hooks */ - PyThread_acquire_lock(is->runtime->audit_hooks.mutex, WAIT_LOCK); + /* Call global hooks + * + * We don't worry about any races on hooks getting added, + * since that would not leave is in an inconsistent state. */ _Py_AuditHookEntry *e = is->runtime->audit_hooks.head; - PyThread_release_lock(is->runtime->audit_hooks.mutex); for (; e; e = e->next) { if (e->hookCFunction(event, eventArgs, e->userData) < 0) { goto exit; @@ -358,10 +356,12 @@ _PySys_ClearAuditHooks(PyThreadState *ts) _PySys_Audit(ts, "cpython._PySys_ClearAuditHooks", NULL); _PyErr_Clear(ts); - PyThread_acquire_lock(runtime->audit_hooks.mutex, WAIT_LOCK); + /* We don't worry about the very unlikely race right here, + * since it's entirely benign. Nothing else removes entries + * from the list and adding an entry right now would not cause + * any trouble. */ _Py_AuditHookEntry *e = runtime->audit_hooks.head, *n; runtime->audit_hooks.head = NULL; - PyThread_release_lock(runtime->audit_hooks.mutex); while (e) { n = e->next; PyMem_RawFree(e); @@ -369,6 +369,22 @@ _PySys_ClearAuditHooks(PyThreadState *ts) } } +static void +add_audit_hook_entry_unlocked(_PyRuntimeState *runtime, + _Py_AuditHookEntry *entry) +{ + if (runtime->audit_hooks.head == NULL) { + runtime->audit_hooks.head = entry; + } + else { + _Py_AuditHookEntry *last = runtime->audit_hooks.head; + while (last->next) { + last = last->next; + } + last->next = entry; + } +} + int PySys_AddAuditHook(Py_AuditHookFunction hook, void *userData) { @@ -396,31 +412,28 @@ PySys_AddAuditHook(Py_AuditHookFunction hook, void *userData) } } - PyThread_acquire_lock(runtime->audit_hooks.mutex, WAIT_LOCK); - _Py_AuditHookEntry *e = runtime->audit_hooks.head; - if (!e) { - e = (_Py_AuditHookEntry*)PyMem_RawMalloc(sizeof(_Py_AuditHookEntry)); - runtime->audit_hooks.head = e; - } else { - while (e->next) { - e = e->next; - } - e = e->next = (_Py_AuditHookEntry*)PyMem_RawMalloc( + _Py_AuditHookEntry *e = (_Py_AuditHookEntry*)PyMem_RawMalloc( sizeof(_Py_AuditHookEntry)); - } - PyThread_release_lock(runtime->audit_hooks.mutex); - if (!e) { if (tstate != NULL) { _PyErr_NoMemory(tstate); } return -1; } - e->next = NULL; e->hookCFunction = (Py_AuditHookFunction)hook; e->userData = userData; + if (runtime->audit_hooks.mutex == NULL) { + /* The runtime must not be initailized yet. */ + add_audit_hook_entry_unlocked(runtime, e); + } + else { + PyThread_acquire_lock(runtime->audit_hooks.mutex, WAIT_LOCK); + add_audit_hook_entry_unlocked(runtime, e); + PyThread_release_lock(runtime->audit_hooks.mutex); + } + return 0; }