Skip to content

Commit 0dc1a96

Browse files
vstinnerngoldbaum
andauthored
[3.15] gh-161185: Fix strict aliasing issue in Py_SETREF() macros family (#161186) (#161360)
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. (cherry picked from commit f8e65ee) Co-authored-by: Nathan Goldbaum <nathan.goldbaum@gmail.com>
1 parent 6ac089b commit 0dc1a96

6 files changed

Lines changed: 123 additions & 9 deletions

File tree

‎Include/cpython/object.h‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -353,22 +353,23 @@ PyAPI_FUNC(PyObject *) _PyObject_FunctionStr(PyObject *);
353353
_Py_TYPEOF(&(dst)) _tmp_dst_ptr = &(dst); \
354354
_Py_TYPEOF(dst) _tmp_old_dst = (*_tmp_dst_ptr); \
355355
*_tmp_dst_ptr = (src); \
356-
Py_DECREF(_tmp_old_dst); \
356+
Py_DECREF(_PyObject_CAST(_tmp_old_dst)); \
357357
} while (0)
358358
#elif defined(__cplusplus) && (__cplusplus >= 201103L || _MSVC_LANG >= 201103L)
359359
#define Py_SETREF(dst, src) \
360360
do { \
361361
auto _tmp_dst_ptr = &(dst); \
362362
auto _tmp_old_dst = (*_tmp_dst_ptr); \
363363
*_tmp_dst_ptr = (src); \
364-
Py_DECREF(_tmp_old_dst); \
364+
Py_DECREF(_PyObject_CAST(_tmp_old_dst)); \
365365
} while (0)
366366
#else
367367
#define Py_SETREF(dst, src) \
368368
do { \
369369
PyObject **_tmp_dst_ptr = _Py_CAST(PyObject**, &(dst)); \
370-
PyObject *_tmp_old_dst = (*_tmp_dst_ptr); \
370+
PyObject *_tmp_old_dst; \
371371
PyObject *_tmp_src = _PyObject_CAST(src); \
372+
memcpy(&_tmp_old_dst, _tmp_dst_ptr, sizeof(PyObject*)); \
372373
memcpy(_tmp_dst_ptr, &_tmp_src, sizeof(PyObject*)); \
373374
Py_DECREF(_tmp_old_dst); \
374375
} while (0)
@@ -383,22 +384,23 @@ PyAPI_FUNC(PyObject *) _PyObject_FunctionStr(PyObject *);
383384
_Py_TYPEOF(&(dst)) _tmp_dst_ptr = &(dst); \
384385
_Py_TYPEOF(dst) _tmp_old_dst = (*_tmp_dst_ptr); \
385386
*_tmp_dst_ptr = (src); \
386-
Py_XDECREF(_tmp_old_dst); \
387+
Py_XDECREF(_PyObject_CAST(_tmp_old_dst)); \
387388
} while (0)
388389
#elif defined(__cplusplus) && (__cplusplus >= 201103L || _MSVC_LANG >= 201103L)
389390
#define Py_XSETREF(dst, src) \
390391
do { \
391392
auto _tmp_dst_ptr = &(dst); \
392393
auto _tmp_old_dst = (*_tmp_dst_ptr); \
393394
*_tmp_dst_ptr = (src); \
394-
Py_XDECREF(_tmp_old_dst); \
395+
Py_XDECREF(_PyObject_CAST(_tmp_old_dst)); \
395396
} while (0)
396397
#else
397398
#define Py_XSETREF(dst, src) \
398399
do { \
399400
PyObject **_tmp_dst_ptr = _Py_CAST(PyObject**, &(dst)); \
400-
PyObject *_tmp_old_dst = (*_tmp_dst_ptr); \
401+
PyObject *_tmp_old_dst; \
401402
PyObject *_tmp_src = _PyObject_CAST(src); \
403+
memcpy(&_tmp_old_dst, _tmp_dst_ptr, sizeof(PyObject*)); \
402404
memcpy(_tmp_dst_ptr, &_tmp_src, sizeof(PyObject*)); \
403405
Py_XDECREF(_tmp_old_dst); \
404406
} while (0)

‎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: 4 additions & 3 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*)); \

‎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: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -142,6 +142,90 @@ test_macros(PyObject *Py_UNUSED(module), PyObject *Py_UNUSED(args))
142142
}
143143

144144

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