[Fixed] Crash when typing expressions

[wmayer]

FYI, there is currently a different behaviour of PyCXX for Py2 and Py3. The constructors of PyCXX for Py2 in general don't allow a null pointer as argument and when doing so an exception is raised.

For Py3 null pointers are accepted. But this means then when passing the pointer to any Py function it's up to the caller to verify that the pointer is not null if Py itself doesn't check for it.

And that's exactly what happens here. The identifier res has a null pointer but the function PyModule_Check doesn't check for null and directly accesses the type object. That's why a segfault occurs.

[realthunder]

For Py3 null pointers are accepted. But this means then when passing the pointer to any Py function it's up to the caller to verify that the pointer is not null if Py itself doesn't check for it.

Isn't that a questionable implementation? This means a lot of functions of PyCXX cannot be trusted, such as getItem(), etc? I don't think it's feasible to check null pointer for all those situations. By the look of it, PyCXX is designed with the assumption that the pointer is not null. I think at least Object::accepts() shouldn't accept null pointer if PyErr_Occured().

[wmayer]

Isn't that a questionable implementation?

It's for sure not ideal. If it was only that you must check the pointer when you access it and use the Python C-API on your own I could understand it but it's even worse. The base class Py::Object has methods like type(), str(), dir(), ... and it even crashes if you call them in case the internal pointer is null.

However, in the past it wasn't allowed to create a Py::Object from a null pointer. The restriction has been lifted when the new type Py::Null was introduced. For the Py3 implementation the virtual method accepts() of Py::Object now is:

Code: Select all

        // Can pyob be used in this object's constructor?
        virtual bool accepts( PyObject * /* pyob */ ) const
        {
            // allow any object or NULL
            return true;
        }

But all other types (except of Py::Null) still require a non-null pointer in their constructors.

Maybe it's worth to have a look at later PyCXX versions. Possibly this issue has been solved in a smarter way there.

[realthunder]

Maybe it's worth to have a look at later PyCXX versions. Possibly this issue has been solved in a smarter way there.

I just checked the sourceforge project page, Object::accepts() is still the same. Looks like the project is still maintained with recent commit. Don't know why the problem still persists. Additionally, the Python3 PyCXX document clearly mentioned that Object does not accept null pointer.

So what to do, shall we fix it?

[wmayer]

So what to do, shall we fix it?

This is no so easy because Debian or Fedora package maintainers have the policy not to use the code copies of 3rd party libraries of an upstream project when already an official package for such a library exists for their distribution.

So, this means when we change the local PyCXX code then Debian, Fedora, ... folks won't benefit from it. IMO, the best way would be to open a bug report or start a discussion on the PyCXX project.

In the meantime we have to add the security checks in our code. But I don't think that now we must go through every single use of Py::Object and add this check since in 99.9% of all cases it's guaranteed that the passed pointer is not null. So, I would just add the check where we know it currently crashes.

[wmayer]

I had a look at the PyCXX code too and for Py3 it was from the beginning on that Py::Object accepts null. And it's already more than 3 years that the Py2 implementation has been changed, too: https://sourceforge.net/p/cxx/code/354/

So, apparently the Py2 version inside the FreeCAD code base is quite outdated.

The reason that null is accepted is that it has become possible to delete attributes of a Python extension object. When you have created an instance of an extension object and you type del myextobject.attribute then the callback handler for setattr is called where the second argument is a Py::Object with a null pointer inside.

But it's still a bit odd behaviour to accept null within Py::Object because for Py3 the new type Py::Null (which is missing in Py2) has been added which explicitly allows null.