Fix crash retrieving a PyObject type property via QVariant<PyObjectWrapper>

The old code registered a Shiboken converter for PyObjectWrapper by
pointer conversion. This resulted in the Python to C++ converter
falling back to plain pointer passthrough since it only works for
SbkObjects.

The C++ to Python conversion worked by coincidence for either raw
PyObject * pointers used in meta call handling or pointers obtained
from calling QVariant<PyObjectWrapper>.data(), but without handling
reference counts.

To fix this, remove the Python to C++ conversion entirely and do this
manually via QVariant. Change the C++ to Python to be by value and use
PyObjectWrapper.

Fixes: PYSIDE-2193
Pick-to: 6.9
Change-Id: I00898894651f220d7b8fe60608e93233ef3e6493
Reviewed-by: Shyamnath Premnadh <[email protected]>

Author: FriedemannKleint

sources/pyside6/libpyside/pysidemetafunction.cpp

@@ -4,6 +4,8 @@
 #include "pysidemetafunction.h"
 #include "pysidemetafunction_p.h"
 
+#include <signalmanager.h>
+
 #include <autodecref.h>
 #include <basewrapper.h>
 #include <sbkconverter.h>
@@ -12,6 +14,8 @@
 
 #include <QtCore/qmetaobject.h>
 
+using namespace Qt::StringLiterals;
+
 extern "C"
 {
 
@@ -164,6 +168,10 @@ bool call(QObject *self, int methodIndex, PyObject *args, PyObject **retVal)
                 QString tmp;
                 converter.toCpp(obj, &tmp);
                 methValues[i] = tmp;
+            } else if (metaType.id() == PyObjectWrapper::metaTypeId()) {
+                // Manual conversion, see PyObjectWrapper converter registration
+                methValues[i] = QVariant::fromValue(PyObjectWrapper(obj.object()));
+                methArgs[i] = methValues[i].data();
             } else {
                 converter.toCpp(obj, methArgs[i]);
             }

sources/pyside6/libpyside/pysideproperty.cpp

@@ -6,6 +6,7 @@
 #include "pysideproperty_p.h"
 #include "pysidesignal.h"
 #include "pysidesignal_p.h"
+#include "signalmanager.h"
 
 #include <autodecref.h>
 #include <pep384ext.h>
@@ -17,6 +18,8 @@
 
 using namespace Shiboken;
 
+using namespace Qt::StringLiterals;
+
 extern "C"
 {
 
@@ -148,16 +151,20 @@ void PySidePropertyPrivate::metaCall(PyObject *source, QMetaObject::Call call, v
     switch (call) {
     case QMetaObject::ReadProperty: {
         AutoDecRef value(getValue(source));
-        auto *obValue = value.object();
-        if (obValue) {
-            Conversions::SpecificConverter converter(typeName);
-            if (converter) {
-                converter.toCpp(obValue, args[0]);
-            } else {
-                // PYSIDE-2160: Report an unknown type name to the caller `qtPropertyMetacall`.
-                PyErr_SetObject(PyExc_StopIteration, obValue);
-            }
+        if (value.isNull())
+            return;
+        if (typeName == "PyObject"_ba) {
+            // Manual conversion, see PyObjectWrapper converter registration
+            auto *pw = reinterpret_cast<PySide::PyObjectWrapper *>(args[0]);
+            pw->reset(value.object());
+            return;
+        }
+        if (Conversions::SpecificConverter converter(typeName); converter) {
+            converter.toCpp(value.object(), args[0]);
+            return;
         }
+        // PYSIDE-2160: Report an unknown type name to the caller `qtPropertyMetacall`.
+        PyErr_SetObject(PyExc_StopIteration, value.object());
     }
         break;
 

sources/pyside6/libpyside/signalmanager.cpp

@@ -24,6 +24,7 @@
 #include <QtCore/qcoreevent.h>
 #include <QtCore/qdebug.h>
 #include <QtCore/qhash.h>
+#include <QtCore/qmetatype.h>
 #include <QtCore/qscopedpointer.h>
 
 #include <climits>
@@ -38,6 +39,8 @@ using namespace Qt::StringLiterals;
 
 static PyObject *metaObjectAttr = nullptr;
 
+static int pyObjectWrapperMetaTypeId = QMetaType::UnknownType;
+
 static void destroyMetaObject(PyObject *obj)
 {
     void *ptr = PyCapsule_GetPointer(obj, nullptr);
@@ -169,6 +172,10 @@ PyObjectWrapper::operator PyObject *() const
     return m_me;
 }
 
+int PyObjectWrapper::metaTypeId()
+{
+    return pyObjectWrapperMetaTypeId;
+}
 
 int PyObjectWrapper::toInt() const
 {
@@ -273,19 +280,11 @@ struct SignalManagerPrivate
 SignalManager::QmlMetaCallErrorHandler
     SignalManagerPrivate::m_qmlMetaCallErrorHandler = nullptr;
 
-static void PyObject_PythonToCpp_PyObject_PTR(PyObject *pyIn, void *cppOut)
-{
-    *reinterpret_cast<PyObject **>(cppOut) = pyIn;
-}
-static PythonToCppFunc is_PyObject_PythonToCpp_PyObject_PTR_Convertible(PyObject * /* pyIn */)
-{
-    return PyObject_PythonToCpp_PyObject_PTR;
-}
-static PyObject *PyObject_PTR_CppToPython_PyObject(const void *cppIn)
+static PyObject *CopyCppToPythonPyObject(const void *cppIn)
 {
-    auto *pyOut = reinterpret_cast<PyObject *>(const_cast<void *>(cppIn));
-    if (pyOut)
-        Py_INCREF(pyOut);
+    const auto *wrapper = reinterpret_cast<const PyObjectWrapper *>(cppIn);
+    PyObject *pyOut = *wrapper;
+    Py_XINCREF(pyOut);
     return pyOut;
 }
 
@@ -295,13 +294,16 @@ void SignalManager::init()
     using namespace Shiboken;
 
     // Register PyObject type to use in queued signal and slot connections
-    qRegisterMetaType<PyObjectWrapper>("PyObject");
+    pyObjectWrapperMetaTypeId = qRegisterMetaType<PyObjectWrapper>("PyObject");
     // Register QVariant(enum) conversion to QVariant(int)
     QMetaType::registerConverter<PyObjectWrapper, int>(&PyObjectWrapper::toInt);
 
-    SbkConverter *converter = Shiboken::Conversions::createConverter(&PyBaseObject_Type, nullptr);
-    Shiboken::Conversions::setCppPointerToPythonFunction(converter, PyObject_PTR_CppToPython_PyObject);
-    Shiboken::Conversions::setPythonToCppPointerFunctions(converter, PyObject_PythonToCpp_PyObject_PTR, is_PyObject_PythonToCpp_PyObject_PTR_Convertible);
+    // Register a shiboken converter for PyObjectWrapper->Python (value conversion).
+    // Python->PyObjectWrapper is not registered since the converters do not work for
+    // non-SbkObject types (falling back to plain pointer pass through).
+    // This conversion needs to be done manually via QVariant.
+    SbkConverter *converter = Shiboken::Conversions::createConverter(&PyBaseObject_Type,
+                                                                     CopyCppToPythonPyObject);
     Shiboken::Conversions::registerConverterName(converter, "PyObject");
     Shiboken::Conversions::registerConverterName(converter, "object");
     Shiboken::Conversions::registerConverterName(converter, "PyObjectWrapper");

sources/pyside6/libpyside/signalmanager.h

@@ -44,6 +44,8 @@ class PYSIDE_API PyObjectWrapper
     // The proper fix would be to associate PyObjectWrapper to the corresponding C++ Enum.
     int toInt() const;
 
+    static int metaTypeId();
+
 private:
     PyObject* m_me;
 };

sources/pyside6/tests/QtCore/qobject_property_test.py

@@ -32,6 +32,26 @@ def writeP(self, v):
     myProperty = Property(int, readP, fset=writeP, notify=notifyP)
 
 
+class OtherClass:
+    """Helper for QObjectWithOtherClassPropertyTest."""
+    pass
+
+
+class MyObjectWithOtherClassProperty(QObject):
+    """Helper for QObjectWithOtherClassPropertyTest."""
+    def __init__(self, parent=None):
+        super().__init__(parent)
+        self._otherclass = None
+
+    def _get_otherclass(self):
+        return self._otherclass
+
+    def _set_otherclass(self, o):
+        self._otherclass = o
+
+    otherclass = Property(OtherClass, fget=_get_otherclass, fset=_set_otherclass)
+
+
 class PropertyWithNotify(unittest.TestCase):
     def called(self):
         self.called_ = True
@@ -50,5 +70,19 @@ def testHasProperty(self):
         self.assertEqual(o.property("myProperty"), 10)
 
 
+class QObjectWithOtherClassPropertyTest(unittest.TestCase):
+    """PYSIDE-2193: For properties of custom classes not wrapped by shiboken,
+       QVariant<PyObjectWrapper> is used, which had refcount issues causing crashes.
+       Exercise the QVariant conversion by setting and retrieving via the
+       QVariant-based property()/setProperty() API."""
+    def testNotify(self):
+        obj = MyObjectWithOtherClassProperty()
+        obj.setProperty("otherclass", OtherClass())
+        for i in range(10):
+            pv = obj.property("otherclass")
+            print(pv)  # Exercise repr
+            self.assertTrue(type(pv) is OtherClass)
+
+
 if __name__ == '__main__':
     unittest.main()