Drop usage of PyList_ macros

Switch to the function API, and in particular to PyList_GetItemRef instead of PyList_GET_ITEM because the latter is problematic in free threaded mode. Issue #608
ronaldoussoren · Jul 16, 2024 · 2149aba · 2149aba
1 parent 6f5aa4e
commit 2149aba
Show file tree

Hide file tree

Showing 8 changed files with 110 additions and 27 deletions.
diff --git a/pyobjc-core/Modules/objc/class-builder.m b/pyobjc-core/Modules/objc/class-builder.m
@@ -371,8 +371,12 @@ Class _Nullable PyObjCClass_BuildClass(Class super_class, PyObject* protocols, c
     }
     for (i = 0; i < protocol_count; i++) {
         PyObject* wrapped_protocol;
-        wrapped_protocol = PyList_GET_ITEM(protocols, i);
+        wrapped_protocol = PyList_GetItemRef(protocols, i);
+        if (wrapped_protocol == NULL) {
+            goto error_cleanup;
+        }
         if (!PyObjCFormalProtocol_Check(wrapped_protocol)) {
+            Py_DECREF(wrapped_protocol);
             continue;
         }
 
@@ -381,8 +385,10 @@ Class _Nullable PyObjCClass_BuildClass(Class super_class, PyObject* protocols, c
         if (!class_addProtocol( // LCOV_BR_EXCL_LINE
                 new_class,
                 (Protocol* _Nonnull)PyObjCFormalProtocol_GetProtocol(wrapped_protocol))) {
+            Py_DECREF(wrapped_protocol);
             goto error_cleanup; // LCOV_EXCL_LINE
         }
+        Py_DECREF(wrapped_protocol);
     }
 
     /* add instance variables */

diff --git a/pyobjc-core/Modules/objc/module.m b/pyobjc-core/Modules/objc/module.m
@@ -792,7 +792,7 @@ + (void)targetForBecomingMultiThreaded:(id)sender
         return Py_None;
     }
 
-    protocols = PyList_New(protCount);
+    protocols = PyList_New(0);
 
     if (protocols == NULL) { // LCOV_BR_EXCL_LINE
         return NULL;         // LCOV_EXCL_LINE
@@ -808,7 +808,14 @@ + (void)targetForBecomingMultiThreaded:(id)sender
             // LCOV_EXCL_STOP
         }
 
-        PyList_SET_ITEM(protocols, i, p);
+        int r = PyList_Append(protocols, p);
+        if (r == -1) { // LCOV_BR_EXCL_START
+            // LCOV_EXCL_START
+            Py_DECREF(protocols);
+            free(protlist);
+            return NULL;
+            // LCOV_EXCL_STOP
+        }
     }
 
     free(protlist);

diff --git a/pyobjc-core/Modules/objc/objc-class.m b/pyobjc-core/Modules/objc/objc-class.m
@@ -552,7 +552,7 @@ static Class _Nullable objc_metaclass_locate(PyObject* meta_class)
         Py_DECREF(protocols);
 
         protocols_len = PySequence_Fast_GET_SIZE(seq);
-        protocols     = PyList_New(protocols_len);
+        protocols     = PyList_New(0);
         if (protocols == NULL) { // LCOV_BR_EXCL_LINE
             // LCOV_EXCL_START
             Py_DECREF(hiddenSelectors);
@@ -562,8 +562,13 @@ static Class _Nullable objc_metaclass_locate(PyObject* meta_class)
         }
 
         for (i = 0; i < protocols_len; i++) {
-            PyList_SET_ITEM(protocols, i, PySequence_Fast_GET_ITEM(seq, i));
-            Py_INCREF(PySequence_Fast_GET_ITEM(seq, i));
+            if (PyList_Append(protocols,  PySequence_Fast_GET_ITEM(seq, i)) < 0) { // LCOV_BR_EXCL_LINE
+                // LCOV_EXCL_START
+                Py_DECREF(hiddenSelectors);
+                Py_DECREF(hiddenClassSelectors);
+                return NULL;
+                // LCOV_EXCL_STOP
+            }
         }
 
         Py_DECREF(seq);
@@ -716,8 +721,17 @@ static Class _Nullable objc_metaclass_locate(PyObject* meta_class)
             }
         }
 
-        Py_DECREF(PyList_GET_ITEM(real_bases, 0));
-        PyList_SET_ITEM(real_bases, 0, py_super_class);
+        if (PyList_SetItem(real_bases, 0, py_super_class) < 0) {
+            PyObjC_Assert(objc_class != Nil, NULL);
+            (void)PyObjCClass_UnbuildClass(objc_class);
+            Py_XDECREF(orig_slots);
+            Py_DECREF(protocols);
+            Py_DECREF(real_bases);
+            Py_DECREF(metadict);
+            Py_DECREF(hiddenSelectors);
+            Py_DECREF(hiddenClassSelectors);
+            return NULL;
+        }
     }
 
     v = PyList_AsTuple(real_bases);
@@ -1039,15 +1053,19 @@ static Class _Nullable objc_metaclass_locate(PyObject* meta_class)
     }
 
     /* Merge the "difference" to pick up new selectors */
-    len = PyList_GET_SIZE(keys);
+    len = PyList_Size(keys);
     for (i = 0; i < len; i++) {
-        k = PyList_GET_ITEM(keys, i);
+        k = PyList_GetItemRef(keys, i);
+        if (k == NULL) {
+            return NULL;
+        }
         if (PyDict_GetItem(old_dict, k) == NULL) {
             v = PyDict_GetItem(dict, k);
             if (v != NULL && PyObject_SetAttr(res, k, v) == -1) {
                 PyErr_Clear();
             }
         }
+        Py_DECREF(k);
     }
 
     Py_DECREF(keys);

diff --git a/pyobjc-core/Modules/objc/objc_util.m b/pyobjc-core/Modules/objc/objc_util.m
@@ -1396,18 +1396,23 @@
 
     PyObjC_Assert(PyList_Check(values), NULL);
 
-    len = PyList_GET_SIZE(values);
+    len = PyList_Size(values);
     for (i = 0; i < len; i++) {
-        PyObject* v = PyList_GET_ITEM(values, i);
+        PyObject* v = PyList_GetItemRef(values, i);
+        if (v == NULL) {
+            return NULL;
+        }
 
-        if (!PyObjCSelector_Check(v))
+        if (!PyObjCSelector_Check(v)) {
+            Py_DECREF(v);
             continue;
+        }
 
         if (PyObjCSelector_GetSelector(v) == selector) {
             Py_DECREF(values);
-            Py_INCREF(v);
             return v;
         }
+        Py_DECREF(v);
     }
 
     Py_DECREF(values);

diff --git a/pyobjc-core/Modules/objc/pyobjc-compat.h b/pyobjc-core/Modules/objc/pyobjc-compat.h
@@ -262,6 +262,13 @@ static inline int PyDict_GetItemRef(PyObject *p, PyObject *key, PyObject * _Nonn
         return 1;
     }
 }
+
+static inline PyObject* _Nullable PyList_GetItemRef(PyObject* l, Py_ssize_t i)
+{
+    PyObject* result = PyList_GetItem(l, i);
+    Py_XINCREF(result);
+    return result;
+}
 #endif
 
 

diff --git a/pyobjc-core/Modules/objc/registry.m b/pyobjc-core/Modules/objc/registry.m
@@ -46,19 +46,26 @@
      * Check if there is a registration for *class_name* in
      * *sublist*, if so replace that registration.
      */
-    for (Py_ssize_t i = 0; i < PyList_GET_SIZE(sublist); i++) {
-        PyObject* item = PyList_GET_ITEM(sublist, i);
+    Py_ssize_t len = PyList_Size(sublist);
+    for (Py_ssize_t i = 0; i < len; i++) {
+        PyObject* item = PyList_GetItemRef(sublist, i);
+        if (item == NULL) {
+            return -1;
+        }
 
         PyObjC_Assert(PyTuple_CheckExact(item), -1);
         PyObjC_Assert(PyTuple_GET_SIZE(item) == 2, -1);
 
         int r = PyObject_RichCompareBool(PyTuple_GET_ITEM(item, 0), class_name, Py_EQ);
-        if (r == -1)   // LCOV_BR_EXCL_LINE
+        if (r == -1) {  // LCOV_BR_EXCL_LINE
+            Py_DECREF(item); // LCOV_EXCL_LINE
             return -1; // LCOV_EXCL_LINE
+        }
         if (r) {
             Py_DECREF(PyTuple_GET_ITEM(item, 1));
             PyTuple_SET_ITEM(item, 1, value);
             Py_INCREF(value);
+            Py_DECREF(item);
             return 0;
         }
     }
@@ -96,7 +103,7 @@
     for (i = 0; i < len; i++) {
         Class cur_class;
 
-        cur = PyList_GET_ITEM(sublist, i);
+        cur = PyList_GetItemRef(sublist, i);
         PyObjC_Assert(cur != NULL, NULL);
         PyObjC_Assert(PyTuple_CheckExact(cur), NULL);
 
@@ -106,17 +113,20 @@
         cur_class = objc_lookUpClass(PyBytes_AsString(nm));
 
         if (cur_class == nil) {
+            Py_DECREF(cur);
             continue;
         }
 
         if (!PyObjC_class_isSubclassOf(cls, cur_class)
             && !PyObjC_class_isSubclassOf(cls,
                                           (Class _Nonnull)object_getClass(cur_class))) {
+            Py_DECREF(cur);
             continue;
         }
 
         if (found_class != NULL && found_class != cur_class) {
             if (PyObjC_class_isSubclassOf(found_class, cur_class)) {
+                Py_DECREF(cur);
                 continue;
             }
         }
@@ -125,6 +135,7 @@
         Py_INCREF(PyTuple_GET_ITEM(cur, 1));
         Py_XDECREF(found_value);
         found_value = PyTuple_GET_ITEM(cur, 1);
+        Py_DECREF(cur);
     }
 
     return found_value;
@@ -155,8 +166,8 @@
         }
 #endif
 
-        len    = PyList_GET_SIZE(sublist);
-        sl_new = PyList_New(len);
+        len    = PyList_Size(sublist);
+        sl_new = PyList_New(0);
         if (sl_new == NULL) { // LCOV_BR_EXCL_LINE
             // LCOV_EXCL_START
             Py_DECREF(result);
@@ -176,17 +187,27 @@
             PyObject* item;
             PyObject* new_item;
 
-            item     = PyList_GET_ITEM(sublist, i);
+            item     = PyList_GetItemRef(sublist, i);
+            if (item == NULL) {
+                Py_DECREF(result);
+                return NULL;
+            }
             new_item = Py_BuildValue("(ON)", PyTuple_GET_ITEM(item, 0),
                                      value_transform(PyTuple_GET_ITEM(item, 1)));
+            Py_DECREF(item);
             if (new_item == NULL) { // LCOV_BR_EXCL_LINE
                 // LCOV_EXCL_START
                 Py_DECREF(result);
                 return NULL;
                 // LCOV_EXCL_STOP
             }
 
-            PyList_SET_ITEM(sl_new, i, new_item);
+            if (PyList_Append(sl_new, new_item) < 0) {
+                Py_DECREF(new_item);
+                Py_DECREF(result);
+                return NULL;
+            }
+            Py_DECREF(new_item);
         }
     }
 

diff --git a/pyobjc-core/Modules/objc/super-call.m b/pyobjc-core/Modules/objc/super-call.m
@@ -258,24 +258,36 @@
      *   are preferred over earlier items (those are metadata that
      *   was added later).
      */
-    for (i = 0; i < PyList_GET_SIZE(lst); i++) {
-        PyObject* entry   = PyList_GET_ITEM(lst, i);
+    Py_ssize_t len = PyList_Size(lst);
+    for (i = 0; i < len; i++) {
+        PyObject* entry   = PyList_GetItemRef(lst, i);
+        if (entry == NULL) {
+            goto error;
+        }
         PyObject* pyclass = PyTuple_GET_ITEM(entry, 0);
 
-        if (pyclass == NULL) // LCOV_BR_EXCL_LINE
+        if (pyclass == NULL) { // LCOV_BR_EXCL_LINE
+            Py_DECREF(entry); // LCOV_EXCL_LINE
             continue;        // LCOV_EXCL_LINE
+        }
         if (pyclass != Py_None
             && !PyType_IsSubtype((PyTypeObject*)search_class, (PyTypeObject*)pyclass)) {
+            Py_DECREF(entry);
             continue;
         }
 
         if (!special_class) {
             /* No match yet, use */
+            Py_CLEAR(special_class);
+            Py_CLEAR(result);
             special_class = pyclass;
             result        = PyTuple_GET_ITEM(entry, 1);
+            Py_INCREF(result);
+            Py_DECREF(entry);
 
         } else if (pyclass == Py_None) {
             /* Already have a match, Py_None is less specific */
+            Py_DECREF(entry);
             continue;
 
         } else if (PyType_IsSubtype((PyTypeObject*)special_class,
@@ -285,15 +297,22 @@
              * a more specific match or a similar match later in the
              * list.
              */
+            Py_CLEAR(result);
+            Py_CLEAR(special_class);
+            Py_INCREF(pyclass);
             special_class = pyclass;
             result        = PyTuple_GET_ITEM(entry, 1);
+            Py_INCREF(result);
+            Py_DECREF(entry);
         }
     }
     Py_XDECREF(search_class);
     if (!result)
         goto error;
 
-    return PyCapsule_GetPointer(result, "objc.__memblock__");
+     struct registry* rv = PyCapsule_GetPointer(result, "objc.__memblock__");
+     Py_DECREF(result);
+     return rv;
 
 error:
     return NULL;

diff --git a/pyobjc-core/PyObjCTest/test_metadata.py b/pyobjc-core/PyObjCTest/test_metadata.py
@@ -1550,7 +1550,7 @@ def testResult(self):
         ):
             v["hello":"world"] = 1
 
-        with self.assertRaisesRegex(TypeError, "New value must be a sequence"):
+        with self.assertRaisesRegex(TypeError, "object is not iterable"):
             v[1:3] = 42
 
         self.assertEqual(v.as_tuple(5), (1, 3, 5, 7, 11))