Add Py_SETREF/Py_XSETREF to old limited C API versions

[vstinner]

No description provided.

[vstinner]

@ngoldbaum: Would you mind to review this change?

[vstinner]

cc @prathamhole14

[ngoldbaum]

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().

[vstinner]

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++?

[ngoldbaum]

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.

[vstinner]

If we change the memcpy() implementation of Py_SETREF(), the fix should happen in Python first.

[ngoldbaum]

Agreed, sorry for missing this over there. I didn't get a chance to look this week.

[vstinner]

No problem. I will prepare a fix for Python later.