gh-146270: Fix PyMember_SetOne(..., NULL) not being atomic

Author: dpdani

Lib/test/test_free_threading/test_slots.py

@@ -43,6 +43,51 @@ def reader():
 
         run_in_threads([writer, reader, reader, reader])
 
+    def test_del_object_is_atomic(self):
+        # Testing whether the implementation of `del slots_object.attribute`
+        # removes the attribute atomically, thus avoiding non-sequentially-
+        # consistent behaviors.
+        # https://github.com/python/cpython/issues/146270
+
+        class Spam:
+            __slots__ = [
+                "eggs",
+                "foo",
+            ]
+
+        spam = Spam()
+        iters = 1_000
+        non_seq_cst_behaviour_observed = False
+
+        def deleter():
+            barrier.wait()
+            try:
+                del spam.eggs
+            except AttributeError:
+                pass
+            else:
+                try:
+                    del spam.foo
+                except AttributeError:
+                    nonlocal non_seq_cst_behaviour_observed
+                    non_seq_cst_behaviour_observed = True
+                    # The fact that the else branch was reached implies that
+                    # the `del spam.eggs` call was successful. If that were
+                    # atomic, there is no way for two threads to enter the else
+                    # branch, therefore it must be that only one thread
+                    # attempts to `del spam.foo`. Thus, hitting an
+                    # AttributeError here is non-sequentially-consistent,
+                    # falsifying the assumption that `del spam.eggs` was
+                    # atomic. The test fails if this point is reached.
+
+        for _ in range(iters):
+            spam.eggs = 0
+            spam.foo = 0
+            barrier = _testcapi.SpinningBarrier(2)
+            # threading.Barrier would not create enough contention here
+            run_in_threads([deleter, deleter])
+            self.assertFalse(non_seq_cst_behaviour_observed)
+
     def test_T_BOOL(self):
         spam_old = _testcapi._test_structmembersType_OldAPI()
         spam_new = _testcapi._test_structmembersType_NewAPI()

Modules/_testcapimodule.c

@@ -2642,6 +2642,108 @@ test_soft_deprecated_macros(PyObject *Py_UNUSED(self), PyObject *Py_UNUSED(args)
     Py_RETURN_NONE;
 }
 
+/**
+ * A spinning barrier is a multithreading barrier similar to threading.Barrier,
+ * except that it never parks threads that are waiting on the barrier.
+ *
+ * This is useful in scenarios where it is desirable to increase contention on
+ * the code that follows the barrier. For instance, consider this test:
+ *
+ * def test_my_method_is_atomic():
+ *     x = MyClass()
+ *     b = _testcapi.SpinningBarrier()
+ *
+ *     def thread():
+ *         b.wait()
+ *         x.my_method()
+ *
+ *     for _ in range(1_000):
+ *         threads = [Thread(target=thread), Thread(target=thread)]
+ *         for t in threads: t.start()
+ *         for t in threads: t.join()
+ *
+ * It can be desirable (and sometimes necessary) to increase the contention
+ * on x's internal data structure by avoiding threads parking.
+ * Here, not parking may become necessary when the code in my_method() is so
+ * short that contention-related code paths are never exercised otherwise.
+ *
+ * It is roughly equivalent to this Python class:
+ *
+ * class SpinningBarrier:
+ *     def __init__(self, parties: int):
+ *         self.parties = AtomicInt(parties)  # if only we had atomic integers
+ *
+ *     def wait(self):
+ *         v = self.parties.decrement_and_get()
+ *         while True:
+ *             if v < 0:
+ *                 raise ValueError("wait was called too many times")
+ *             if v == 0:
+ *                 return
+ *             v = self.parties.get()
+ *
+ */
+
+typedef struct {
+    PyObject_HEAD
+    int parties;
+} SpinningBarrier;
+
+int
+SpinningBarrier_init(PyObject *self, PyObject *args, PyObject *kwargs)
+{
+    int parties = 0;
+    const char *kwlist[] = {"parties", NULL};
+    if (!PyArg_ParseTupleAndKeywords(args, kwargs, "i", (char **)kwlist, &parties)) {
+        return -1;
+    }
+    if (parties <= 0) {
+        PyErr_SetString(PyExc_ValueError, "parties must be greater than zero");
+        return -1;
+    }
+
+    SpinningBarrier *self_b = (SpinningBarrier *) self;
+    self_b->parties = parties;
+
+    return 0;
+}
+
+PyObject *
+SpinningBarrier_wait(PyObject *self, PyObject *Py_UNUSED(args))
+{
+    SpinningBarrier *self_b = (SpinningBarrier *) self;
+    const long decremented = _Py_atomic_add_int(&self_b->parties, -1) - 1;
+    long v = decremented;
+    while (1) {
+        if (v < 0) {
+            PyErr_SetString(PyExc_ValueError, "wait was called too many times");
+            return NULL;
+        }
+        if (v == 0) {
+            return PyLong_FromLong(decremented);
+        }
+        v = _Py_atomic_load_int_relaxed(&self_b->parties);
+        if (PyErr_CheckSignals()) {
+            return NULL;
+        }
+    }
+}
+
+static PyMethodDef SpinningBarrier_methods[] = {
+    {"wait", SpinningBarrier_wait, METH_NOARGS},
+    {NULL, NULL},
+};
+
+static PyTypeObject SpinningBarrier_Type = {
+    PyVarObject_HEAD_INIT(NULL, 0)
+    .tp_name = "SpinningBarrier",
+    .tp_basicsize = sizeof(SpinningBarrier),
+    .tp_new = PyType_GenericNew,
+    .tp_free = PyObject_Free,
+    .tp_init = &SpinningBarrier_init,
+    .tp_methods = SpinningBarrier_methods,
+};
+
 static PyMethodDef TestMethods[] = {
     {"set_errno",               set_errno,                       METH_VARARGS},
     {"test_config",             test_config,                     METH_NOARGS},
@@ -3369,6 +3471,12 @@ _testcapi_exec(PyObject *m)
     Py_INCREF(&MethStatic_Type);
     PyModule_AddObject(m, "MethStatic", (PyObject *)&MethStatic_Type);
 
+    if (PyType_Ready(&SpinningBarrier_Type) < 0) {
+        return -1;
+    }
+    Py_INCREF(&SpinningBarrier_Type);
+    PyModule_AddObject(m, "SpinningBarrier", (PyObject *)&SpinningBarrier_Type);
+
     PyModule_AddObject(m, "CHAR_MAX", PyLong_FromLong(CHAR_MAX));
     PyModule_AddObject(m, "CHAR_MIN", PyLong_FromLong(CHAR_MIN));
     PyModule_AddObject(m, "UCHAR_MAX", PyLong_FromLong(UCHAR_MAX));

Python/structmember.c

@@ -171,19 +171,10 @@ PyMember_SetOne(char *addr, PyMemberDef *l, PyObject *v)
         PyErr_SetString(PyExc_AttributeError, "readonly attribute");
         return -1;
     }
-    if (v == NULL) {
-        if (l->type == Py_T_OBJECT_EX) {
-            /* Check if the attribute is set. */
-            if (*(PyObject **)addr == NULL) {
-                PyErr_SetString(PyExc_AttributeError, l->name);
-                return -1;
-            }
-        }
-        else if (l->type != _Py_T_OBJECT) {
-            PyErr_SetString(PyExc_TypeError,
-                            "can't delete numeric/char attribute");
-            return -1;
-        }
+    if (v == NULL && l->type != Py_T_OBJECT_EX && l->type != _Py_T_OBJECT) {
+        PyErr_SetString(PyExc_TypeError,
+                        "can't delete numeric/char attribute");
+        return -1;
     }
     switch (l->type) {
     case Py_T_BOOL:{
@@ -335,6 +326,19 @@ PyMember_SetOne(char *addr, PyMemberDef *l, PyObject *v)
         FT_ATOMIC_STORE_PTR_RELEASE(*(PyObject **)addr, Py_XNewRef(v));
         Py_END_CRITICAL_SECTION();
         Py_XDECREF(oldv);
+        if (v == NULL && oldv == NULL && l->type == Py_T_OBJECT_EX) {
+            // Pseudo-non-existing attribute is deleted: raise AttributeError.
+            // The attribute doesn't exist to Python, but CPython knows that it
+            // could have existed because it was declared in __slots__.
+            // _Py_T_OBJECT does not raise an exception here, and
+            // PyMember_GetOne will return Py_None instead of NULL.
+            PyErr_SetString(PyExc_AttributeError, l->name);
+            return -1;
+        }
+        // Other cases are already covered by the above:
+        // oldv == NULL && v != NULL: pseudo-non-existing attribute is set, ok
+        // oldv != NULL && v == NULL: existing attribute is deleted, ok
+        // oldv != NULL && v != NULL: existing attribute is set, ok
         break;
     case Py_T_CHAR: {
         const char *string;

Tools/c-analyzer/cpython/ignored.tsv

@@ -542,6 +542,7 @@ Modules/_testcapimodule.c	-	meth_class_methods	-
 Modules/_testcapimodule.c	-	meth_instance_methods	-
 Modules/_testcapimodule.c	-	meth_static_methods	-
 Modules/_testcapimodule.c	-	ml	-
+Modules/_testcapimodule.c	-	SpinningBarrier_Type	-
 Modules/_testcapimodule.c	-	str1	-
 Modules/_testcapimodule.c	-	str2	-
 Modules/_testcapimodule.c	-	test_c_thread	-