Repository navigation
gh-161185: Fix strict aliasing issue in Py_SETREF() macros family - #161186
Merged
Merged
Conversation
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>
vstinner
added
topic-C-API
needs backport to 3.14
Member
Author
Member
Author
|
To reproduce #161185 issue:
=> The test does crash (segfault) or fails with a C assertion error. Patch to force the memcpy() implementation and reintroduce the strict aliasing bug: Detailsdiff --git a/Include/refcount.h b/Include/refcount.h
index 2bc2e83ef1c..7e5e9e46117 100644
--- a/Include/refcount.h
+++ b/Include/refcount.h
@@ -482,7 +482,7 @@ static inline Py_ALWAYS_INLINE void Py_DECREF(PyObject *op)
* On C++11 and newer, use "auto". On MSVC, check also _MSVC_LANG since
* __cplusplus is 199711L unless the /Zc:__cplusplus flag is used.
*/
-#ifdef _Py_TYPEOF
+#if 0 // def _Py_TYPEOF
#define Py_CLEAR(op) \
do { \
_Py_TYPEOF(&(op)) _tmp_op_ptr = &(op); \
@@ -492,7 +492,7 @@ static inline Py_ALWAYS_INLINE void Py_DECREF(PyObject *op)
Py_DECREF(_PyObject_CAST(_tmp_old_op)); \
} \
} while (0)
-#elif defined(__cplusplus) && (__cplusplus >= 201103L || _MSVC_LANG >= 201103L)
+#elif 0 // defined(__cplusplus) && (__cplusplus >= 201103L || _MSVC_LANG >= 201103L)
#define Py_CLEAR(op) \
do { \
auto _tmp_op_ptr = &(op); \
@@ -506,8 +506,7 @@ static inline Py_ALWAYS_INLINE void Py_DECREF(PyObject *op)
#define Py_CLEAR(op) \
do { \
PyObject **_tmp_op_ptr = _Py_CAST(PyObject**, &(op)); \
- PyObject *_tmp_old_op; \
- memcpy(&_tmp_old_op, _tmp_op_ptr, sizeof(PyObject*)); \
+ PyObject *_tmp_old_op = (*_tmp_op_ptr); \
if (_tmp_old_op != _Py_NULL) { \
PyObject *_null_ptr = _Py_NULL; \
memcpy(_tmp_op_ptr, &_null_ptr, sizeof(PyObject*)); \
@@ -540,7 +539,7 @@ static inline Py_ALWAYS_INLINE void Py_DECREF(PyObject *op)
* The memcpy() implementation does not emit a compiler warning if 'src' has
* not the same type than 'src': any pointer type is accepted for 'src'.
*/
-#ifdef _Py_TYPEOF
+#if 0 // def _Py_TYPEOF
#define Py_SETREF(dst, src) \
do { \
_Py_TYPEOF(&(dst)) _tmp_dst_ptr = &(dst); \
@@ -548,7 +547,7 @@ static inline Py_ALWAYS_INLINE void Py_DECREF(PyObject *op)
*_tmp_dst_ptr = (src); \
Py_DECREF(_PyObject_CAST(_tmp_old_dst)); \
} while (0)
-#elif defined(__cplusplus) && (__cplusplus >= 201103L || _MSVC_LANG >= 201103L)
+#elif 0 // defined(__cplusplus) && (__cplusplus >= 201103L || _MSVC_LANG >= 201103L)
#define Py_SETREF(dst, src) \
do { \
auto _tmp_dst_ptr = &(dst); \
@@ -560,8 +559,7 @@ static inline Py_ALWAYS_INLINE void Py_DECREF(PyObject *op)
#define Py_SETREF(dst, src) \
do { \
PyObject **_tmp_dst_ptr = _Py_CAST(PyObject**, &(dst)); \
- PyObject *_tmp_old_dst; \
- memcpy(&_tmp_old_dst, _tmp_dst_ptr, sizeof(PyObject*)); \
+ PyObject *_tmp_old_dst = (*_tmp_dst_ptr); \
PyObject *_tmp_src = _PyObject_CAST(src); \
memcpy(_tmp_dst_ptr, &_tmp_src, sizeof(PyObject*)); \
Py_DECREF(_tmp_old_dst); \
@@ -571,7 +569,7 @@ static inline Py_ALWAYS_INLINE void Py_DECREF(PyObject *op)
/* Py_XSETREF() is a variant of Py_SETREF() that uses Py_XDECREF() instead of
* Py_DECREF().
*/
-#ifdef _Py_TYPEOF
+#if 0 // def _Py_TYPEOF
#define Py_XSETREF(dst, src) \
do { \
_Py_TYPEOF(&(dst)) _tmp_dst_ptr = &(dst); \
@@ -579,7 +577,7 @@ static inline Py_ALWAYS_INLINE void Py_DECREF(PyObject *op)
*_tmp_dst_ptr = (src); \
Py_XDECREF(_PyObject_CAST(_tmp_old_dst)); \
} while (0)
-#elif defined(__cplusplus) && (__cplusplus >= 201103L || _MSVC_LANG >= 201103L)
+#elif 0 // defined(__cplusplus) && (__cplusplus >= 201103L || _MSVC_LANG >= 201103L)
#define Py_XSETREF(dst, src) \
do { \
auto _tmp_dst_ptr = &(dst); \
@@ -591,8 +589,7 @@ static inline Py_ALWAYS_INLINE void Py_DECREF(PyObject *op)
#define Py_XSETREF(dst, src) \
do { \
PyObject **_tmp_dst_ptr = _Py_CAST(PyObject**, &(dst)); \
- PyObject *_tmp_old_dst; \
- memcpy(&_tmp_old_dst, _tmp_dst_ptr, sizeof(PyObject*)); \
+ PyObject *_tmp_old_dst = (*_tmp_dst_ptr); \
PyObject *_tmp_src = _PyObject_CAST(src); \
memcpy(_tmp_dst_ptr, &_tmp_src, sizeof(PyObject*)); \
Py_XDECREF(_tmp_old_dst); \ |
ngoldbaum
approved these changes
Nov 4, 2026
ngoldbaum
left a comment
ngoldbaum
left a comment
Contributor
There was a problem hiding this comment.
Looks good besides the one test nit. I'm hitting approve.
My clanker says the fallback path isn't exercised in any extant test configuration. Does it maybe make sense to force it? like this:
Details
--- a/Include/pyport.h
+++ b/Include/pyport.h
@@ -550,6 +550,11 @@
# define _Py_TYPEOF(expr) __typeof__(expr)
#endif
+#if defined(_Py_IS_TESTCEXT) && defined(_Py_TESTCEXT_NO_TYPEOF)
+ // Exercise fallback implementations in test_cext.
+# undef _Py_TYPEOF
+#endif
+
/* A convenient way for code to know if sanitizers are enabled. */
#if defined(__has_feature)
--- a/Lib/test/test_cext/__init__.py
+++ b/Lib/test/test_cext/__init__.py
@@ -146,6 +146,18 @@
# Please ask the C API WG before adding a new C++11-only feature.
self.check_build('_test_cppext_cpp03', std='c++03')
+ @unittest.skipIf(MSVC, "MSVC doesn't support /std:c++03")
+ def test_build_cpp03_memcpy(self):
+ # Exercise the memcpy fallback with strict aliasing enabled.
+ self.check_build(
+ '_test_cppext_cpp03_memcpy',
+ std='c++03',
+ extra_cflags=(
+ '-D_Py_IS_TESTCEXT -D_Py_TESTCEXT_NO_TYPEOF '
+ '-O2 -fstrict-aliasing'
+ ),
+ )
+Move variable declaration before code.
There was a problem hiding this comment.
@vstinner Thank you for the PR, I was about to comment on the test typo. This looks good to me
Edit: Oh I see Nathan has already pointed that out!
Add the _Py_TESTCEXT_NO_TYPEOF macro to undefine the _Py_TYPEOF macro. Co-Authored-by: Nathan Goldbaum <nathan.goldbaum@gmail.com>
Member
Author
Yes, it makes sense. It should help to catch regressions earlier, rather than waiting until someone hit a bug in a specific operating system with a specific C or C++ compiler. I applied your patch (with some minor changes). |
Member
Author
|
I reintroduced the bug to make sure that test_cext catches it. And it does catch it! TestPublicC.test_build_no_typeof() and TestPublicCpp.test_build_cpp03_no_typeof() does fail with a crash. I ran test_cext on Fedora 45 with GCC 16.2.1. I also tested clang 23.1.4 using Patch to reintroduce the bug on top of this PR: diff --git a/Include/refcount.h b/Include/refcount.h
index 9b681f6a254..78260d3c457 100644
--- a/Include/refcount.h
+++ b/Include/refcount.h
@@ -506,8 +506,7 @@ static inline Py_ALWAYS_INLINE void Py_DECREF(PyObject *op)
#define Py_CLEAR(op) \
do { \
PyObject **_tmp_op_ptr = _Py_CAST(PyObject**, &(op)); \
- PyObject *_tmp_old_op; \
- memcpy(&_tmp_old_op, _tmp_op_ptr, sizeof(PyObject*)); \
+ PyObject *_tmp_old_op = (*_tmp_op_ptr); \
if (_tmp_old_op != _Py_NULL) { \
PyObject *_null_ptr = _Py_NULL; \
memcpy(_tmp_op_ptr, &_null_ptr, sizeof(PyObject*)); \
@@ -560,9 +559,8 @@ static inline Py_ALWAYS_INLINE void Py_DECREF(PyObject *op)
#define Py_SETREF(dst, src) \
do { \
PyObject **_tmp_dst_ptr = _Py_CAST(PyObject**, &(dst)); \
- PyObject *_tmp_old_dst; \
+ PyObject *_tmp_old_dst = (*_tmp_dst_ptr); \
PyObject *_tmp_src = _PyObject_CAST(src); \
- memcpy(&_tmp_old_dst, _tmp_dst_ptr, sizeof(PyObject*)); \
memcpy(_tmp_dst_ptr, &_tmp_src, sizeof(PyObject*)); \
Py_DECREF(_tmp_old_dst); \
} while (0)
@@ -591,9 +589,8 @@ static inline Py_ALWAYS_INLINE void Py_DECREF(PyObject *op)
#define Py_XSETREF(dst, src) \
do { \
PyObject **_tmp_dst_ptr = _Py_CAST(PyObject**, &(dst)); \
- PyObject *_tmp_old_dst; \
+ PyObject *_tmp_old_dst = (*_tmp_dst_ptr); \
PyObject *_tmp_src = _PyObject_CAST(src); \
- memcpy(&_tmp_old_dst, _tmp_dst_ptr, sizeof(PyObject*)); \
memcpy(_tmp_dst_ptr, &_tmp_src, sizeof(PyObject*)); \
Py_XDECREF(_tmp_old_dst); \
} while (0) |
The Py_tp_basicsize slot is not required to define a type.
Member
Author
|
I modified the test so it can also be run on Free Threading. I tested manually that test_cext also catch the regresssion if I apply #161186 (comment) patch. |
vstinner
added
the
🔨 test-with-buildbots
|
🤖 New build scheduled with the buildbot fleet by @vstinner for commit 4d3f620 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F161186%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
bedevere-bot
removed
the
🔨 test-with-buildbots
Member
Author
|
Ok, let me run the test on buildbots to double check that I didn't miss anything. If buildbots are happy, I will merge the change ;-) |
Member
Author
|
Note to myself: once this PR is merged, I can also python/pythoncapi-compat#187 (Add Py_SETREF/Py_XSETREF to old limited C API versions) with the updated Py_CLEAR/Py_SETREF macro implementation. |
Member
Author
Unrelated failures:
Unrelated failure |
Member
Author
Unrelated failure: test_tcl failed. |
|
Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.14, 3.15. |
|
Sorry, @vstinner, I could not cleanly backport this to Please backport manually with cherry_picker, see the devguide for more information. |
|
Sorry, @vstinner, I could not cleanly backport this to Please backport manually with cherry_picker, see the devguide for more information. |
vstinner
added a commit
to vstinner/cpython
that referenced
this pull request
Nov 4, 2026
…ly (python#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> (cherry picked from commit f8e65ee)
|
GH-161360 is a backport of this pull request to the 3.15 branch. |
Member
Author
|
Thanks for your reviews! I merged my change and backport it to 3.15 (the 3.14 backport will follow). |
vstinner
added a commit
that referenced
this pull request
Nov 4, 2026
…ily (#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>
vstinner
added a commit
that referenced
this pull request
Nov 4, 2026
…ily (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>
|
|
|
|
|
|
|
Member
Author
|
Ah. I didn't test test_cext locally before merging the 3.14 backport, and obviously it fails on all buildbots :-) I wrote #161373 to fix test_cext. |
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.