Skip to content

Commit c03e05c

Browse files
vstinnervstinner
authored
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.
1 parent 0124b5d commit c03e05c

File tree

6 files changed

+175
-29
lines changed

6 files changed

+175
-29
lines changed

‎Doc/c-api/refcounting.rst‎

Lines changed: 44 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,8 @@
77
Reference Counting
88
******************
99

10-
The macros in this section are used for managing reference counts of Python
11-
objects.
10+
The functions and macros in this section are used for managing reference counts
11+
of Python objects.
1212

1313

1414
.. c:function:: Py_ssize_t Py_REFCNT(PyObject *o)
@@ -129,6 +129,11 @@ objects.
129129
It is a good idea to use this macro whenever decrementing the reference
130130
count of an object that might be traversed during garbage collection.
131131
132+
.. versionchanged:: 3.12
133+
The macro argument is now only evaluated once. If the argument has side
134+
effects, these are no longer duplicated.
135+
136+
132137
.. c:function:: void Py_IncRef(PyObject *o)
133138
134139
Increment the reference count for object *o*. A function version of :c:func:`Py_XINCREF`.
@@ -139,3 +144,40 @@ objects.
139144
140145
Decrement the reference count for object *o*. A function version of :c:func:`Py_XDECREF`.
141146
It can be used for runtime dynamic embedding of Python.
147+
148+
149+
.. c:macro:: Py_SETREF(dst, src)
150+
151+
Macro safely decrementing the `dst` reference count and setting `dst` to
152+
`src`.
153+
154+
As in case of :c:func:`Py_CLEAR`, "the obvious" code can be deadly::
155+
156+
Py_DECREF(dst);
157+
dst = src;
158+
159+
The safe way is::
160+
161+
Py_SETREF(dst, src);
162+
163+
That arranges to set `dst` to `src` _before_ decrementing reference count of
164+
*dst* old value, so that any code triggered as a side-effect of `dst`
165+
getting torn down no longer believes `dst` points to a valid object.
166+
167+
.. versionadded:: 3.6
168+
169+
.. versionchanged:: 3.12
170+
The macro arguments are now only evaluated once. If an argument has side
171+
effects, these are no longer duplicated.
172+
173+
174+
.. c:macro:: Py_XSETREF(dst, src)
175+
176+
Variant of :c:macro:`Py_SETREF` macro that uses :c:func:`Py_XDECREF` instead
177+
of :c:func:`Py_DECREF`.
178+
179+
.. versionadded:: 3.6
180+
181+
.. versionchanged:: 3.12
182+
The macro arguments are now only evaluated once. If an argument has side
183+
effects, these are no longer duplicated.

‎Doc/whatsnew/3.12.rst‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -792,6 +792,11 @@ Porting to Python 3.12
792792
:class:`bytes` type is accepted for bytes strings.
793793
(Contributed by Victor Stinner in :gh:`98393`.)
794794

795+
* The :c:macro:`Py_CLEAR`, :c:macro:`Py_SETREF` and :c:macro:`Py_XSETREF`
796+
macros now only evaluate their argument once. If the argument has side
797+
effects, these side effects are no longer duplicated.
798+
(Contributed by Victor Stinner in :gh:`98724`.)
799+
795800
Deprecated
796801
----------
797802

‎Include/cpython/object.h‎

Lines changed: 24 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -305,37 +305,41 @@ _PyObject_GenericSetAttrWithDict(PyObject *, PyObject *,
305305

306306
PyAPI_FUNC(PyObject *) _PyObject_FunctionStr(PyObject *);
307307

308-
/* Safely decref `op` and set `op` to `op2`.
308+
/* Safely decref `dst` and set `dst` to `src`.
309309
*
310310
* As in case of Py_CLEAR "the obvious" code can be deadly:
311311
*
312-
* Py_DECREF(op);
313-
* op = op2;
312+
* Py_DECREF(dst);
313+
* dst = src;
314314
*
315315
* The safe way is:
316316
*
317-
* Py_SETREF(op, op2);
317+
* Py_SETREF(dst, src);
318318
*
319-
* That arranges to set `op` to `op2` _before_ decref'ing, so that any code
320-
* triggered as a side-effect of `op` getting torn down no longer believes
321-
* `op` points to a valid object.
319+
* That arranges to set `dst` to `src` _before_ decref'ing, so that any code
320+
* triggered as a side-effect of `dst` getting torn down no longer believes
321+
* `dst` points to a valid object.
322322
*
323-
* Py_XSETREF is a variant of Py_SETREF that uses Py_XDECREF instead of
324-
* Py_DECREF.
323+
* gh-98724: Use the _tmp_dst_ptr variable to evaluate the 'dst' macro argument
324+
* exactly once, to prevent the duplication of side effects in this macro.
325325
*/
326-
327-
#define Py_SETREF(op, op2) \
328-
do { \
329-
PyObject *_py_tmp = _PyObject_CAST(op); \
330-
(op) = (op2); \
331-
Py_DECREF(_py_tmp); \
326+
#define Py_SETREF(dst, src) \
327+
do { \
328+
PyObject **_tmp_dst_ptr = _Py_CAST(PyObject**, &(dst)); \
329+
PyObject *_tmp_dst = (*_tmp_dst_ptr); \
330+
*_tmp_dst_ptr = _PyObject_CAST(src); \
331+
Py_DECREF(_tmp_dst); \
332332
} while (0)
333333

334-
#define Py_XSETREF(op, op2) \
335-
do { \
336-
PyObject *_py_tmp = _PyObject_CAST(op); \
337-
(op) = (op2); \
338-
Py_XDECREF(_py_tmp); \
334+
/* Py_XSETREF() is a variant of Py_SETREF() that uses Py_XDECREF() instead of
335+
* Py_DECREF().
336+
*/
337+
#define Py_XSETREF(dst, src) \
338+
do { \
339+
PyObject **_tmp_dst_ptr = _Py_CAST(PyObject**, &(dst)); \
340+
PyObject *_tmp_dst = (*_tmp_dst_ptr); \
341+
*_tmp_dst_ptr = _PyObject_CAST(src); \
342+
Py_XDECREF(_tmp_dst); \
339343
} while (0)
340344

341345

‎Include/object.h‎

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -598,16 +598,21 @@ static inline void Py_DECREF(PyObject *op)
598598
* one of those can't cause problems -- but in part that relies on that
599599
* Python integers aren't currently weakly referencable. Best practice is
600600
* to use Py_CLEAR() even if you can't think of a reason for why you need to.
601+
*
602+
* gh-98724: Use the _py_tmp_ptr variable to evaluate the macro argument
603+
* exactly once, to prevent the duplication of side effects in this macro.
601604
*/
602-
#define Py_CLEAR(op) \
603-
do { \
604-
PyObject *_py_tmp = _PyObject_CAST(op); \
605-
if (_py_tmp != NULL) { \
606-
(op) = NULL; \
607-
Py_DECREF(_py_tmp); \
608-
} \
605+
#define Py_CLEAR(op) \
606+
do { \
607+
PyObject **_py_tmp_ptr = _Py_CAST(PyObject**, &(op)); \
608+
if (*_py_tmp_ptr != NULL) { \
609+
PyObject* _py_tmp = (*_py_tmp_ptr); \
610+
*_py_tmp_ptr = NULL; \
611+
Py_DECREF(_py_tmp); \
612+
} \
609613
} while (0)
610614

615+
611616
/* Function to use in case the object pointer can be NULL: */
612617
static inline void Py_XINCREF(PyObject *op)
613618
{

‎Misc/NEWS.d/next/C API/2022-11-04-16-13-35.gh-issue-98724.p0urWO.rst‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
The :c:macro:`Py_CLEAR`, :c:macro:`Py_SETREF` and :c:macro:`Py_XSETREF` macros
2+
now only evaluate their argument once. If the argument has side effects, these
3+
side effects are no longer duplicated. Patch by Victor Stinner.

‎Modules/_testcapimodule.c‎

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5138,6 +5138,91 @@ test_set_type_size(PyObject *self, PyObject *Py_UNUSED(ignored))
51385138
}
51395139

51405140

5141+
// Test Py_CLEAR() macro
5142+
static PyObject*
5143+
test_py_clear(PyObject *self, PyObject *Py_UNUSED(ignored))
5144+
{
5145+
// simple case with a variable
5146+
PyObject *obj = PyList_New(0);
5147+
if (obj == NULL) {
5148+
return NULL;
5149+
}
5150+
Py_CLEAR(obj);
5151+
assert(obj == NULL);
5152+
5153+
// gh-98724: complex case, Py_CLEAR() argument has a side effect
5154+
PyObject* array[1];
5155+
array[0] = PyList_New(0);
5156+
if (array[0] == NULL) {
5157+
return NULL;
5158+
}
5159+
5160+
PyObject **p = array;
5161+
Py_CLEAR(*p++);
5162+
assert(array[0] == NULL);
5163+
assert(p == array + 1);
5164+
5165+
Py_RETURN_NONE;
5166+
}
5167+
5168+
5169+
// Test Py_SETREF() and Py_XSETREF() macros, similar to test_py_clear()
5170+
static PyObject*
5171+
test_py_setref(PyObject *self, PyObject *Py_UNUSED(ignored))
5172+
{
5173+
// Py_SETREF() simple case with a variable
5174+
PyObject *obj = PyList_New(0);
5175+
if (obj == NULL) {
5176+
return NULL;
5177+
}
5178+
Py_SETREF(obj, NULL);
5179+
assert(obj == NULL);
5180+
5181+
// Py_XSETREF() simple case with a variable
5182+
PyObject *obj2 = PyList_New(0);
5183+
if (obj2 == NULL) {
5184+
return NULL;
5185+
}
5186+
Py_XSETREF(obj2, NULL);
5187+
assert(obj2 == NULL);
5188+
// test Py_XSETREF() when the argument is NULL
5189+
Py_XSETREF(obj2, NULL);
5190+
assert(obj2 == NULL);
5191+
5192+
// gh-98724: complex case, Py_SETREF() argument has a side effect
5193+
PyObject* array[1];
5194+
array[0] = PyList_New(0);
5195+
if (array[0] == NULL) {
5196+
return NULL;
5197+
}
5198+
5199+
PyObject **p = array;
5200+
Py_SETREF(*p++, NULL);
5201+
assert(array[0] == NULL);
5202+
assert(p == array + 1);
5203+
5204+
// gh-98724: complex case, Py_XSETREF() argument has a side effect
5205+
PyObject* array2[1];
5206+
array2[0] = PyList_New(0);
5207+
if (array2[0] == NULL) {
5208+
return NULL;
5209+
}
5210+
5211+
PyObject **p2 = array2;
5212+
Py_XSETREF(*p2++, NULL);
5213+
assert(array2[0] == NULL);
5214+
assert(p2 == array2 + 1);
5215+
5216+
// test Py_XSETREF() when the argument is NULL
5217+
p2 = array2;
5218+
Py_XSETREF(*p2++, NULL);
5219+
assert(array2[0] == NULL);
5220+
assert(p2 == array2 + 1);
5221+
5222+
Py_RETURN_NONE;
5223+
}
5224+
5225+
51415226
#define TEST_REFCOUNT() \
51425227
do { \
51435228
PyObject *obj = PyList_New(0); \
@@ -6311,6 +6396,8 @@ static PyMethodDef TestMethods[] = {
63116396
{"pynumber_tobase", pynumber_tobase, METH_VARARGS},
63126397
{"without_gc", without_gc, METH_O},
63136398
{"test_set_type_size", test_set_type_size, METH_NOARGS},
6399+
{"test_py_clear", test_py_clear, METH_NOARGS},
6400+
{"test_py_setref", test_py_setref, METH_NOARGS},
63146401
{"test_refcount_macros", test_refcount_macros, METH_NOARGS},
63156402
{"test_refcount_funcs", test_refcount_funcs, METH_NOARGS},
63166403
{"test_py_is_macros", test_py_is_macros, METH_NOARGS},

0 commit comments

Comments
 (0)