diff options
| -rw-r--r-- | Doc/c-api/refcounting.rst | 46 | ||||
| -rw-r--r-- | Doc/whatsnew/3.12.rst | 5 | ||||
| -rw-r--r-- | Include/cpython/object.h | 44 | ||||
| -rw-r--r-- | Include/object.h | 19 | ||||
| -rw-r--r-- | Misc/NEWS.d/next/C API/2022-11-04-16-13-35.gh-issue-98724.p0urWO.rst | 3 | ||||
| -rw-r--r-- | Modules/_testcapimodule.c | 87 |
6 files changed, 175 insertions, 29 deletions
diff --git a/Doc/c-api/refcounting.rst b/Doc/c-api/refcounting.rst index cd1f2ef..d8e9c2d 100644 --- a/Doc/c-api/refcounting.rst +++ b/Doc/c-api/refcounting.rst @@ -7,8 +7,8 @@ Reference Counting ****************** -The macros in this section are used for managing reference counts of Python -objects. +The functions and macros in this section are used for managing reference counts +of Python objects. .. c:function:: Py_ssize_t Py_REFCNT(PyObject *o) @@ -129,6 +129,11 @@ objects. It is a good idea to use this macro whenever decrementing the reference count of an object that might be traversed during garbage collection. + .. versionchanged:: 3.12 + The macro argument is now only evaluated once. If the argument has side + effects, these are no longer duplicated. + + .. c:function:: void Py_IncRef(PyObject *o) Increment the reference count for object *o*. A function version of :c:func:`Py_XINCREF`. @@ -139,3 +144,40 @@ objects. Decrement the reference count for object *o*. A function version of :c:func:`Py_XDECREF`. It can be used for runtime dynamic embedding of Python. + + +.. c:macro:: Py_SETREF(dst, src) + + Macro safely decrementing the `dst` reference count and setting `dst` to + `src`. + + As in case of :c:func:`Py_CLEAR`, "the obvious" code can be deadly:: + + Py_DECREF(dst); + dst = src; + + The safe way is:: + + Py_SETREF(dst, src); + + That arranges to set `dst` to `src` _before_ decrementing reference count of + *dst* old value, so that any code triggered as a side-effect of `dst` + getting torn down no longer believes `dst` points to a valid object. + + .. versionadded:: 3.6 + + .. versionchanged:: 3.12 + The macro arguments are now only evaluated once. If an argument has side + effects, these are no longer duplicated. + + +.. c:macro:: Py_XSETREF(dst, src) + + Variant of :c:macro:`Py_SETREF` macro that uses :c:func:`Py_XDECREF` instead + of :c:func:`Py_DECREF`. + + .. versionadded:: 3.6 + + .. versionchanged:: 3.12 + The macro arguments are now only evaluated once. If an argument has side + effects, these are no longer duplicated. diff --git a/Doc/whatsnew/3.12.rst b/Doc/whatsnew/3.12.rst index 7ebfc55..37b9fb1 100644 --- a/Doc/whatsnew/3.12.rst +++ b/Doc/whatsnew/3.12.rst @@ -792,6 +792,11 @@ Porting to Python 3.12 :class:`bytes` type is accepted for bytes strings. (Contributed by Victor Stinner in :gh:`98393`.) +* The :c:macro:`Py_CLEAR`, :c:macro:`Py_SETREF` and :c:macro:`Py_XSETREF` + macros now only evaluate their argument once. If the argument has side + effects, these side effects are no longer duplicated. + (Contributed by Victor Stinner in :gh:`98724`.) + Deprecated ---------- diff --git a/Include/cpython/object.h b/Include/cpython/object.h index fa0cfb2..d07cb3b 100644 --- a/Include/cpython/object.h +++ b/Include/cpython/object.h @@ -305,37 +305,41 @@ _PyObject_GenericSetAttrWithDict(PyObject *, PyObject *, PyAPI_FUNC(PyObject *) _PyObject_FunctionStr(PyObject *); -/* Safely decref `op` and set `op` to `op2`. +/* Safely decref `dst` and set `dst` to `src`. * * As in case of Py_CLEAR "the obvious" code can be deadly: * - * Py_DECREF(op); - * op = op2; + * Py_DECREF(dst); + * dst = src; * * The safe way is: * - * Py_SETREF(op, op2); + * Py_SETREF(dst, src); * - * That arranges to set `op` to `op2` _before_ decref'ing, so that any code - * triggered as a side-effect of `op` getting torn down no longer believes - * `op` points to a valid object. + * That arranges to set `dst` to `src` _before_ decref'ing, so that any code + * triggered as a side-effect of `dst` getting torn down no longer believes + * `dst` points to a valid object. * - * Py_XSETREF is a variant of Py_SETREF that uses Py_XDECREF instead of - * Py_DECREF. + * gh-98724: Use the _tmp_dst_ptr variable to evaluate the 'dst' macro argument + * exactly once, to prevent the duplication of side effects in this macro. */ - -#define Py_SETREF(op, op2) \ - do { \ - PyObject *_py_tmp = _PyObject_CAST(op); \ - (op) = (op2); \ - Py_DECREF(_py_tmp); \ +#define Py_SETREF(dst, src) \ + do { \ + PyObject **_tmp_dst_ptr = _Py_CAST(PyObject**, &(dst)); \ + PyObject *_tmp_dst = (*_tmp_dst_ptr); \ + *_tmp_dst_ptr = _PyObject_CAST(src); \ + Py_DECREF(_tmp_dst); \ } while (0) -#define Py_XSETREF(op, op2) \ - do { \ - PyObject *_py_tmp = _PyObject_CAST(op); \ - (op) = (op2); \ - Py_XDECREF(_py_tmp); \ +/* Py_XSETREF() is a variant of Py_SETREF() that uses Py_XDECREF() instead of + * Py_DECREF(). + */ +#define Py_XSETREF(dst, src) \ + do { \ + PyObject **_tmp_dst_ptr = _Py_CAST(PyObject**, &(dst)); \ + PyObject *_tmp_dst = (*_tmp_dst_ptr); \ + *_tmp_dst_ptr = _PyObject_CAST(src); \ + Py_XDECREF(_tmp_dst); \ } while (0) diff --git a/Include/object.h b/Include/object.h index 75624fe..a2ed0bd 100644 --- a/Include/object.h +++ b/Include/object.h @@ -598,16 +598,21 @@ static inline void Py_DECREF(PyObject *op) * one of those can't cause problems -- but in part that relies on that * Python integers aren't currently weakly referencable. Best practice is * to use Py_CLEAR() even if you can't think of a reason for why you need to. + * + * gh-98724: Use the _py_tmp_ptr variable to evaluate the macro argument + * exactly once, to prevent the duplication of side effects in this macro. */ -#define Py_CLEAR(op) \ - do { \ - PyObject *_py_tmp = _PyObject_CAST(op); \ - if (_py_tmp != NULL) { \ - (op) = NULL; \ - Py_DECREF(_py_tmp); \ - } \ +#define Py_CLEAR(op) \ + do { \ + PyObject **_py_tmp_ptr = _Py_CAST(PyObject**, &(op)); \ + if (*_py_tmp_ptr != NULL) { \ + PyObject* _py_tmp = (*_py_tmp_ptr); \ + *_py_tmp_ptr = NULL; \ + Py_DECREF(_py_tmp); \ + } \ } while (0) + /* Function to use in case the object pointer can be NULL: */ static inline void Py_XINCREF(PyObject *op) { diff --git a/Misc/NEWS.d/next/C API/2022-11-04-16-13-35.gh-issue-98724.p0urWO.rst b/Misc/NEWS.d/next/C API/2022-11-04-16-13-35.gh-issue-98724.p0urWO.rst new file mode 100644 index 0000000..2613b17 --- /dev/null +++ b/Misc/NEWS.d/next/C API/2022-11-04-16-13-35.gh-issue-98724.p0urWO.rst @@ -0,0 +1,3 @@ +The :c:macro:`Py_CLEAR`, :c:macro:`Py_SETREF` and :c:macro:`Py_XSETREF` macros +now only evaluate their argument once. If the argument has side effects, these +side effects are no longer duplicated. Patch by Victor Stinner. diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index 0615c73..7b018b1 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -5138,6 +5138,91 @@ test_set_type_size(PyObject *self, PyObject *Py_UNUSED(ignored)) } +// Test Py_CLEAR() macro +static PyObject* +test_py_clear(PyObject *self, PyObject *Py_UNUSED(ignored)) +{ + // simple case with a variable + PyObject *obj = PyList_New(0); + if (obj == NULL) { + return NULL; + } + Py_CLEAR(obj); + assert(obj == NULL); + + // gh-98724: complex case, Py_CLEAR() argument has a side effect + PyObject* array[1]; + array[0] = PyList_New(0); + if (array[0] == NULL) { + return NULL; + } + + PyObject **p = array; + Py_CLEAR(*p++); + assert(array[0] == NULL); + assert(p == array + 1); + + Py_RETURN_NONE; +} + + +// Test Py_SETREF() and Py_XSETREF() macros, similar to test_py_clear() +static PyObject* +test_py_setref(PyObject *self, PyObject *Py_UNUSED(ignored)) +{ + // Py_SETREF() simple case with a variable + PyObject *obj = PyList_New(0); + if (obj == NULL) { + return NULL; + } + Py_SETREF(obj, NULL); + assert(obj == NULL); + + // Py_XSETREF() simple case with a variable + PyObject *obj2 = PyList_New(0); + if (obj2 == NULL) { + return NULL; + } + Py_XSETREF(obj2, NULL); + assert(obj2 == NULL); + // test Py_XSETREF() when the argument is NULL + Py_XSETREF(obj2, NULL); + assert(obj2 == NULL); + + // gh-98724: complex case, Py_SETREF() argument has a side effect + PyObject* array[1]; + array[0] = PyList_New(0); + if (array[0] == NULL) { + return NULL; + } + + PyObject **p = array; + Py_SETREF(*p++, NULL); + assert(array[0] == NULL); + assert(p == array + 1); + + // gh-98724: complex case, Py_XSETREF() argument has a side effect + PyObject* array2[1]; + array2[0] = PyList_New(0); + if (array2[0] == NULL) { + return NULL; + } + + PyObject **p2 = array2; + Py_XSETREF(*p2++, NULL); + assert(array2[0] == NULL); + assert(p2 == array2 + 1); + + // test Py_XSETREF() when the argument is NULL + p2 = array2; + Py_XSETREF(*p2++, NULL); + assert(array2[0] == NULL); + assert(p2 == array2 + 1); + + Py_RETURN_NONE; +} + + #define TEST_REFCOUNT() \ do { \ PyObject *obj = PyList_New(0); \ @@ -6311,6 +6396,8 @@ static PyMethodDef TestMethods[] = { {"pynumber_tobase", pynumber_tobase, METH_VARARGS}, {"without_gc", without_gc, METH_O}, {"test_set_type_size", test_set_type_size, METH_NOARGS}, + {"test_py_clear", test_py_clear, METH_NOARGS}, + {"test_py_setref", test_py_setref, METH_NOARGS}, {"test_refcount_macros", test_refcount_macros, METH_NOARGS}, {"test_refcount_funcs", test_refcount_funcs, METH_NOARGS}, {"test_py_is_macros", test_py_is_macros, METH_NOARGS}, |