diff --git a/Doc/howto/free-threading-extensions.rst b/Doc/howto/free-threading-extensions.rst index 6abe93d71ad529..c1ad42e7e55ee5 100644 --- a/Doc/howto/free-threading-extensions.rst +++ b/Doc/howto/free-threading-extensions.rst @@ -167,6 +167,8 @@ that return :term:`strong references `. +-----------------------------------+-----------------------------------+ | :c:func:`PyImport_AddModule` | :c:func:`PyImport_AddModuleRef` | +-----------------------------------+-----------------------------------+ +| :c:func:`PyCell_GET` | :c:func:`PyCell_Get` | ++-----------------------------------+-----------------------------------+ Not all APIs that return borrowed references are problematic. For example, :c:func:`PyTuple_GetItem` is safe because tuples are immutable. diff --git a/Include/Python.h b/Include/Python.h index 717e27feab62db..64be80145890a3 100644 --- a/Include/Python.h +++ b/Include/Python.h @@ -69,6 +69,7 @@ #include "pystats.h" #include "pyatomic.h" #include "lock.h" +#include "critical_section.h" #include "object.h" #include "refcount.h" #include "objimpl.h" @@ -130,7 +131,6 @@ #include "import.h" #include "abstract.h" #include "bltinmodule.h" -#include "critical_section.h" #include "cpython/pyctype.h" #include "pystrtod.h" #include "pystrcmp.h" diff --git a/Include/cpython/cellobject.h b/Include/cpython/cellobject.h index 47a6a491497ea0..85a63a13747d87 100644 --- a/Include/cpython/cellobject.h +++ b/Include/cpython/cellobject.h @@ -22,10 +22,14 @@ PyAPI_FUNC(PyObject *) PyCell_Get(PyObject *); PyAPI_FUNC(int) PyCell_Set(PyObject *, PyObject *); static inline PyObject* PyCell_GET(PyObject *op) { + PyObject *res; PyCellObject *cell; assert(PyCell_Check(op)); cell = _Py_CAST(PyCellObject*, op); - return cell->ob_ref; + Py_BEGIN_CRITICAL_SECTION(cell); + res = cell->ob_ref; + Py_END_CRITICAL_SECTION(); + return res; } #define PyCell_GET(op) PyCell_GET(_PyObject_CAST(op)) @@ -33,7 +37,9 @@ static inline void PyCell_SET(PyObject *op, PyObject *value) { PyCellObject *cell; assert(PyCell_Check(op)); cell = _Py_CAST(PyCellObject*, op); + Py_BEGIN_CRITICAL_SECTION(cell); cell->ob_ref = value; + Py_END_CRITICAL_SECTION(); } #define PyCell_SET(op, value) PyCell_SET(_PyObject_CAST(op), (value)) diff --git a/Lib/test/test_free_threading/test_races.py b/Lib/test/test_free_threading/test_races.py new file mode 100644 index 00000000000000..09e1d52e3509f9 --- /dev/null +++ b/Lib/test/test_free_threading/test_races.py @@ -0,0 +1,134 @@ +# It's most useful to run these tests with ThreadSanitizer enabled. +import sys +import functools +import threading +import time +import unittest + +from test.support import threading_helper + + +class TestBase(unittest.TestCase): + pass + + +def do_race(func1, func2): + """Run func1() and func2() repeatedly in separate threads.""" + n = 1000 + + barrier = threading.Barrier(2) + + def repeat(func): + barrier.wait() + for _i in range(n): + func() + + threads = [ + threading.Thread(target=functools.partial(repeat, func1)), + threading.Thread(target=functools.partial(repeat, func2)), + ] + for thread in threads: + thread.start() + for thread in threads: + thread.join() + + +@threading_helper.requires_working_threading() +class TestRaces(TestBase): + def test_racing_cell_set(self): + """Test cell object gettr/settr properties.""" + + def nested_func(): + x = 0 + + def inner(): + nonlocal x + x += 1 + + # This doesn't race because LOAD_DEREF and STORE_DEREF on the + # cell object use critical sections. + do_race(nested_func, nested_func) + + def nested_func2(): + x = 0 + + def inner(): + y = x + frame = sys._getframe(1) + frame.f_locals["x"] = 2 + + return inner + + def mutate_func2(): + inner = nested_func2() + cell = inner.__closure__[0] + old_value = cell.cell_contents + cell.cell_contents = 1000 + time.sleep(0) + cell.cell_contents = old_value + time.sleep(0) + + # This revealed a race with cell_set_contents() since it was missing + # the critical section. + do_race(nested_func2, mutate_func2) + + def test_racing_cell_cmp_repr(self): + """Test cell object compare and repr methods.""" + + def nested_func(): + x = 0 + y = 0 + + def inner(): + return x + y + + return inner.__closure__ + + cell_a, cell_b = nested_func() + + def mutate(): + cell_a.cell_contents += 1 + + def access(): + cell_a == cell_b + s = repr(cell_a) + + # cell_richcompare() and cell_repr used to have data races + do_race(mutate, access) + + def test_racing_load_super_attr(self): + """Test (un)specialization of LOAD_SUPER_ATTR opcode.""" + + class C: + def __init__(self): + try: + super().__init__ + super().__init__() + except RuntimeError: + pass # happens if __class__ is replaced with non-type + + def access(): + C() + + def mutate(): + # Swap out the super() global with a different one + real_super = super + globals()["super"] = lambda s=1: s + time.sleep(0) + globals()["super"] = real_super + time.sleep(0) + # Swap out the __class__ closure value with a non-type + cell = C.__init__.__closure__[0] + real_class = cell.cell_contents + cell.cell_contents = 99 + time.sleep(0) + cell.cell_contents = real_class + + # The initial PR adding specialized opcodes for LOAD_SUPER_ATTR + # had some races (one with the super() global changing and one + # with the cell binding being changed). + do_race(access, mutate) + + +if __name__ == "__main__": + unittest.main() diff --git a/Objects/cellobject.c b/Objects/cellobject.c index 4ab9083af5e300..ec2eeb1a855b63 100644 --- a/Objects/cellobject.c +++ b/Objects/cellobject.c @@ -82,6 +82,17 @@ cell_dealloc(PyObject *self) PyObject_GC_Del(op); } +static PyObject * +cell_compare_impl(PyObject *a, PyObject *b, int op) +{ + if (a != NULL && b != NULL) { + return PyObject_RichCompare(a, b, op); + } + else { + Py_RETURN_RICHCOMPARE(b == NULL, a == NULL, op); + } +} + static PyObject * cell_richcompare(PyObject *a, PyObject *b, int op) { @@ -92,27 +103,28 @@ cell_richcompare(PyObject *a, PyObject *b, int op) if (!PyCell_Check(a) || !PyCell_Check(b)) { Py_RETURN_NOTIMPLEMENTED; } + PyObject *a_ref = PyCell_GetRef((PyCellObject *)a); + PyObject *b_ref = PyCell_GetRef((PyCellObject *)b); /* compare cells by contents; empty cells come before anything else */ - a = ((PyCellObject *)a)->ob_ref; - b = ((PyCellObject *)b)->ob_ref; - if (a != NULL && b != NULL) - return PyObject_RichCompare(a, b, op); + PyObject *res = cell_compare_impl(a_ref, b_ref, op); - Py_RETURN_RICHCOMPARE(b == NULL, a == NULL, op); + Py_XDECREF(a_ref); + Py_XDECREF(b_ref); + return res; } static PyObject * cell_repr(PyObject *self) { - PyCellObject *op = _PyCell_CAST(self); - if (op->ob_ref == NULL) { - return PyUnicode_FromFormat("", op); + PyObject *ref = PyCell_GetRef((PyCellObject *)self); + if (ref == NULL) { + return PyUnicode_FromFormat("", self); } - - return PyUnicode_FromFormat("", - op, Py_TYPE(op->ob_ref)->tp_name, - op->ob_ref); + PyObject *res = PyUnicode_FromFormat("", + self, Py_TYPE(ref)->tp_name, ref); + Py_DECREF(ref); + return res; } static int @@ -135,11 +147,12 @@ static PyObject * cell_get_contents(PyObject *self, void *closure) { PyCellObject *op = _PyCell_CAST(self); - if (op->ob_ref == NULL) { + PyObject *res = PyCell_GetRef(op); + if (res == NULL) { PyErr_SetString(PyExc_ValueError, "Cell is empty"); return NULL; } - return Py_NewRef(op->ob_ref); + return res; } static int diff --git a/Objects/frameobject.c b/Objects/frameobject.c index c743c254848d3a..03ed2b9480f8c9 100644 --- a/Objects/frameobject.c +++ b/Objects/frameobject.c @@ -8,6 +8,7 @@ #include "pycore_moduleobject.h" // _PyModule_GetDict() #include "pycore_modsupport.h" // _PyArg_CheckPositional() #include "pycore_object.h" // _PyObject_GC_UNTRACK() +#include "pycore_cell.h" // PyCell_GetRef() PyCell_SetTakeRef() #include "pycore_opcode_metadata.h" // _PyOpcode_Deopt, _PyOpcode_Caches @@ -187,11 +188,8 @@ framelocalsproxy_setitem(PyObject *self, PyObject *key, PyObject *value) } } if (cell != NULL) { - PyObject *oldvalue_o = PyCell_GET(cell); - if (value != oldvalue_o) { - PyCell_SET(cell, Py_XNewRef(value)); - Py_XDECREF(oldvalue_o); - } + Py_XINCREF(value); + PyCell_SetTakeRef((PyCellObject *)cell, value); } else if (value != PyStackRef_AsPyObjectBorrow(oldvalue)) { PyStackRef_XCLOSE(fast[i]); fast[i] = PyStackRef_FromPyObjectNew(value); @@ -1987,19 +1985,25 @@ frame_get_var(_PyInterpreterFrame *frame, PyCodeObject *co, int i, if (kind & CO_FAST_FREE) { // The cell was set by COPY_FREE_VARS. assert(value != NULL && PyCell_Check(value)); - value = PyCell_GET(value); + value = PyCell_GetRef((PyCellObject *)value); } else if (kind & CO_FAST_CELL) { if (value != NULL) { if (PyCell_Check(value)) { assert(!_PyFrame_IsIncomplete(frame)); - value = PyCell_GET(value); + value = PyCell_GetRef((PyCellObject *)value); + } + else { + // (likely) Otherwise it is an arg (kind & CO_FAST_LOCAL), + // with the initial value set when the frame was created... + // (unlikely) ...or it was set via the f_locals proxy. + Py_INCREF(value); } - // (likely) Otherwise it is an arg (kind & CO_FAST_LOCAL), - // with the initial value set when the frame was created... - // (unlikely) ...or it was set via the f_locals proxy. } } + else { + Py_XINCREF(value); + } } *pvalue = value; return 1; @@ -2076,14 +2080,14 @@ PyFrame_GetVar(PyFrameObject *frame_obj, PyObject *name) continue; } - PyObject *value; // borrowed reference + PyObject *value; if (!frame_get_var(frame, co, i, &value)) { break; } if (value == NULL) { break; } - return Py_NewRef(value); + return value; } PyErr_Format(PyExc_NameError, "variable %R does not exist", name); diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 2611404a3d0d61..bf9049bce3adeb 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -19,6 +19,7 @@ #include "pycore_typeobject.h" // struct type_cache #include "pycore_unionobject.h" // _Py_union_type_or #include "pycore_weakref.h" // _PyWeakref_GET_REF() +#include "pycore_cell.h" // PyCell_GetRef() #include "opcode.h" // MAKE_CELL #include // ptrdiff_t @@ -11676,23 +11677,28 @@ super_init_without_args(_PyInterpreterFrame *cframe, PyTypeObject **type_p, assert(_PyFrame_GetCode(cframe)->co_nlocalsplus > 0); PyObject *firstarg = PyStackRef_AsPyObjectBorrow(_PyFrame_GetLocalsArray(cframe)[0]); + if (firstarg == NULL) { + PyErr_SetString(PyExc_RuntimeError, "super(): arg[0] deleted"); + return -1; + } // The first argument might be a cell. - if (firstarg != NULL && (_PyLocals_GetKind(co->co_localspluskinds, 0) & CO_FAST_CELL)) { - // "firstarg" is a cell here unless (very unlikely) super() - // was called from the C-API before the first MAKE_CELL op. - if (_PyInterpreterFrame_LASTI(cframe) >= 0) { - // MAKE_CELL and COPY_FREE_VARS have no quickened forms, so no need - // to use _PyOpcode_Deopt here: - assert(_PyCode_CODE(co)[0].op.code == MAKE_CELL || - _PyCode_CODE(co)[0].op.code == COPY_FREE_VARS); - assert(PyCell_Check(firstarg)); - firstarg = PyCell_GET(firstarg); + // "firstarg" is a cell here unless (very unlikely) super() + // was called from the C-API before the first MAKE_CELL op. + if ((_PyLocals_GetKind(co->co_localspluskinds, 0) & CO_FAST_CELL) && + (_PyInterpreterFrame_LASTI(cframe) >= 0)) { + // MAKE_CELL and COPY_FREE_VARS have no quickened forms, so no need + // to use _PyOpcode_Deopt here: + assert(_PyCode_CODE(co)[0].op.code == MAKE_CELL || + _PyCode_CODE(co)[0].op.code == COPY_FREE_VARS); + assert(PyCell_Check(firstarg)); + firstarg = PyCell_GetRef((PyCellObject *)firstarg); + if (firstarg == NULL) { + PyErr_SetString(PyExc_RuntimeError, "super(): arg[0] deleted"); + return -1; } } - if (firstarg == NULL) { - PyErr_SetString(PyExc_RuntimeError, - "super(): arg[0] deleted"); - return -1; + else { + Py_INCREF(firstarg); } // Look for __class__ in the free vars. @@ -11707,18 +11713,22 @@ super_init_without_args(_PyInterpreterFrame *cframe, PyTypeObject **type_p, if (cell == NULL || !PyCell_Check(cell)) { PyErr_SetString(PyExc_RuntimeError, "super(): bad __class__ cell"); + Py_DECREF(firstarg); return -1; } - type = (PyTypeObject *) PyCell_GET(cell); + type = (PyTypeObject *) PyCell_GetRef((PyCellObject *)cell); if (type == NULL) { PyErr_SetString(PyExc_RuntimeError, "super(): empty __class__ cell"); + Py_DECREF(firstarg); return -1; } if (!PyType_Check(type)) { PyErr_Format(PyExc_RuntimeError, "super(): __class__ is not a type (%s)", Py_TYPE(type)->tp_name); + Py_DECREF(type); + Py_DECREF(firstarg); return -1; } break; @@ -11727,6 +11737,7 @@ super_init_without_args(_PyInterpreterFrame *cframe, PyTypeObject **type_p, if (type == NULL) { PyErr_SetString(PyExc_RuntimeError, "super(): __class__ cell not found"); + Py_DECREF(firstarg); return -1; } @@ -11773,16 +11784,24 @@ super_init_impl(PyObject *self, PyTypeObject *type, PyObject *obj) { return -1; } } + else { + Py_INCREF(type); + Py_XINCREF(obj); + } - if (obj == Py_None) + if (obj == Py_None) { + Py_DECREF(obj); obj = NULL; + } if (obj != NULL) { obj_type = supercheck(type, obj); - if (obj_type == NULL) + if (obj_type == NULL) { + Py_DECREF(type); + Py_DECREF(obj); return -1; - Py_INCREF(obj); + } } - Py_XSETREF(su->type, (PyTypeObject*)Py_NewRef(type)); + Py_XSETREF(su->type, (PyTypeObject*)type); Py_XSETREF(su->obj, obj); Py_XSETREF(su->obj_type, obj_type); return 0; diff --git a/Python/bltinmodule.c b/Python/bltinmodule.c index 17df9208f224f4..fb9868b3740b8c 100644 --- a/Python/bltinmodule.c +++ b/Python/bltinmodule.c @@ -13,6 +13,7 @@ #include "pycore_pythonrun.h" // _Py_SourceAsString() #include "pycore_sysmodule.h" // _PySys_GetAttr() #include "pycore_tuple.h" // _PyTuple_FromArray() +#include "pycore_cell.h" // PyCell_GetRef() #include "clinic/bltinmodule.c.h" @@ -209,7 +210,7 @@ builtin___build_class__(PyObject *self, PyObject *const *args, Py_ssize_t nargs, PyObject *margs[3] = {name, bases, ns}; cls = PyObject_VectorcallDict(meta, margs, 3, mkw); if (cls != NULL && PyType_Check(cls) && PyCell_Check(cell)) { - PyObject *cell_cls = PyCell_GET(cell); + PyObject *cell_cls = PyCell_GetRef((PyCellObject *)cell); if (cell_cls != cls) { if (cell_cls == NULL) { const char *msg = @@ -221,9 +222,13 @@ builtin___build_class__(PyObject *self, PyObject *const *args, Py_ssize_t nargs, "__class__ set to %.200R defining %.200R as %.200R"; PyErr_Format(PyExc_TypeError, msg, cell_cls, name, cls); } + Py_XDECREF(cell_cls); Py_SETREF(cls, NULL); goto error; } + else { + Py_DECREF(cell_cls); + } } } error: