[PyQt] Python owned QObject released from a thread with pending Meta call events.

[Ales Erjavec]

I do not think that would do.

The sipPySelf read itself in qt_metacall needs to be guarded. i.e.

```
int sipQObject::qt_metacall(QMetaObject::Call _c,int _id,void **_a)
{
    _id =  ::QObject::qt_metacall(_c,_id,_a);

    if (_id >= 0){
        SIP_BLOCK_THREADS
        _id = sip_QtCore_qt_metacall(sipPySelf,sipType_QObject,_c,_id,_a);
        SIP_UNBLOCK_THREADS
    }
    return _id;
}
```
(or maybe it could be passed 'indirectly' via sipSimpleWrapper
**pySelf, and then guard *pySelf uses with SIP_BLOCK/UNBLOCK_THREADS).


P.S. I am not sure if this helps but incidence of segfaults is
increased if running with `PYTHONMALLOC=debug` env variable set.

On Sat, Apr 18, 2020 at 3:51 PM Phil Thompson
<phil at riverbankcomputing.com> wrote:
>
> On 15/04/2020 10:49, Ales Erjavec wrote:
> > Hi,
> >
> > I have been struggling with segfaults when using QObject subclass as a
> > queued communicator between threads.
> >
> > I think there is an unsafe access in the implementation of
> > sipQObject::qt_metacall, qpycore_qobject_qt_metacall and the
> > `dealloc_QObject`, when the last reference to the sip python wrapper
> > object is released and deallocated from a thread that is not its own
> > (i.e. QThread().currentThread() is not qobj.thread())
> >
> > In particular; dealloc_QObject:
> >
> > ```
> > static void dealloc_QObject(sipSimpleWrapper *sipSelf)
> > {
> >     if (sipIsDerivedClass(sipSelf))
> >         reinterpret_cast<sipQObject
> > *>(sipGetAddress(sipSelf))->sipPySelf = SIP_NULLPTR;
> >
> >     if (sipIsOwnedByPython(sipSelf))
> >     {
> >         release_QObject(sipGetAddress(sipSelf),
> > sipIsDerivedClass(sipSelf));
> >     }
> > }
> > ```
> >
> > Here the pointer from sipQObject to sip python wrapper (sipPySelf) is
> > cleared (this happens with the GIL held), but when
> > the current thread is not the qobjects's thread the sipQObject's
> > deletion is scheduled by deleteLater() in release_QObject:
> >
> > ```
> > static void release_QObject(void *sipCppV, int)
> > {
> >      ::QObject *sipCpp = reinterpret_cast< ::QObject *>(sipCppV);
> >
> >     if (QThread::currentThread() == sipCpp->thread())
> >         delete sipCpp;
> >     else
> >         sipCpp->deleteLater();
> > }
> > ```
> >
> > Say however that the object has pending meta call events
> > (slots/signals scheduled via Qt's meta object system e.g. signal/slot
> > connections using Qt.QueuedConnection). Such events eventually get to:
> >
> > ```
> > int sipQObject::qt_metacall(QMetaObject::Call _c,int _id,void **_a)
> > {
> >     _id =  ::QObject::qt_metacall(_c,_id,_a);
> >
> >     if (_id >= 0)
> >         _id =
> > sip_QtCore_qt_metacall(sipPySelf,sipType_QObject,_c,_id,_a);
> >
> >     return _id;
> > }
> > ```
> >
> > This is called without the GIL held, and reads the value of sipPySelf
> > to pass to `sip_QtCore_qt_metacall` (alias for
> > `qpycore_qobject_qt_metacall`)
> >
> > ```
> > int qpycore_qobject_qt_metacall(sipSimpleWrapper *pySelf, sipTypeDef
> > *base,
> >         QMetaObject::Call _c, int _id, void **_a)
> > {
> >     // Check if the Python object has gone.
> >     if (!pySelf)
> >         return -1;
> >
> >     SIP_BLOCK_THREADS
> >     _id = qt_metacall_worker(pySelf, Py_TYPE(pySelf), base, _c, _id,
> > _a);
> >     SIP_UNBLOCK_THREADS
> >
> >     return _id;
> > }
> >
> > ```
> > Between the point in sipQObject::qt_metacall where the sipPySelf is
> > read and SIP_BLOCK_THREADS in `qpycore_qobject_qt_metacall` the python
> > wrapper can be deallocated, leaving pySelf on the stack a dangling
> > pointer (in fact the memory it points to could have already been
> > reused).
> >
> > I think the entire
> > `sip_QtCore_qt_metacall(sipPySelf,sipType_QObject,_c,_id,_a)` call in
> > sipQObject::qt_metacall needs to be inside a
> > SIP_BLOCK/UNBLOCK_THREADS.
>
> That sounds reasonable. I've changed the code (for tonight's PyQt5
> snapshot) to be...
>
>      SIP_BLOCK_THREADS
>
>      // Check that the Python object has not gone.
>      if (pySelf)
>          _id = qt_metacall_worker(pySelf, Py_TYPE(pySelf), base, _c, _id,
> _a);
>      else
>          _id = -1;
>
>      SIP_UNBLOCK_THREADS
>
>      return _id;
>
> Phil