Skip to content

Commit adb9185

Browse files
miss-islingtonvstinnerngoldbaum
authored
[3.14] gh-161185: Fix strict aliasing issue in Py_SETREF() macros family (GH-161186) (GH-161360) (#161362)
[3.15] gh-161185: Fix strict aliasing issue in Py_SETREF() macros family (GH-161186) (GH-161360) gh-161185: Fix strict aliasing issue in Py_SETREF() macros family (GH-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) (cherry picked from commit 0dc1a96) Co-authored-by: Victor Stinner <vstinner@python.org> Co-authored-by: Nathan Goldbaum <nathan.goldbaum@gmail.com>
1 parent 6fea59a commit adb9185

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
@@ -344,22 +344,23 @@ PyAPI_FUNC(PyObject *) _PyObject_FunctionStr(PyObject *);
344344
_Py_TYPEOF(&(dst)) _tmp_dst_ptr = &(dst); \
345345
_Py_TYPEOF(dst) _tmp_old_dst = (*_tmp_dst_ptr); \
346346
*_tmp_dst_ptr = (src); \
347-
Py_DECREF(_tmp_old_dst); \
347+
Py_DECREF(_PyObject_CAST(_tmp_old_dst)); \
348348
} while (0)
349349
#elif defined(__cplusplus) && (__cplusplus >= 201103L || _MSVC_LANG >= 201103L)
350350
#define Py_SETREF(dst, src) \
351351
do { \
352352
auto _tmp_dst_ptr = &(dst); \
353353
auto _tmp_old_dst = (*_tmp_dst_ptr); \
354354
*_tmp_dst_ptr = (src); \
355-
Py_DECREF(_tmp_old_dst); \
355+
Py_DECREF(_PyObject_CAST(_tmp_old_dst)); \
356356
} while (0)
357357
#else
358358
#define Py_SETREF(dst, src) \
359359
do { \
360360
PyObject **_tmp_dst_ptr = _Py_CAST(PyObject**, &(dst)); \
361-
PyObject *_tmp_old_dst = (*_tmp_dst_ptr); \
361+
PyObject *_tmp_old_dst; \
362362
PyObject *_tmp_src = _PyObject_CAST(src); \
363+
memcpy(&_tmp_old_dst, _tmp_dst_ptr, sizeof(PyObject*)); \
363364
memcpy(_tmp_dst_ptr, &_tmp_src, sizeof(PyObject*)); \
364365
Py_DECREF(_tmp_old_dst); \
365366
} while (0)
@@ -374,22 +375,23 @@ PyAPI_FUNC(PyObject *) _PyObject_FunctionStr(PyObject *);
374375
_Py_TYPEOF(&(dst)) _tmp_dst_ptr = &(dst); \
375376
_Py_TYPEOF(dst) _tmp_old_dst = (*_tmp_dst_ptr); \
376377
*_tmp_dst_ptr = (src); \
377-
Py_XDECREF(_tmp_old_dst); \
378+
Py_XDECREF(_PyObject_CAST(_tmp_old_dst)); \
378379
} while (0)
379380
#elif defined(__cplusplus) && (__cplusplus >= 201103L || _MSVC_LANG >= 201103L)
380381
#define Py_XSETREF(dst, src) \
381382
do { \
382383
auto _tmp_dst_ptr = &(dst); \
383384
auto _tmp_old_dst = (*_tmp_dst_ptr); \
384385
*_tmp_dst_ptr = (src); \
385-
Py_XDECREF(_tmp_old_dst); \
386+
Py_XDECREF(_PyObject_CAST(_tmp_old_dst)); \
386387
} while (0)
387388
#else
388389
#define Py_XSETREF(dst, src) \
389390
do { \
390391
PyObject **_tmp_dst_ptr = _Py_CAST(PyObject**, &(dst)); \
391-
PyObject *_tmp_old_dst = (*_tmp_dst_ptr); \
392+
PyObject *_tmp_old_dst; \
392393
PyObject *_tmp_src = _PyObject_CAST(src); \
394+
memcpy(&_tmp_old_dst, _tmp_dst_ptr, sizeof(PyObject*)); \
393395
memcpy(_tmp_dst_ptr, &_tmp_src, sizeof(PyObject*)); \
394396
Py_XDECREF(_tmp_old_dst); \
395397
} while (0)

‎Include/pyport.h‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -577,6 +577,11 @@ extern "C" {
577577
# define _Py_TYPEOF(expr) __typeof__(expr)
578578
#endif
579579

580+
#ifdef _Py_TESTCEXT_NO_TYPEOF
581+
// Exercise fallback implementations in test_cext.
582+
# undef _Py_TYPEOF
583+
#endif
584+
580585

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

‎Include/refcount.h‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -483,7 +483,7 @@ static inline Py_ALWAYS_INLINE void Py_DECREF(PyObject *op)
483483
_Py_TYPEOF(op) _tmp_old_op = (*_tmp_op_ptr); \
484484
if (_tmp_old_op != _Py_NULL) { \
485485
*_tmp_op_ptr = _Py_NULL; \
486-
Py_DECREF(_tmp_old_op); \
486+
Py_DECREF(_PyObject_CAST(_tmp_old_op)); \
487487
} \
488488
} while (0)
489489
#elif defined(__cplusplus) && (__cplusplus >= 201103L || _MSVC_LANG >= 201103L)
@@ -493,14 +493,15 @@ static inline Py_ALWAYS_INLINE void Py_DECREF(PyObject *op)
493493
auto _tmp_old_op = (*_tmp_op_ptr); \
494494
if (_tmp_old_op != _Py_NULL) { \
495495
*_tmp_op_ptr = _Py_NULL; \
496-
Py_DECREF(_tmp_old_op); \
496+
Py_DECREF(_PyObject_CAST(_tmp_old_op)); \
497497
} \
498498
} while (0)
499499
#else
500500
#define Py_CLEAR(op) \
501501
do { \
502502
PyObject **_tmp_op_ptr = _Py_CAST(PyObject**, &(op)); \
503-
PyObject *_tmp_old_op = (*_tmp_op_ptr); \
503+
PyObject *_tmp_old_op; \
504+
memcpy(&_tmp_old_op, _tmp_op_ptr, sizeof(PyObject*)); \
504505
if (_tmp_old_op != _Py_NULL) { \
505506
PyObject *_null_ptr = _Py_NULL; \
506507
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
@@ -115,6 +115,13 @@ class TestPublicC(BaseTests, unittest.TestCase):
115115
def test_build(self):
116116
self.check_build('_test_cext')
117117

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

154+
@unittest.skipIf(MSVC, "MSVC doesn't support /std:c++03")
155+
def test_build_cpp03_no_typeof(self):
156+
# gh-161185: Exercise the Py_CLEAR/Py_SETREF() memcpy implementation
157+
# with strict aliasing enabled: undefine the _Py_TYPEOF macro.
158+
self.check_build(
159+
'_test_cppext_cpp03_no_typeof',
160+
std='c++03',
161+
extra_cflags='-D_Py_TESTCEXT_NO_TYPEOF -O2 -fstrict-aliasing')
162+
147163
@unittest.skipIf(MSVC, "MSVC doesn't support /std:c++11")
148164
def test_build_cpp11(self):
149165
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
@@ -145,6 +145,90 @@ test_macros(PyObject *Py_UNUSED(module), PyObject *Py_UNUSED(args))
145145
}
146146

147147

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