diff --git a/pyobjc-core/Modules/objc/block_support.m b/pyobjc-core/Modules/objc/block_support.m index 8c1cd41ee..b19cbc609 100644 --- a/pyobjc-core/Modules/objc/block_support.m +++ b/pyobjc-core/Modules/objc/block_support.m @@ -88,8 +88,7 @@ struct block_literal* src = (struct block_literal*)_src; PyObjC_BEGIN_WITH_GIL - dst->invoke_cleanup = src->invoke_cleanup; - Py_XINCREF(dst->invoke_cleanup); + dst->invoke_cleanup = Py_XNewRef(src->invoke_cleanup); PyObjC_END_WITH_GIL } diff --git a/pyobjc-core/Modules/objc/closure_pool.m b/pyobjc-core/Modules/objc/closure_pool.m index 24cf48b88..092952eef 100644 --- a/pyobjc-core/Modules/objc/closure_pool.m +++ b/pyobjc-core/Modules/objc/closure_pool.m @@ -3,6 +3,7 @@ * alive forever and we therefore don't have to return storage to the OS. * * These functions are only used when deploying to macOS 10.14 or earlier. + * */ #include "pyobjc.h" #include "closure_pool.h" @@ -18,6 +19,15 @@ struct freelist* _Nullable next; } freelist; +#if PY_VERSION_HEX >= 0x030d0000 +/* + * Mutex protecting *closure_freelist*. There is no need for + * minimizing the amount of code protected by the lock, allocating + * of new closures is not anywhere close to being on a fast path. + */ +static PyMutex freelist_mutex = {0}; +#endif + static freelist* closure_freelist = NULL; #ifdef MAP_JIT @@ -29,7 +39,7 @@ static int use_map_jit(void) { - static int cached_result = -1; + static _Atomic int cached_result = -1; if (cached_result == -1) { char buf[256]; @@ -93,23 +103,38 @@ return NULL; } PyObjC_Assert(codeloc, NULL); +#if PY_VERSION_HEX >= 0x030d0000 + PyMutex_Lock(&freelist_mutex); +#endif if (closure_freelist == NULL) { closure_freelist = allocate_block(); if (closure_freelist == NULL) { +#if PY_VERSION_HEX >= 0x030d0000 + PyMutex_Unlock(&freelist_mutex); +#endif return NULL; } } ffi_closure* result = (ffi_closure*)closure_freelist; closure_freelist = closure_freelist->next; *codeloc = (void*)result; +#if PY_VERSION_HEX >= 0x030d0000 + PyMutex_Unlock(&freelist_mutex); +#endif return result; } int PyObjC_ffi_closure_free(ffi_closure* cl) { +#if PY_VERSION_HEX >= 0x030d0000 + PyMutex_Lock(&freelist_mutex); +#endif ((freelist*)cl)->next = closure_freelist; closure_freelist = (freelist*)cl; +#if PY_VERSION_HEX >= 0x030d0000 + PyMutex_Unlock(&freelist_mutex); +#endif return 0; } diff --git a/pyobjc-core/Modules/objc/file_wrapper.m b/pyobjc-core/Modules/objc/file_wrapper.m index bdb771128..22d688b30 100644 --- a/pyobjc-core/Modules/objc/file_wrapper.m +++ b/pyobjc-core/Modules/objc/file_wrapper.m @@ -4,6 +4,10 @@ /* A basic wrapper for C's "FILE*" * that implements a usable API. + * + * NOTE: The locking using critical section is not very fine grained, + * but that shouldn't be a problem given how little FILE* objects + * are used in Objective-C APIs. */ static PyObject* FILE_Type; @@ -56,6 +60,8 @@ { struct file_object* self = (struct file_object*)_self; + Py_BEGIN_CRITICAL_SECTION(_self); + if (self->fp == NULL) { PyErr_SetString(PyExc_ValueError, "Closing closed file"); return NULL; @@ -67,6 +73,8 @@ self->fp = NULL; + Py_END_CRITICAL_SECTION(); + Py_INCREF(Py_None); return Py_None; } @@ -74,68 +82,88 @@ static PyObject* _Nullable file_flush(PyObject* _self) { struct file_object* self = (struct file_object*)_self; + PyObject* retval; int result; + Py_BEGIN_CRITICAL_SECTION(_self); + if (self->fp == NULL) { PyErr_SetString(PyExc_ValueError, "Using closed file"); - return NULL; - } - - result = fflush(self->fp); - if (result != 0) { - return PyErr_SetFromErrno(PyExc_OSError); + retval = NULL; + } else { + result = fflush(self->fp); + if (result != 0) { + retval = PyErr_SetFromErrno(PyExc_OSError); + } else { + Py_INCREF(Py_None); + retval = Py_None; + } } - - Py_INCREF(Py_None); - return Py_None; + Py_END_CRITICAL_SECTION(); + return retval; } static PyObject* _Nullable file_errors(PyObject* _self) { struct file_object* self = (struct file_object*)_self; int result; + PyObject* retval; + + Py_BEGIN_CRITICAL_SECTION(_self); if (self->fp == NULL) { PyErr_SetString(PyExc_ValueError, "Using closed file"); - return NULL; + retval = NULL; + } else { + result = ferror(self->fp); + retval = PyBool_FromLong(result); } - result = ferror(self->fp); + Py_END_CRITICAL_SECTION(); - return PyBool_FromLong(result); + return retval; } static PyObject* _Nullable file_at_eof(PyObject* _self) { struct file_object* self = (struct file_object*)_self; int result; + PyObject* retval; + + Py_BEGIN_CRITICAL_SECTION(_self); if (self->fp == NULL) { PyErr_SetString(PyExc_ValueError, "Using closed file"); - return NULL; + retval = NULL; + } else { + result = feof(self->fp); + retval = PyBool_FromLong(result); } + Py_END_CRITICAL_SECTION(); - result = feof(self->fp); - - return PyBool_FromLong(result); + return retval; } static PyObject* _Nullable file_tell(PyObject* _self) { struct file_object* self = (struct file_object*)_self; long offset; + PyObject* retval; + Py_BEGIN_CRITICAL_SECTION(_self); if (self->fp == NULL) { PyErr_SetString(PyExc_ValueError, "Using closed file"); - return NULL; - } - - offset = ftell(self->fp); - if (offset < 0) { - return PyErr_SetFromErrno(PyExc_OSError); + retval = NULL; + } else { + offset = ftell(self->fp); + if (offset < 0) { + retval = PyErr_SetFromErrno(PyExc_OSError); + } else { + retval = PyLong_FromLong(offset); + } } - - return PyLong_FromLong(offset); + Py_END_CRITICAL_SECTION(); + return retval; } static PyObject* _Nullable file_seek(PyObject* _self, PyObject* args, PyObject* kwds) @@ -146,39 +174,46 @@ Py_ssize_t offset; int whence; long result; + PyObject* retval; + Py_BEGIN_CRITICAL_SECTION(_self); if (self->fp == NULL) { PyErr_SetString(PyExc_ValueError, "Using closed file"); - return NULL; - } - - if (!PyArg_ParseTupleAndKeywords(args, kwds, "ni", keywords, &offset, &whence)) { - return NULL; - } - - result = fseek(self->fp, offset, whence); - if (result < 0) { - return PyErr_SetFromErrno(PyExc_OSError); + retval = NULL; + } else if (!PyArg_ParseTupleAndKeywords(args, kwds, "ni", keywords, &offset, &whence)) { + retval = NULL; + } else { + result = fseek(self->fp, offset, whence); + if (result < 0) { + retval = PyErr_SetFromErrno(PyExc_OSError); + } else { + Py_INCREF(Py_None); + retval = Py_None; + } } + Py_END_CRITICAL_SECTION(); - Py_INCREF(Py_None); - return Py_None; + return retval; } static PyObject* _Nullable file_fileno(PyObject* _self) { struct file_object* self = (struct file_object*)_self; int fd; + PyObject* retval; + Py_BEGIN_CRITICAL_SECTION(_self); if (self->fp == NULL) { PyErr_SetString(PyExc_ValueError, "Using closed file"); - return NULL; - } - - fd = fileno(self->fp); - /* According to the manpage this function cannot fail */ + retval = NULL; + } else { + fd = fileno(self->fp); + /* According to the manpage this function cannot fail */ - return PyLong_FromLong(fd); + retval = PyLong_FromLong(fd); + } + Py_END_CRITICAL_SECTION(); + return retval; } static PyObject* _Nullable file_write(PyObject* _self, PyObject* args, PyObject* kwds) @@ -189,18 +224,22 @@ void* buffer; Py_ssize_t buffer_size; size_t result; - - if (self->fp == NULL) { - PyErr_SetString(PyExc_ValueError, "Using closed file"); - return NULL; - } + PyObject* retval; if (!PyArg_ParseTupleAndKeywords(args, kwds, "y#", keywords, &buffer, &buffer_size)) { return NULL; } - result = fwrite(buffer, 1, buffer_size, self->fp); - return Py_BuildValue("k", (unsigned long)result); + Py_BEGIN_CRITICAL_SECTION(_self); + if (self->fp == NULL) { + PyErr_SetString(PyExc_ValueError, "Using closed file"); + retval = NULL; + } else { + result = fwrite(buffer, 1, buffer_size, self->fp); + retval = PyLong_FromSize_t(result); + } + Py_END_CRITICAL_SECTION(); + return retval; } static PyObject* _Nullable file_readline(PyObject* _self) @@ -208,18 +247,22 @@ struct file_object* self = (struct file_object*)_self; char buffer[2048]; char* result; + PyObject* retval; + Py_BEGIN_CRITICAL_SECTION(_self); if (self->fp == NULL) { PyErr_SetString(PyExc_ValueError, "Using closed file"); return NULL; - } - - result = fgets(buffer, 2048, self->fp); - if (result == NULL) { - return PyBytes_FromStringAndSize("", 0); } else { - return PyBytes_FromString(result); + result = fgets(buffer, 2048, self->fp); + if (result == NULL) { + retval = PyBytes_FromStringAndSize("", 0); + } else { + retval = PyBytes_FromString(result); + } } + Py_END_CRITICAL_SECTION(); + return retval; } static PyObject* _Nullable file_read(PyObject* _self, PyObject* args, PyObject* kwds) @@ -231,23 +274,22 @@ Py_ssize_t buffer_size; size_t result; + Py_BEGIN_CRITICAL_SECTION(_self); + if (self->fp == NULL) { PyErr_SetString(PyExc_ValueError, "Using closed file"); - return NULL; - } + buffer = NULL; + } else if (!PyArg_ParseTupleAndKeywords(args, kwds, "n", keywords, &buffer_size)) { + buffer = NULL; + } else { + buffer = PyBytes_FromStringAndSize(NULL, buffer_size); + if (buffer != NULL) { + result = fread(PyBytes_AsString(buffer), 1, buffer_size, self->fp); - if (!PyArg_ParseTupleAndKeywords(args, kwds, "n", keywords, &buffer_size)) { - return NULL; + _PyBytes_Resize(&buffer, result); + } } - - buffer = PyBytes_FromStringAndSize(NULL, buffer_size); - if (buffer == NULL) { - return NULL; - } - - result = fread(PyBytes_AsString(buffer), 1, buffer_size, self->fp); - - _PyBytes_Resize(&buffer, result); + Py_END_CRITICAL_SECTION(); return buffer; } diff --git a/pyobjc-core/Modules/objc/helpers-foundation-nsdecimal.m b/pyobjc-core/Modules/objc/helpers-foundation-nsdecimal.m index a86552842..7716afbc9 100644 --- a/pyobjc-core/Modules/objc/helpers-foundation-nsdecimal.m +++ b/pyobjc-core/Modules/objc/helpers-foundation-nsdecimal.m @@ -658,7 +658,7 @@ static int decimal_init(PyObject* self, PyObject* _Nullable args, if (left == NULL) // LCOV_BR_EXCL_LINE goto error; // LCOV_EXCL_LINE - args = Py_BuildValue("(O)", *l); /* XXX: really? */ + args = PyTuple_Pack(1, *l); if (args == NULL) // LCOV_BR_EXCL_LINE goto error; // LCOV_EXCL_LINE @@ -679,7 +679,7 @@ static int decimal_init(PyObject* self, PyObject* _Nullable args, if (right == NULL) // LCOV_BR_EXCL_LINE goto error; // LCOV_EXCL_LINE - args = Py_BuildValue("(O)", *r); /* XXX: really? */ + args = PyTuple_Pack(1, *r); if (args == NULL) // LCOV_BR_EXCL_LINE goto error; // LCOV_EXCL_LINE diff --git a/pyobjc-core/Modules/objc/objc-runtime-compat.m b/pyobjc-core/Modules/objc/objc-runtime-compat.m index 3b09276ba..fdd07d1e9 100644 --- a/pyobjc-core/Modules/objc/objc-runtime-compat.m +++ b/pyobjc-core/Modules/objc/objc-runtime-compat.m @@ -1,10 +1,3 @@ -/* - * Objective-C runtime 2.0 compatibility for MacOS X 10.4 and earlier. - * - * This code works by poking into the ObjC runtime, which means loads of - * warnings on 10.5+ ;-) - */ - #define PYOBJC_COMPAT_IMPL #include "pyobjc.h" diff --git a/pyobjc-core/Modules/objc/options.m b/pyobjc-core/Modules/objc/options.m index 9e7d7c7ef..4c75d4253 100644 --- a/pyobjc-core/Modules/objc/options.m +++ b/pyobjc-core/Modules/objc/options.m @@ -31,7 +31,7 @@ static PyObject* NAME##_get(PyObject* s __attribute__((__unused__)), \ void* c __attribute__((__unused__))) \ { \ - return Py_BuildValue("n", VAR); \ + return PyLong_FromSsize_t(VAR); \ } \ \ static int NAME##_set(PyObject* s __attribute__((__unused__)), PyObject* newVal, \ @@ -55,7 +55,7 @@ static PyObject* NAME##_get(PyObject* s __attribute__((__unused__)), \ void* c __attribute__((__unused__))) \ { \ - return Py_BuildValue("i", VAR); \ + return PyLong_FromLong(VAR); \ } \ \ static int NAME##_set(PyObject* s __attribute__((__unused__)), PyObject* newVal, \ diff --git a/pyobjc-core/Modules/objc/pyobjc-compat.h b/pyobjc-core/Modules/objc/pyobjc-compat.h index f162317c3..07b7d86d5 100644 --- a/pyobjc-core/Modules/objc/pyobjc-compat.h +++ b/pyobjc-core/Modules/objc/pyobjc-compat.h @@ -243,9 +243,11 @@ extern PyObject* _Nullable PyObjC_get_tp_dict(PyTypeObject* _Nonnull tp); #if PY_VERSION_HEX < 0x030d0000 #define Py_BEGIN_CRITICAL_SECTION(value) { #define Py_END_CRITICAL_SECTION() } +#define Py_EXIT_CRITICAL_SECTION() ((void)0) #define Py_BEGIN_CRITICAL_SECTION2(value1, value2) { #define Py_END_CRITICAL_SECTION2() } +#define Py_EXIT_CRITICAL_SECTION2() ((void)0) static inline int PyDict_GetItemRef(PyObject *p, PyObject *key, PyObject * _Nonnull* _Nullable result) @@ -269,6 +271,10 @@ static inline PyObject* _Nullable PyList_GetItemRef(PyObject* l, Py_ssize_t i) Py_XINCREF(result); return result; } + +#else +#define Py_EXIT_CRITICAL_SECTION() PyCriticalSection_End(&_py_cs) +#define Py_EXIT_CRITICAL_SECTION2() PyCriticalSection_End2(&_py_cs2) #endif diff --git a/pyobjc-core/Modules/objc/registry.m b/pyobjc-core/Modules/objc/registry.m index 697458621..31a1ece02 100644 --- a/pyobjc-core/Modules/objc/registry.m +++ b/pyobjc-core/Modules/objc/registry.m @@ -26,15 +26,34 @@ case -1: return -1; case 0: - sublist = PyList_New(0); - if (sublist == NULL) { // LCOV_BR_EXCL_LINE - return -1; // LCOV_EXCL_LINE + result = 0; +#ifdef Py_GIL_DISABLED + /* For the free threaded build add the new sublist in + * a critical section to avoid race conditions for this. + */ + Py_BEGIN_CRITICAL_SECTION(registry); + + switch (PyDict_GetItemRef(registry, selector, &sublist)) { + case -1: + result = -1; + break; + case 0: +#endif /* Py_GIL_DISABLED */ + sublist = PyList_New(0); + if (sublist == NULL) { + result = -1; + } else { + result = PyDict_SetItem(registry, selector, sublist); + } +#ifdef Py_GIL_DISABLED + /* case 1: pass */ } - result = PyDict_SetItem(registry, selector, sublist); - if (result == -1) { // LCOV_BR_EXCL_LINE - return -1; // LCOV_EXCL_LINE + Py_END_CRITICAL_SECTION(); +#endif /* Py_GIL_DISABLED */ + if (result != 0) { + return result; } - /* default: fallthrough */ + /* case 1: fallthrough */ } if (!PyObjC_UpdatingMetaData) { @@ -45,11 +64,14 @@ * Check if there is a registration for *class_name* in * *sublist*, if so replace that registration. */ + + Py_BEGIN_CRITICAL_SECTION(sublist); 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) { Py_DECREF(sublist); + Py_EXIT_CRITICAL_SECTION(); return -1; } @@ -60,31 +82,45 @@ if (r == -1) { // LCOV_BR_EXCL_LINE Py_DECREF(item); // LCOV_EXCL_LINE Py_DECREF(sublist); // LCOV_EXCL_LINE + Py_EXIT_CRITICAL_SECTION(); return -1; // LCOV_EXCL_LINE } if (r) { - Py_DECREF(PyTuple_GET_ITEM(item, 1)); - PyTuple_SET_ITEM(item, 1, value); - Py_INCREF(value); + PyObject* new_item = PyTuple_Pack(2, + PyTuple_GET_ITEM(item, 0), + value); + + r = PyList_SetItem(sublist, i, new_item); Py_DECREF(item); Py_DECREF(sublist); - return 0; + Py_EXIT_CRITICAL_SECTION(); + return r; } } - PyObject* item = Py_BuildValue("(OO)", class_name, value); + PyObject* item = PyTuple_Pack(2, class_name, value); if (item == NULL) { // LCOV_BR_EXCL_LINE Py_DECREF(sublist); // LCOV_EXCL_LINE + Py_EXIT_CRITICAL_SECTION(); return -1; // LCOV_EXCL_LINE } result = PyList_Append(sublist, item); Py_DECREF(item); Py_DECREF(sublist); + + Py_END_CRITICAL_SECTION(); return result; } PyObject* _Nullable PyObjC_FindInRegistry(PyObject* registry, Class cls, SEL selector) { + /* + * This function does not need to critical sections to access *registry* + * because the function itself doesn't change the datastructure, and any + * changes make concurrently are fine because this function does not use + * borrowed references and the found sublist can only grow (e.g. we might + * at worst miss a newly added item on a race condition). + */ Py_ssize_t i; Py_ssize_t len; PyObject* cur; @@ -162,6 +198,14 @@ PyObject* sublist; Py_ssize_t pos = 0; + /* + * This function locks the registry to avoid problems with concurrent + * modification of the the dict, as documented the PyDict_Next API + * does not lock the dict itself. + */ + + Py_BEGIN_CRITICAL_SECTION(registry); + while (PyDict_Next(registry, &pos, &key, &sublist)) { Py_ssize_t i, len; PyObject* sl_new; @@ -171,6 +215,7 @@ // LCOV_EXCL_START PyErr_SetString(PyObjCExc_InternalError, "sublist of registry is not a list"); Py_DECREF(result); + Py_EXIT_CRITICAL_SECTION(); return NULL; // LCOV_EXCL_STOP } @@ -181,6 +226,7 @@ if (sl_new == NULL) { // LCOV_BR_EXCL_LINE // LCOV_EXCL_START Py_DECREF(result); + Py_EXIT_CRITICAL_SECTION(); return NULL; // LCOV_EXCL_STOP } @@ -188,6 +234,7 @@ // LCOV_EXCL_START Py_DECREF(sl_new); Py_DECREF(result); + Py_EXIT_CRITICAL_SECTION(); return NULL; // LCOV_EXCL_STOP } @@ -196,18 +243,27 @@ for (i = 0; i < len; i++) { PyObject* item; PyObject* new_item; + PyObject* new_value; item = PyList_GetItemRef(sublist, i); if (item == NULL) { Py_DECREF(result); + Py_EXIT_CRITICAL_SECTION(); + return NULL; + } + new_value = value_transform(PyTuple_GET_ITEM(item, 1)); + if (new_value == NULL) { + Py_DECREF(result); + Py_EXIT_CRITICAL_SECTION(); return NULL; } - new_item = Py_BuildValue("(ON)", PyTuple_GET_ITEM(item, 0), - value_transform(PyTuple_GET_ITEM(item, 1))); + new_item = PyTuple_Pack(2, PyTuple_GET_ITEM(item, 0), new_value); + Py_DECREF(new_value); Py_DECREF(item); if (new_item == NULL) { // LCOV_BR_EXCL_LINE // LCOV_EXCL_START Py_DECREF(result); + Py_EXIT_CRITICAL_SECTION(); return NULL; // LCOV_EXCL_STOP } @@ -215,11 +271,13 @@ if (PyList_Append(sl_new, new_item) < 0) { Py_DECREF(new_item); Py_DECREF(result); + Py_EXIT_CRITICAL_SECTION(); return NULL; } Py_DECREF(new_item); } } + Py_END_CRITICAL_SECTION(); return result; } diff --git a/pyobjc-core/Modules/objc/struct-wrapper.m b/pyobjc-core/Modules/objc/struct-wrapper.m index 7fe5b0445..acdfca931 100644 --- a/pyobjc-core/Modules/objc/struct-wrapper.m +++ b/pyobjc-core/Modules/objc/struct-wrapper.m @@ -239,7 +239,7 @@ PyTuple_SET_ITEM(values, i, v); } - result = Py_BuildValue("(OO)", Py_TYPE(self), values); + result = PyTuple_Pack(2, Py_TYPE(self), values); Py_DECREF(values); return result; }