GeezerVison is showing github.com — Add Py_SETREF/Py_XSETREF to old limited C API versions by vstinner · Pull Request #187 · python/pythoncapi-compat
Conversation
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
prathamhole14 reacted with rocket emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
I asked an AI model to look this over and it spotted an issue (CPython's fallback also has this problem):
The memcpy() protects the write, but the preceding read still violates strict aliasing:
PyObject **_tmp_dst_ptr = _Py_CAST(PyObject**, &(dst));
PyObject *_tmp_old_dst = (*_tmp_dst_ptr);
If dst is a PyTypeObject *, dereferencing _tmp_dst_ptr accesses a PyTypeObject * object through a PyObject * lvalue. GCC can therefore assume that this read does not observe a write made through PyTypeObject **.
Here is a standalone reproducer using the exact Py_XSETREF() implementation from this PR:
#include <Python.h>
#include <cstdio>
#include <cstring>
#ifndef _Py_CAST
# define _Py_CAST(type, expr) ((type)(expr))
#endif
#ifndef _PyObject_CAST
# define _PyObject_CAST(obj) _Py_CAST(PyObject *, (obj))
#endif
#define REPRO_XSETREF(dst, src) \
do { \
PyObject **_tmp_dst_ptr = _Py_CAST(PyObject **, &(dst)); \
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); \
} while (0)
__attribute__((noinline))
static void
replace(PyTypeObject **a, PyTypeObject **b,
PyTypeObject *first, PyTypeObject *second)
{
*a = first;
REPRO_XSETREF(*b, second);
}
int
main()
{
Py_Initialize();
PyType_Slot slots[] = {{0, NULL}};
PyType_Spec spec = {
"repro.T", sizeof(PyObject), 0, Py_TPFLAGS_DEFAULT, slots
};
PyObject *first = PyType_FromSpec(&spec);
PyObject *second = PyType_FromSpec(&spec);
if (first == NULL || second == NULL) {
PyErr_Print();
return 1;
}
Py_ssize_t before = Py_REFCNT(first);
PyTypeObject *slot = NULL;
// Both pointer arguments alias. The assignment makes first the old value
// that REPRO_XSETREF() must release before storing second.
replace(&slot, &slot,
reinterpret_cast<PyTypeObject *>(Py_NewRef(first)),
reinterpret_cast<PyTypeObject *>(Py_NewRef(second)));
std::printf("first refcount: %zd; expected: %zd\n",
Py_REFCNT(first), before);
return 0;
}
On CPython 3.10 with GCC 15.2.0:
$ g++ -std=c++11 $(python3.10-config --cflags) \
-DPy_LIMITED_API=0x030a0000 -O2 -fstrict-aliasing \
repro.cpp $(python3.10-config --embed --ldflags) -o repro
$ ./repro
first refcount: 3; expected: 2
The reference acquired by Py_NewRef(first) was not released. Compiling the same source with -fno-strict-aliasing produces the expected refcount of 2.
With Py_SETREF() instead of Py_XSETREF(), the stale value can be NULL, causing a segmentation fault when it is passed to Py_DECREF().
|
prathamhole14 reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
The memcpy() protects the write, but the preceding read still violates strict aliasing
This issue is super annoying. What do you recommend to fix the issue? Copy/paste the whole Py_SETREF/XSETREF implementation to use typeof() / __typeof__() and auto on C++?
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
ngoldbaum
commented
Oct 7, 2026
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
|
The AI model's suggestion is two memcpys:
#ifndef Py_SETREF
#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_src = _PyObject_CAST(src); \
memcpy(_tmp_dst_ptr, &_tmp_src, sizeof(PyObject*)); \
Py_DECREF(_tmp_old_dst); \
} while (0)
#endif
#ifndef Py_XSETREF
#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_src = _PyObject_CAST(src); \
memcpy(_tmp_dst_ptr, &_tmp_src, sizeof(PyObject*)); \
Py_XDECREF(_tmp_old_dst); \
} while (0)
#endif
I don't think this is a performance concern, the compiler should be able to replace the memcpys with a load or store.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
If we change the memcpy() implementation of Py_SETREF(), the fix should happen in Python first.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Agreed, sorry for missing this over there. I didn't get a chance to look this week.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
No problem. I will prepare a fix for Python later.
|
prathamhole14 reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Did the CPython work happen?
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
I just checked the aliasing problem is not yet fixed in cpython, should I open the PR @vstinner ?
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
should I open the PR
Yes, please do open a CPython PR.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
prathamhole14 reacted with heart emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
|
Aaaand, it's now merged! @ngoldbaum: Thanks for spotting the strict aliasing issue!
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
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.
|