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

[Phil Thompson]

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