[Python-checkins] gh-98724: Fix Py_CLEAR() macro side effects (#99100)
vstinner
webhook-mailer at python.org
Wed Nov 9 08:07:06 EST 2022
https://github.com/python/cpython/commit/c03e05c2e72f3ea5e797389e7d1042eef85ad37a
commit: c03e05c2e72f3ea5e797389e7d1042eef85ad37a
branch: main
author: Victor Stinner <vstinner at python.org>
committer: vstinner <vstinner at python.org>
date: 2022-11-09T14:06:36+01:00
summary:
gh-98724: Fix Py_CLEAR() macro side effects (#99100)
The Py_CLEAR(), Py_SETREF() and Py_XSETREF() macros now only evaluate
their argument once. If an argument has side effects, these side
effects are no longer duplicated.
Add test_py_clear() and test_py_setref() unit tests to _testcapi.
files:
A Misc/NEWS.d/next/C API/2022-11-04-16-13-35.gh-issue-98724.p0urWO.rst
M Doc/c-api/refcounting.rst
M Doc/whatsnew/3.12.rst
M Include/cpython/object.h
M Include/object.h
M Modules/_testcapimodule.c
diff --git a/Doc/c-api/refcounting.rst b/Doc/c-api/refcounting.rst
index cd1f2ef70768..d8e9c2da6f3f 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 7ebfc5537c7a..37b9fb1ea4e8 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 fa0cfb244849..d07cb3be4c66 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 75624fe8c77a..a2ed0bd2349f 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 000000000000..2613b1792401
--- /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 0615c7352ca0..7b018b11e86f 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},
More information about the Python-checkins
mailing list