From c1712ef066321c01bf09cba3f22fc474b5b8dfa7 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Thu, 28 Mar 2024 16:05:08 +0100 Subject: [PATCH] gh-116664: Make module state Py_SETREF's in _warnings thread-safe (#116959) Mark the swap operations as critical sections. Add an internal Py_BEGIN_CRITICAL_SECTION_MUT API that takes a PyMutex pointer instead of a PyObject pointer. --- Include/internal/pycore_critical_section.h | 8 ++- Include/internal/pycore_warnings.h | 1 + Python/_warnings.c | 58 ++++++++++++++-------- 3 files changed, 44 insertions(+), 23 deletions(-) diff --git a/Include/internal/pycore_critical_section.h b/Include/internal/pycore_critical_section.h index 9163b5cf0f2..23b85c2f9e9 100644 --- a/Include/internal/pycore_critical_section.h +++ b/Include/internal/pycore_critical_section.h @@ -87,10 +87,13 @@ extern "C" { #define _Py_CRITICAL_SECTION_MASK 0x3 #ifdef Py_GIL_DISABLED -# define Py_BEGIN_CRITICAL_SECTION(op) \ +# define Py_BEGIN_CRITICAL_SECTION_MUT(mutex) \ { \ _PyCriticalSection _cs; \ - _PyCriticalSection_Begin(&_cs, &_PyObject_CAST(op)->ob_mutex) + _PyCriticalSection_Begin(&_cs, mutex) + +# define Py_BEGIN_CRITICAL_SECTION(op) \ + Py_BEGIN_CRITICAL_SECTION_MUT(&_PyObject_CAST(op)->ob_mutex) # define Py_END_CRITICAL_SECTION() \ _PyCriticalSection_End(&_cs); \ @@ -138,6 +141,7 @@ extern "C" { #else /* !Py_GIL_DISABLED */ // The critical section APIs are no-ops with the GIL. +# define Py_BEGIN_CRITICAL_SECTION_MUT(mut) # define Py_BEGIN_CRITICAL_SECTION(op) # define Py_END_CRITICAL_SECTION() # define Py_XBEGIN_CRITICAL_SECTION(op) diff --git a/Include/internal/pycore_warnings.h b/Include/internal/pycore_warnings.h index 9785d7cc467..114796df42b 100644 --- a/Include/internal/pycore_warnings.h +++ b/Include/internal/pycore_warnings.h @@ -14,6 +14,7 @@ struct _warnings_runtime_state { PyObject *filters; /* List */ PyObject *once_registry; /* Dict */ PyObject *default_action; /* String */ + struct _PyMutex mutex; long filters_version; }; diff --git a/Python/_warnings.c b/Python/_warnings.c index dfa82c569e1..66a460e2a2c 100644 --- a/Python/_warnings.c +++ b/Python/_warnings.c @@ -1,4 +1,5 @@ #include "Python.h" +#include "pycore_critical_section.h" // Py_BEGIN_CRITICAL_SECTION_MUT() #include "pycore_interp.h" // PyInterpreterState.warnings #include "pycore_long.h" // _PyLong_GetZero() #include "pycore_pyerrors.h" // _PyErr_Occurred() @@ -235,14 +236,12 @@ get_warnings_attr(PyInterpreterState *interp, PyObject *attr, int try_import) static PyObject * get_once_registry(PyInterpreterState *interp) { - PyObject *registry; - WarningsState *st = warnings_get_state(interp); - if (st == NULL) { - return NULL; - } + assert(st != NULL); - registry = GET_WARNINGS_ATTR(interp, onceregistry, 0); + _Py_CRITICAL_SECTION_ASSERT_MUTEX_LOCKED(&st->mutex); + + PyObject *registry = GET_WARNINGS_ATTR(interp, onceregistry, 0); if (registry == NULL) { if (PyErr_Occurred()) return NULL; @@ -265,14 +264,12 @@ get_once_registry(PyInterpreterState *interp) static PyObject * get_default_action(PyInterpreterState *interp) { - PyObject *default_action; - WarningsState *st = warnings_get_state(interp); - if (st == NULL) { - return NULL; - } + assert(st != NULL); - default_action = GET_WARNINGS_ATTR(interp, defaultaction, 0); + _Py_CRITICAL_SECTION_ASSERT_MUTEX_LOCKED(&st->mutex); + + PyObject *default_action = GET_WARNINGS_ATTR(interp, defaultaction, 0); if (default_action == NULL) { if (PyErr_Occurred()) { return NULL; @@ -299,15 +296,12 @@ get_filter(PyInterpreterState *interp, PyObject *category, PyObject *text, Py_ssize_t lineno, PyObject *module, PyObject **item) { - PyObject *action; - Py_ssize_t i; - PyObject *warnings_filters; WarningsState *st = warnings_get_state(interp); - if (st == NULL) { - return NULL; - } + assert(st != NULL); - warnings_filters = GET_WARNINGS_ATTR(interp, filters, 0); + _Py_CRITICAL_SECTION_ASSERT_MUTEX_LOCKED(&st->mutex); + + PyObject *warnings_filters = GET_WARNINGS_ATTR(interp, filters, 0); if (warnings_filters == NULL) { if (PyErr_Occurred()) return NULL; @@ -324,7 +318,7 @@ get_filter(PyInterpreterState *interp, PyObject *category, } /* WarningsState.filters could change while we are iterating over it. */ - for (i = 0; i < PyList_GET_SIZE(filters); i++) { + for (Py_ssize_t i = 0; i < PyList_GET_SIZE(filters); i++) { PyObject *tmp_item, *action, *msg, *cat, *mod, *ln_obj; Py_ssize_t ln; int is_subclass, good_msg, good_mod; @@ -384,7 +378,7 @@ get_filter(PyInterpreterState *interp, PyObject *category, Py_DECREF(tmp_item); } - action = get_default_action(interp); + PyObject *action = get_default_action(interp); if (action != NULL) { *item = Py_NewRef(Py_None); return action; @@ -1000,8 +994,13 @@ do_warn(PyObject *message, PyObject *category, Py_ssize_t stack_level, &filename, &lineno, &module, ®istry)) return NULL; + WarningsState *st = warnings_get_state(tstate->interp); + assert(st != NULL); + + Py_BEGIN_CRITICAL_SECTION_MUT(&st->mutex); res = warn_explicit(tstate, category, message, filename, lineno, module, registry, NULL, source); + Py_END_CRITICAL_SECTION(); Py_DECREF(filename); Py_DECREF(registry); Py_DECREF(module); @@ -1149,8 +1148,14 @@ warnings_warn_explicit_impl(PyObject *module, PyObject *message, return NULL; } } + + WarningsState *st = warnings_get_state(tstate->interp); + assert(st != NULL); + + Py_BEGIN_CRITICAL_SECTION_MUT(&st->mutex); returned = warn_explicit(tstate, category, message, filename, lineno, mod, registry, source_line, sourceobj); + Py_END_CRITICAL_SECTION(); Py_XDECREF(source_line); return returned; } @@ -1290,8 +1295,14 @@ PyErr_WarnExplicitObject(PyObject *category, PyObject *message, if (tstate == NULL) { return -1; } + + WarningsState *st = warnings_get_state(tstate->interp); + assert(st != NULL); + + Py_BEGIN_CRITICAL_SECTION_MUT(&st->mutex); res = warn_explicit(tstate, category, message, filename, lineno, module, registry, NULL, NULL); + Py_END_CRITICAL_SECTION(); if (res == NULL) return -1; Py_DECREF(res); @@ -1356,8 +1367,13 @@ PyErr_WarnExplicitFormat(PyObject *category, PyObject *res; PyThreadState *tstate = get_current_tstate(); if (tstate != NULL) { + WarningsState *st = warnings_get_state(tstate->interp); + assert(st != NULL); + + Py_BEGIN_CRITICAL_SECTION_MUT(&st->mutex); res = warn_explicit(tstate, category, message, filename, lineno, module, registry, NULL, NULL); + Py_END_CRITICAL_SECTION(); Py_DECREF(message); if (res != NULL) { Py_DECREF(res);