Skip to content

Commit f8e65ee

Browse files
vstinnerngoldbaum
andauthored
gh-161185: Fix strict aliasing issue in Py_SETREF() macros family (#161186)
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. Add the _Py_TESTCEXT_NO_TYPEOF macro to undefine the _Py_TYPEOF macro. Co-authored-by: Nathan Goldbaum <nathan.goldbaum@gmail.com>
1 parent 31bd6a4 commit f8e65ee

5 files changed

Lines changed: 118 additions & 9 deletions

File tree

‎Include/pyport.h‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -550,6 +550,11 @@ extern "C" {
550550
# define _Py_TYPEOF(expr) __typeof__(expr)
551551
#endif
552552

553+
#ifdef _Py_TESTCEXT_NO_TYPEOF
554+
// Exercise fallback implementations in test_cext.
555+
# undef _Py_TYPEOF
556+
#endif
557+
553558

554559
/* A convenient way for code to know if sanitizers are enabled. */
555560
#if defined(__has_feature)

‎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,22 +546,23 @@ 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; \
563564
PyObject *_tmp_src = _PyObject_CAST(src); \
565+
memcpy(&_tmp_old_dst, _tmp_dst_ptr, sizeof(PyObject*)); \
564566
memcpy(_tmp_dst_ptr, &_tmp_src, sizeof(PyObject*)); \
565567
Py_DECREF(_tmp_old_dst); \
566568
} while (0)
@@ -575,22 +577,23 @@ 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; \
593595
PyObject *_tmp_src = _PyObject_CAST(src); \
596+
memcpy(&_tmp_old_dst, _tmp_dst_ptr, sizeof(PyObject*)); \
594597
memcpy(_tmp_dst_ptr, &_tmp_src, sizeof(PyObject*)); \
595598
Py_XDECREF(_tmp_old_dst); \
596599
} while (0)

‎Lib/test/test_cext/__init__.py‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,13 @@ class TestPublicC(BaseTests, unittest.TestCase):
113113
def test_build(self):
114114
self.check_build('_test_cext')
115115

116+
def test_build_no_typeof(self):
117+
# gh-161185: Exercise the Py_CLEAR/Py_SETREF() memcpy implementation
118+
# with strict aliasing enabled: undefine the _Py_TYPEOF macro.
119+
self.check_build(
120+
'_test_cext_no_typeof',
121+
extra_cflags='-D_Py_TESTCEXT_NO_TYPEOF -O2 -fstrict-aliasing')
122+
116123
@unittest.skipIf(MSVC, "MSVC doesn't support /std:c99")
117124
def test_build_c99(self):
118125
# In public docs, we say C API is compatible with C11. However,
@@ -146,6 +153,15 @@ def test_build_cpp03(self):
146153
# Please ask the C API WG before adding a new C++11-only feature.
147154
self.check_build('_test_cppext_cpp03', std='c++03')
148155

156+
@unittest.skipIf(MSVC, "MSVC doesn't support /std:c++03")
157+
def test_build_cpp03_no_typeof(self):
158+
# gh-161185: Exercise the Py_CLEAR/Py_SETREF() memcpy implementation
159+
# with strict aliasing enabled: undefine the _Py_TYPEOF macro.
160+
self.check_build(
161+
'_test_cppext_cpp03_no_typeof',
162+
std='c++03',
163+
extra_cflags='-D_Py_TESTCEXT_NO_TYPEOF -O2 -fstrict-aliasing')
164+
149165
@unittest.skipIf(MSVC, "MSVC doesn't support /std:c++11")
150166
def test_build_cpp11(self):
151167
self.check_build('_test_cppext_cpp11', std='c++11')

‎Lib/test/test_cext/extension.c‎

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

142142

143+
Py_NO_INLINE static void
144+
pyclear_strict_aliasing(PyTypeObject **a, PyTypeObject **b)
145+
{
146+
Py_CLEAR(*a);
147+
*b = NULL;
148+
}
149+
150+
151+
Py_NO_INLINE static void
152+
setref_strict_aliasing(PyTypeObject **a, PyTypeObject **b,
153+
PyTypeObject *first, PyTypeObject *second)
154+
{
155+
*a = first;
156+
Py_SETREF(*b, second);
157+
}
158+
159+
160+
Py_NO_INLINE static void
161+
xsetref_strict_aliasing(PyTypeObject **a, PyTypeObject **b,
162+
PyTypeObject *first, PyTypeObject *second)
163+
{
164+
*a = first;
165+
Py_XSETREF(*b, second);
166+
}
167+
168+
169+
// gh-161185: Test that Py_SETREF(), Py_XSETREF() and Py_CLEAR() macros
170+
// have no strict aliasing issue
171+
static PyObject *
172+
test_setref_strict_aliasing(PyObject *Py_UNUSED(module), PyObject *Py_UNUSED(args))
173+
{
174+
PySlot slots[] = {
175+
PySlot_PTR_STATIC(Py_tp_name, STR(MODULE_NAME) ".setref_type"),
176+
PySlot_PTR_STATIC(Py_tp_flags, Py_TPFLAGS_DEFAULT),
177+
PySlot_END
178+
};
179+
PyObject *first, *second;
180+
Py_ssize_t first_refcnt, second_refcnt;
181+
PyTypeObject *slot = NULL;
182+
183+
first = PyType_FromSlots(slots);
184+
if (first == NULL) {
185+
return NULL;
186+
}
187+
second = PyType_FromSlots(slots);
188+
if (second == NULL) {
189+
Py_DECREF(first);
190+
return NULL;
191+
}
192+
first_refcnt = Py_REFCNT(first);
193+
second_refcnt = Py_REFCNT(second);
194+
195+
// Both pointer arguments alias. The assignment makes first the old value
196+
// that Py_SETREF() must release before storing second.
197+
setref_strict_aliasing(&slot, &slot,
198+
(PyTypeObject*)Py_NewRef(first),
199+
(PyTypeObject*)Py_NewRef(second));
200+
assert((PyObject*)slot == second);
201+
assert(Py_REFCNT(first) == first_refcnt);
202+
assert(Py_REFCNT(second) == (second_refcnt + 1));
203+
204+
pyclear_strict_aliasing(&slot, &slot);
205+
assert(slot == NULL);
206+
assert(Py_REFCNT(first) == first_refcnt);
207+
assert(Py_REFCNT(second) == second_refcnt);
208+
209+
xsetref_strict_aliasing(&slot, &slot,
210+
(PyTypeObject*)Py_NewRef(first),
211+
(PyTypeObject*)Py_NewRef(second));
212+
assert((PyObject*)slot == second);
213+
assert(Py_REFCNT(first) == first_refcnt);
214+
assert(Py_REFCNT(second) == (second_refcnt + 1));
215+
216+
Py_DECREF(first);
217+
Py_DECREF(second);
218+
Py_DECREF((PyObject*)slot);
219+
220+
Py_RETURN_NONE;
221+
}
222+
223+
143224
static PyObject *
144225
test_datetime(PyObject *Py_UNUSED(module), PyObject *Py_UNUSED(args))
145226
{
@@ -366,6 +447,7 @@ test_virtual_object(PyObject *Py_UNUSED(module), PyObject *Py_UNUSED(args))
366447
static PyMethodDef module_methods[] = {
367448
{"add", test_add, METH_VARARGS, test_add_doc},
368449
{"test_macros", test_macros, METH_NOARGS, _Py_NULL},
450+
{"test_setref_strict_aliasing", test_setref_strict_aliasing, METH_NOARGS, _Py_NULL},
369451
{"test_datetime", test_datetime, METH_NOARGS, _Py_NULL},
370452
{"test_unicode", test_unicode, METH_NOARGS, _Py_NULL},
371453
#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)