Skip to content

Commit

Permalink
gh-98724: Fix Py_CLEAR() macro side effects (#99100) (#99288)
Browse files Browse the repository at this point in the history
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.

(cherry picked from commit c03e05c)
  • Loading branch information
vstinner authored Nov 9, 2022
1 parent cf5dbb4 commit 1082890
Show file tree
Hide file tree
Showing 4 changed files with 126 additions and 27 deletions.
44 changes: 24 additions & 20 deletions Include/cpython/object.h
Original file line number Diff line number Diff line change
Expand Up @@ -309,37 +309,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)


Expand Down
19 changes: 12 additions & 7 deletions Include/object.h
Original file line number Diff line number Diff line change
Expand Up @@ -575,16 +575,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)
{
Expand Down
Original file line number Diff line number Diff line change
@@ -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.
87 changes: 87 additions & 0 deletions Modules/_testcapimodule.c
Original file line number Diff line number Diff line change
Expand Up @@ -5684,6 +5684,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); \
Expand Down Expand Up @@ -6475,6 +6560,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},
Expand Down

0 comments on commit 1082890

Please sign in to comment.