Skip to content

Commit f528d13

Browse files
vstinnerngoldbaum
andcommitted
gh-161185: Fix strict aliasing in Py_SETREF() macros family
Py_SETREF(), Py_XSETREF() and Py_CLEAR() now use memcpy() to set _tmp_old_dst/_tmp_old_op to fix a strict aliasing issue. Py_SETREF(), Py_XSETREF() and Py_CLEAR() now also cast _tmp_old_dst/_tmp_old_op to PyObject* when calling Py_DECREF()/Py_XDECREF() to avoid compiler warnings with the limited C API. Co-Authored-by: Nathan Goldbaum <nathan.goldbaum@gmail.com>
1 parent bb9d26d commit f528d13

3 files changed

Lines changed: 103 additions & 9 deletions

File tree

‎Include/refcount.h‎

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -489,7 +489,7 @@ static inline Py_ALWAYS_INLINE void Py_DECREF(PyObject *op)
489489
_Py_TYPEOF(op) _tmp_old_op = (*_tmp_op_ptr); \
490490
if (_tmp_old_op != _Py_NULL) { \
491491
*_tmp_op_ptr = _Py_NULL; \
492-
Py_DECREF(_tmp_old_op); \
492+
Py_DECREF(_PyObject_CAST(_tmp_old_op)); \
493493
} \
494494
} while (0)
495495
#elif defined(__cplusplus) && (__cplusplus >= 201103L || _MSVC_LANG >= 201103L)
@@ -499,14 +499,15 @@ static inline Py_ALWAYS_INLINE void Py_DECREF(PyObject *op)
499499
auto _tmp_old_op = (*_tmp_op_ptr); \
500500
if (_tmp_old_op != _Py_NULL) { \
501501
*_tmp_op_ptr = _Py_NULL; \
502-
Py_DECREF(_tmp_old_op); \
502+
Py_DECREF(_PyObject_CAST(_tmp_old_op)); \
503503
} \
504504
} while (0)
505505
#else
506506
#define Py_CLEAR(op) \
507507
do { \
508508
PyObject **_tmp_op_ptr = _Py_CAST(PyObject**, &(op)); \
509-
PyObject *_tmp_old_op = (*_tmp_op_ptr); \
509+
PyObject *_tmp_old_op; \
510+
memcpy(&_tmp_old_op, _tmp_op_ptr, sizeof(PyObject*)); \
510511
if (_tmp_old_op != _Py_NULL) { \
511512
PyObject *_null_ptr = _Py_NULL; \
512513
memcpy(_tmp_op_ptr, &_null_ptr, sizeof(PyObject*)); \
@@ -545,21 +546,22 @@ static inline Py_ALWAYS_INLINE void Py_DECREF(PyObject *op)
545546
_Py_TYPEOF(&(dst)) _tmp_dst_ptr = &(dst); \
546547
_Py_TYPEOF(dst) _tmp_old_dst = (*_tmp_dst_ptr); \
547548
*_tmp_dst_ptr = (src); \
548-
Py_DECREF(_tmp_old_dst); \
549+
Py_DECREF(_PyObject_CAST(_tmp_old_dst)); \
549550
} while (0)
550551
#elif defined(__cplusplus) && (__cplusplus >= 201103L || _MSVC_LANG >= 201103L)
551552
#define Py_SETREF(dst, src) \
552553
do { \
553554
auto _tmp_dst_ptr = &(dst); \
554555
auto _tmp_old_dst = (*_tmp_dst_ptr); \
555556
*_tmp_dst_ptr = (src); \
556-
Py_DECREF(_tmp_old_dst); \
557+
Py_DECREF(_PyObject_CAST(_tmp_old_dst)); \
557558
} while (0)
558559
#else
559560
#define Py_SETREF(dst, src) \
560561
do { \
561562
PyObject **_tmp_dst_ptr = _Py_CAST(PyObject**, &(dst)); \
562-
PyObject *_tmp_old_dst = (*_tmp_dst_ptr); \
563+
PyObject *_tmp_old_dst; \
564+
memcpy(&_tmp_old_dst, _tmp_dst_ptr, sizeof(PyObject*)); \
563565
PyObject *_tmp_src = _PyObject_CAST(src); \
564566
memcpy(_tmp_dst_ptr, &_tmp_src, sizeof(PyObject*)); \
565567
Py_DECREF(_tmp_old_dst); \
@@ -575,21 +577,22 @@ static inline Py_ALWAYS_INLINE void Py_DECREF(PyObject *op)
575577
_Py_TYPEOF(&(dst)) _tmp_dst_ptr = &(dst); \
576578
_Py_TYPEOF(dst) _tmp_old_dst = (*_tmp_dst_ptr); \
577579
*_tmp_dst_ptr = (src); \
578-
Py_XDECREF(_tmp_old_dst); \
580+
Py_XDECREF(_PyObject_CAST(_tmp_old_dst)); \
579581
} while (0)
580582
#elif defined(__cplusplus) && (__cplusplus >= 201103L || _MSVC_LANG >= 201103L)
581583
#define Py_XSETREF(dst, src) \
582584
do { \
583585
auto _tmp_dst_ptr = &(dst); \
584586
auto _tmp_old_dst = (*_tmp_dst_ptr); \
585587
*_tmp_dst_ptr = (src); \
586-
Py_XDECREF(_tmp_old_dst); \
588+
Py_XDECREF(_PyObject_CAST(_tmp_old_dst)); \
587589
} while (0)
588590
#else
589591
#define Py_XSETREF(dst, src) \
590592
do { \
591593
PyObject **_tmp_dst_ptr = _Py_CAST(PyObject**, &(dst)); \
592-
PyObject *_tmp_old_dst = (*_tmp_dst_ptr); \
594+
PyObject *_tmp_old_dst; \
595+
memcpy(&_tmp_old_dst, _tmp_dst_ptr, sizeof(PyObject*)); \
593596
PyObject *_tmp_src = _PyObject_CAST(src); \
594597
memcpy(_tmp_dst_ptr, &_tmp_src, sizeof(PyObject*)); \
595598
Py_XDECREF(_tmp_old_dst); \

‎Lib/test/test_cext/extension.c‎

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -140,6 +140,91 @@ test_macros(PyObject *Py_UNUSED(module), PyObject *Py_UNUSED(args))
140140
}
141141

142142

143+
// sizeof(PyObject) in PySlot below is incompatible with opaque PyObject
144+
#ifndef Py_TARGET_ABI3T
145+
Py_NO_INLINE static void
146+
pyclear_strict_aliasing(PyTypeObject **a, PyTypeObject **b)
147+
{
148+
Py_CLEAR(*a);
149+
*b = NULL;
150+
}
151+
152+
153+
Py_NO_INLINE static void
154+
setref_strict_aliasing(PyTypeObject **a, PyTypeObject **b,
155+
PyTypeObject *first, PyTypeObject *second)
156+
{
157+
*a = first;
158+
Py_SETREF(*b, second);
159+
}
160+
161+
162+
Py_NO_INLINE static void
163+
xsetref_strict_aliasing(PyTypeObject **a, PyTypeObject **b,
164+
PyTypeObject *first, PyTypeObject *second)
165+
{
166+
*a = first;
167+
Py_XSETREF(*b, second);
168+
}
169+
170+
171+
// gh-161185: Test that Py_SETREF(), Py_XSETREF() and Py_CLEAR() macros
172+
// have no strict aliasing issue
173+
static PyObject *
174+
test_setref_strict_aliasing(PyObject *Py_UNUSED(module), PyObject *Py_UNUSED(args))
175+
{
176+
PySlot slots[] = {
177+
PySlot_PTR_STATIC(Py_tp_name, STR(MODULE_NAME) ".setref_type"),
178+
PySlot_PTR_STATIC(Py_tp_basicsize, sizeof(PyObject)),
179+
PySlot_PTR_STATIC(Py_tp_flags, Py_TPFLAGS_DEFAULT),
180+
PySlot_END
181+
};
182+
PyObject *first, *second;
183+
Py_ssize_t first_refcnt, second_refcnt;
184+
PyTypeObject *slot = NULL;
185+
186+
first = PyType_FromSlots(slots);
187+
if (first == NULL) {
188+
return NULL;
189+
}
190+
second = PyType_FromSlots(slots);
191+
if (second == NULL) {
192+
Py_DECREF(first);
193+
return NULL;
194+
}
195+
first_refcnt = Py_REFCNT(first);
196+
second_refcnt = Py_REFCNT(first);
197+
198+
// Both pointer arguments alias. The assignment makes first the old value
199+
// that Py_SETREF() must release before storing second.
200+
setref_strict_aliasing(&slot, &slot,
201+
(PyTypeObject*)Py_NewRef(first),
202+
(PyTypeObject*)Py_NewRef(second));
203+
assert((PyObject*)slot == second);
204+
assert(Py_REFCNT(first) == first_refcnt);
205+
assert(Py_REFCNT(second) == (second_refcnt + 1));
206+
207+
pyclear_strict_aliasing(&slot, &slot);
208+
assert(slot == NULL);
209+
assert(Py_REFCNT(first) == first_refcnt);
210+
assert(Py_REFCNT(second) == second_refcnt);
211+
212+
xsetref_strict_aliasing(&slot, &slot,
213+
(PyTypeObject*)Py_NewRef(first),
214+
(PyTypeObject*)Py_NewRef(second));
215+
assert((PyObject*)slot == second);
216+
assert(Py_REFCNT(first) == first_refcnt);
217+
assert(Py_REFCNT(second) == (second_refcnt + 1));
218+
219+
Py_DECREF(first);
220+
Py_DECREF(second);
221+
Py_DECREF((PyObject*)slot);
222+
223+
Py_RETURN_NONE;
224+
}
225+
#endif // !Py_TARGET_ABI3T
226+
227+
143228
static PyObject *
144229
test_datetime(PyObject *Py_UNUSED(module), PyObject *Py_UNUSED(args))
145230
{
@@ -366,6 +451,9 @@ test_virtual_object(PyObject *Py_UNUSED(module), PyObject *Py_UNUSED(args))
366451
static PyMethodDef module_methods[] = {
367452
{"add", test_add, METH_VARARGS, test_add_doc},
368453
{"test_macros", test_macros, METH_NOARGS, _Py_NULL},
454+
#ifndef Py_TARGET_ABI3T
455+
{"test_setref_strict_aliasing", test_setref_strict_aliasing, METH_NOARGS, _Py_NULL},
456+
#endif
369457
{"test_datetime", test_datetime, METH_NOARGS, _Py_NULL},
370458
{"test_unicode", test_unicode, METH_NOARGS, _Py_NULL},
371459
#ifdef __cplusplus
Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
Fix a strict aliasing issue in the :c:func:`!memcpy` implementation of
2+
:c:macro:`Py_SETREF`, :c:macro:`Py_XSETREF` and :c:macro:`Py_CLEAR` macros.
3+
Patch by Nathan Goldbaum and Victor Stinner.

0 commit comments

Comments
 (0)