Macro Py_CLEAR references argument two times.

[hochl]

Bug report

The macro Py_CLEAR(op) references the argument op two times. If the macro is called with an expression it will be evaluated two times, for example Py_CLEAR(p++).

Your environment

x86_64

• CPython versions tested on:
  Python 3.7m
• Operating system and architecture:
  Debian Stable

I suggest a fix similar to this (old version commented out with #if 0):

#if 0
#define Py_CLEAR(op)                            \
    do {                                        \
        PyObject *_py_tmp = (PyObject *)(op);   \
        if (_py_tmp != NULL) {                  \
            (op) = NULL;                        \
            Py_DECREF(_py_tmp);                 \
        }                                       \
    } while (0)
#else
#define Py_CLEAR(op)                                         \
    do {                                                     \
        PyObject **_py_tmp = (PyObject **)&(op);             \
        if (*_py_tmp != NULL) {                              \
            PyObject *_py_tmp2 = *_py_tmp;                   \
            (*_py_tmp) = NULL;                               \
            Py_DECREF(_py_tmp2);                             \
        }                                                    \
    } while (0)
#endif

I am not sure if this has happened anywhere, but I see a possible problem here. I think the compiler will optimize out the additional temporary variable in most cases.

• PR: gh-98724: Fix Py_CLEAR() macro side effects #99100

• PR: [3.11] gh-98724: Fix Py_CLEAR() macro side effects (#99100) #99288

• PR: [3.10] gh-98724: Fix Py_CLEAR() macro side effects (GH-99100) (GH-99288) #99292

• PR: [3.11] Revert "[3.11] gh-98724: Fix Py_CLEAR() macro side effects (#99100)" #99573

• PR: Revert "gh-98724: Fix Py_CLEAR() macro side effects" #99737

• PR: gh-98724: Fix type punning issue in Py_SETREF() #99739

• PR: gh-98724: Fix warnings on Py_SETREF() usage #99781

• PR: gh-98724: Fix Py_CLEAR() macro side effects (#99100) #100070

• PR: gh-98724: Fix the Py_CLEAR() macro in the limited C API #100121

[sobolevn]

Here's an example of an expression in PyClear:

cpython/Modules/selectmodule.c

Lines 108 to 116 in 365852a

	static void
	reap_obj(pylist fd2obj[FD_SETSIZE + 1])
	{
	unsigned int i;
	for (i = 0; i < (unsigned int)FD_SETSIZE + 1 && fd2obj[i].sentinel >= 0; i++) {
	Py_CLEAR(fd2obj[i].obj);
	}
	fd2obj[0].sentinel = -1;
	}

We also have:

static int
template_clear(TemplateObject *self)
{
    Py_CLEAR(self->literal);
    for (Py_ssize_t i = 0, n = Py_SIZE(self); i < n; i++) {
        Py_CLEAR(self->items[i].literal);
    }
    return 0;
}

And

        for (Py_ssize_t i = 0; i < keys->dk_nentries; i++) {
            Py_CLEAR(values->values[i]);
        }

All cases are element access, but this is the most complex examples I am able to find.

[hochl]

I am not sure that I made the point totally clear. Here is an example program for clarification, using some dummy type for PyObject.

#include <stdlib.h>

#define _PyObject_CAST(op) ((PyObject*)(op))
#define Py_CLEAR(op)                            \
    do {                                        \
        PyObject *_py_tmp = _PyObject_CAST(op); \
        if (_py_tmp != NULL) {                  \
            (op) = NULL;                        \
            /* Py_DECREF(_py_tmp); */           \
            free((void*)_py_tmp);               \
        }                                       \
    } while (0)

typedef struct {
} PyObject;

int main()
{
    PyObject* obj[16];
    PyObject** p = obj;
    size_t i;

    for (i = 0; i < 16; i++) {
        obj[i] = malloc(sizeof(PyObject));
    }

#if 1
    for (int i = 0; i < 16; i++) {
        Py_CLEAR(*p++);
    }
#else
    for (int i = 0; i < 16; i++, p++) {
        Py_CLEAR(*p);
    }
#endif
}

This code will give a buffer overflow. Changing the last line to for (int i = 0; i < 16; i++, p++) Py_CLEAR(*p); works without problems. The side effect is not triggered by Python library code and most probably not in any sane C project, but it is not unconceivable that this macro shoots someone out there into the foot. I suggest that, if you have the choice, to somehow make all macros evaluate their arguments just once.

[ericvsmith]

I'm not sure we've ever promised that macros won't evaluate their args more than once. @vstinner ?

[vstinner]

"Duplication of side effects" is one catch of macros that PEP 670 tries to avoid: https://peps.python.org/pep-0670/#rationale

Sadly, Py_CLEAR() cannot be converted to a function (as part of PEP 670) since it magically gets a reference to a pointer thanks to magic macro preprocessor. An hypothetical Py_Clear() function would take a pointer to a Python object, so PyObject** type, like: Py_Clear(&variable);.

IMO using &argument in the macro implementation is an acceptable fix to prevent the duplication of side effects.

I think the compiler will optimize out the additional temporary variable in most cases.

I agree with you. Moreover, correctness matters more than performance for Py_CLEAR() API.

cc @serhiy-storchaka @erlend-aasland

[vstinner]

Maybe a Py_ClearRef(PyObject **) function shoud also be added?

Somehow related discussions:

• [C API] Add Py_NewRef() function to get a new strong reference to an object #86428
• [C API] Add new C functions with more regular reference counting like PyTuple_GetItemRef() #86460

[serhiy-storchaka]

I am against extending ABI without need. Py_CLEAR is a part of API, but not in ABI. Incref/decref is anough, Py_CLEAR is just an API sugar.

Calling Py_Clear(&variable) can prevent some compiler optimizations. The compiler can no longer use a register for variable in a register, and it can no longer assume variable is now NULL, and it cannot guarantee the the value of local variable will not change outside of the local code. I faced a similar situation recently. And the problem was not only that the compiler generated less efficient code, but that it started issuing warnings about correct code that it could no longer fully analyze.

[hochl]

How about this macro?

#define Py_CLEAR(op)                              \
    do {                                          \
        PyObject **_py_tmp = (PyObject**)&(op);   \
        if (*_py_tmp != NULL) {                   \
            PyObject* _py_tmp2 = *_py_tmp;        \
            *_py_tmp = NULL;                      \
            /* Py_DECREF(_py_tmp2); */            \
            free((void*)_py_tmp2);                \
        }                                         \
    } while (0)

Replace the free line with the commented out line for real Python, this is for my earlier example program.

[vstinner]

The macro Py_CLEAR(op) references the argument op two times. If the macro is called with an expression it will be evaluated two times, for example Py_CLEAR(p++).

This issue looks an hypothetical bug, but I'm not convinced that it's possible to write an expression which a side effect and which makes sense to set to NULL. For example, in your example, what's the point of writing p++ = NULL; (through the macro).

To be clear, GCC fails to build the following C code:

int main()
{
    void *ptr = 0;
    ptr++ = 0;
    return 0;
}

[vstinner]

I created PR #99100 to fix this issue. I'm not convinced that we should fix it, but a full PR might help to make a decision.

In the past, I saw surprising bug like https://bugs.python.org/issue43181 about passing a C++ expression to a C macro. So well, maybe in case of doubt, it's better to fix the issue to be extra safe. Py_CLEAR() pretends to be safer than using directly the Py_DECREF() macro ;-)

[vstinner]

Ah wait, I read again #98724 (comment) and now I got the issue :-) I updated the unit test in my PR #99100.

[hochl]

Perfect! The main problem I have with macros that pretend to be functions is that they might evaluate their arguments several times, and this is totally unexpected. I hope the improved version works for all use-cases :)

[serhiy-storchaka]

I am not sure that it is not just a documentation issue. We can just document that the argument of Py_CLEAR (and the first argument of Py_SETREF) should not have side effect.

We can make it working even for arguments with a side effect, but should it be considered a bug fix or a new feature?