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

[Phil Thompson]

Fixes will be in tonight's SIP and PyQt5 snapshots. With them your test 
script doesn't seem to crash.

Thanks,
Phil

On 21/04/2020 10:53, Ales Erjavec wrote:
> 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
> _______________________________________________
> PyQt mailing list    PyQt at riverbankcomputing.com
> https://www.riverbankcomputing.com/mailman/listinfo/pyqt