gh-111178: Generate correct signature for most self converters

[erlend-aasland]

• Issue: UBSan: Calling a function through pointer to incorrect function type is undefined behavior #111178

[erlend-aasland]

WDYT, @picnixz?

[picnixz]

I think this is what I had in mind. I'm no more on my dev session but I'll pick your branch, create a branch with all my known UBSan fixes branches and then I'll need to manually address the remaining ones.

AFAICT, no new _impl functions were added so performances shouldn't be affected I think? (it's only pointer casts for the compiler). I'll review more in details tomorrow but thank you for the work!

[erlend-aasland]

Yeah, this PR only makes sure we're using PyObject pointers in the PyMethodDef "entries". No new [clinic input] sections, no new _impl functions. Performance should not be affected.

[erlend-aasland]

I'll leave this in draft so we don't create a review rush.

[erlend-aasland]

Friendly ping, @picnixz!

[picnixz]

Sorry for the late reply. I checked what I still need to do for other UBSan failures and there are quite a lot of places to fix. Here's what I can suggest: we first merge your PR and then we continue fixing the rest of UBSan failures. I wanted to delay this PR until the other failures are patched but there just too many errors in the clang output that it's hard to know where to start.

So, I think we can first fix those now and then continue the work normally. I'll also tag @vstinner, @encukou and @ZeroIntensity who worked with me on that matter to hear their thoughts.

[encukou]

+1, let's not delay this step.

[picnixz]

Not sure if this is meant to be covered by this PR but:

static PyObject *
list_append(PyListObject *self, PyObject *object)
{
    PyObject *return_value = NULL;

    Py_BEGIN_CRITICAL_SECTION(self);
    return_value = list_append_impl((PyListObject *)self, object);
    Py_END_CRITICAL_SECTION();

    return return_value;
}

has not been changed. list_append (in listobject.c.h) should accept a PyObject *. Same for list_remove and others.

[picnixz]

Also, the subsequent casts:

#define PY_LIST_CLEAR_METHODDEF    \
    {"clear", (PyCFunction)py_list_clear, METH_NOARGS, py_list_clear__doc__},

for

static PyObject *
py_list_clear(PyObject *self, PyObject *Py_UNUSED(ignored))
{
    PyObject *return_value = NULL;

    Py_BEGIN_CRITICAL_SECTION(self);
    return_value = py_list_clear_impl((PyListObject *)self);
    Py_END_CRITICAL_SECTION();

    return return_value;
}

are not needed but I don't know if it's possible to only remove the cast for the "fixed" methods or not.

[erlend-aasland]

Correct, @picnixz, I don't think this is a complete solution. But these kinds of changes have a huge impact. It is pretty easy to validate this change, for example, but if we bake in another Argument Clinic change, it will be harder to validate the generated output. IMO, it is better to address this in a follow-up PR.

[picnixz]

, it will be harder to validate the generated output. IMO, it is better to address this in a follow-up PR.

Sure! I just wanted to be sure that you knew about this. But yes, otherwise I validate the current change (sorry for not having said it explicitly).

[picnixz]

@erlend-aasland We're having some CI failures now but I think it's because main wasn't merged recently and clinic not regen.

[erlend-aasland]

Hm, yeah I guess we should've synced it before merging.

[picnixz]

Do you want to make a patch? (I can also do it if you don't have the time now)

[erlend-aasland]

Yes, I'm creating an amendment now. Compiling ... 🤖 🤖 🤖

[erlend-aasland]

Amendment up: #129060.

[vstinner]

Nice enhancement, thanks.