Fix crash in `QVariant::convert` and `QVariant::view`

`QVariant::convert` may lead to crash or produce garbage data when
attempting to convert a gadget between a pointer type and a value type,
for example from a variant holding a QLocale gadget to a QLocale*
pointer and vice versa. Similarly, `QVariant::view` may crash under the
same conditions.

The reason is that conversion is implemented through copy construction
assuming that both source and target types are either both pointers or
both values. If converting from pointer to value type, the result is
crash during destruction of the QVariant. If converting from value to
pointer type, the result is a QVariant holding a pointer to garbage
data (and possibly crash if pointer is dereferenced).

Similarly, if attempting to convert a pointer to a QObject derived type
to its value type, the system crashes, with a slightly different failure
mode. During `QVariant::convert`, a temporary `QVariant` of the target
type is created. Since objects that can not be copy constructed are
invalid for `QVariant`, the temporary is left empty without constructing
the target value. Then, when attempting to convert from a pointer type
to a value type, the temporary's destructor is incorrectly called on the
owned object. Since the owned object is never constructed, this leads to
a crash.

The proposed fix is to return false from `QMetaType::view`,
`QMetaType::canView`, `QMetaType::convert`, and `QMetaType::canConvert`
if the target type is of different 'pointedness' than the source type.

After this fix, converting and viewing gadgets and QObjects behaves the
same way as primitive types and core types, which already returned false
when converting between value type and pointer type.

Fixes: QTBUG-114797
Pick-to: 6.5 6.6
Change-Id: If5ad764a60f2f3c912070198073b28999d995f17
Reviewed-by: Ulf Hermann <ulf.hermann@qt.io>
Reviewed-by: Mårten Nordheim <marten.nordheim@qt.io>

2 files changed, 149 insertions, 2 deletions

diff --git a/src/corelib/kernel/qmetatype.cpp b/src/corelib/kernel/qmetatype.cpp
index dbdd5b21ef..36dd4f5ef5 100644
--- a/src/corelib/kernel/qmetatype.cpp
+++ b/src/corelib/kernel/qmetatype.cpp
@@ -2233,6 +2233,9 @@ static bool convertToAssociativeIterable(QMetaType fromType, const void *from, v
 
 static bool canConvertMetaObject(QMetaType fromType, QMetaType toType)
 {
+    if ((fromType.flags() & QMetaType::IsPointer) != (toType.flags() & QMetaType::IsPointer))
+        return false; // Can not convert between pointer and value
+
     const QMetaObject *f = fromType.metaObject();
     const QMetaObject *t = toType.metaObject();
     if (f && t) {
@@ -2303,7 +2306,8 @@ static bool convertMetaObject(QMetaType fromType, const void *from, QMetaType to
             *static_cast<void **>(to) = nullptr;
             return fromType.metaObject()->inherits(toType.metaObject());
         }
-    } else {
+    } else if ((fromType.flags() & QMetaType::IsPointer) == (toType.flags() & QMetaType::IsPointer)) {
+        // fromType and toType are of same 'pointedness'
         const QMetaObject *f = fromType.metaObject();
         const QMetaObject *t = toType.metaObject();
         if (f && t && f->inherits(t)) {
diff --git a/tests/auto/corelib/kernel/qvariant/tst_qvariant.cpp b/tests/auto/corelib/kernel/qvariant/tst_qvariant.cpp
index d1d47b329b..d8ffe32b60 100644
--- a/tests/auto/corelib/kernel/qvariant/tst_qvariant.cpp
+++ b/tests/auto/corelib/kernel/qvariant/tst_qvariant.cpp
@@ -174,6 +174,12 @@ private slots:
 
     void canConvert_data();
     void canConvert();
+
+    void canConvertAndConvert_ReturnFalse_WhenConvertingBetweenPointerAndValue_data();
+    void canConvertAndConvert_ReturnFalse_WhenConvertingBetweenPointerAndValue();
+
+    void canConvertAndConvert_ReturnFalse_WhenConvertingQObjectBetweenPointerAndValue();
+
     void convert();
 
     void toSize_data();
@@ -371,6 +377,8 @@ private slots:
     void preferDirectConversionOverInterfaces();
     void mutableView();
 
+    void canViewAndView_ReturnFalseAndDefault_WhenConvertingBetweenPointerAndValue();
+
     void moveOperations();
     void equalsWithoutMetaObject();
 
@@ -402,6 +410,8 @@ private:
     void getIf_impl(T t) const;
     template <typename T>
     void get_impl(T t) const;
+    template<typename T>
+    void canViewAndView_ReturnFalseAndDefault_WhenConvertingBetweenPointerAndValue_impl(const QByteArray &typeName);
     void dataStream_data(QDataStream::Version version);
     void loadQVariantFromDataStream(QDataStream::Version version);
     void saveQVariantFromDataStream(QDataStream::Version version);
@@ -690,6 +700,100 @@ QT_WARNING_POP
 #endif // QT_DEPRECATED_SINCE(6, 0)
 }
 
+namespace {
+
+// Used for testing canConvert/convert of QObject derived types
+struct QObjectDerived : QObject
+{
+    Q_OBJECT
+};
+
+// Adds a test table row for checking value <-> pointer conversion
+// If type is a pointer, the target type is value type and vice versa.
+template<typename T>
+void addRowForPointerValueConversion()
+{
+    using ValueType = std::remove_pointer_t<T>;
+    if constexpr (!std::is_same_v<ValueType, std::nullptr_t>) {
+
+        static ValueType instance{}; // static since we may need a pointer to a valid object
+
+        QVariant variant;
+        if constexpr (std::is_pointer_v<T>)
+            variant = QVariant::fromValue(&instance);
+        else
+            variant = QVariant::fromValue(instance);
+
+        // Toggle pointer/value type
+        using TargetType = std::conditional_t<std::is_pointer_v<T>, ValueType, T *>;
+
+        const QMetaType fromType = QMetaType::fromType<T>();
+        const QMetaType toType = QMetaType::fromType<TargetType>();
+
+        QTest::addRow("%s->%s", fromType.name(), toType.name())
+                << variant << QMetaType::fromType<TargetType>();
+    }
+}
+
+} // namespace
+
+void tst_QVariant::canConvertAndConvert_ReturnFalse_WhenConvertingBetweenPointerAndValue_data()
+{
+    QTest::addColumn<QVariant>("variant");
+    QTest::addColumn<QMetaType>("targetType");
+
+#define ADD_ROW(typeName, typeNameId, realType)  \
+    addRowForPointerValueConversion<realType>(); \
+    addRowForPointerValueConversion<realType *>();
+
+    // Add rows for static primitive types
+    QT_FOR_EACH_STATIC_PRIMITIVE_NON_VOID_TYPE(ADD_ROW)
+
+    // Add rows for static core types
+    QT_FOR_EACH_STATIC_CORE_CLASS(ADD_ROW)
+#undef ADD_ROW
+
+}
+
+void tst_QVariant::canConvertAndConvert_ReturnFalse_WhenConvertingBetweenPointerAndValue()
+{
+    QFETCH(QVariant, variant);
+    QFETCH(QMetaType, targetType);
+
+    QVERIFY(!variant.canConvert(targetType));
+
+    QVERIFY(!variant.convert(targetType));
+
+    // As per the documentation, when QVariant::convert fails, the
+    // QVariant is cleared and changed to the requested type.
+    QVERIFY(variant.isNull());
+    QCOMPARE(variant.metaType(), targetType);
+}
+
+void tst_QVariant::canConvertAndConvert_ReturnFalse_WhenConvertingQObjectBetweenPointerAndValue()
+{
+    // Types derived from QObject are non-copyable and require their own test.
+    // We only test pointer -> value conversion, because constructing a QVariant
+    // from a non-copyable object will just set the QVariant to null.
+
+    QObjectDerived object;
+    QVariant variant = QVariant::fromValue(&object);
+
+    constexpr QMetaType targetType = QMetaType::fromType<QObjectDerived>();
+    QVERIFY(!variant.canConvert(targetType));
+
+    QTest::ignoreMessage(
+            QtWarningMsg,
+            QRegularExpression(".*does not support destruction and copy construction"));
+
+    QVERIFY(!variant.convert(targetType));
+
+    // When the QVariant::convert fails, the QVariant is cleared, and since the target type is
+    // invalid for QVariant, the QVariant's type is also cleared to an unknown type.
+    QVERIFY(variant.isNull());
+    QCOMPARE(variant.metaType(), QMetaType());
+}
+
 void tst_QVariant::convert()
 {
    // verify that after convert(), the variant's type has been changed
@@ -699,7 +803,6 @@ void tst_QVariant::convert()
    QCOMPARE(var.toInt(), 0);
 }
 
-
 void tst_QVariant::toInt_data()
 {
     QTest::addColumn<QVariant>("value");
@@ -5618,6 +5721,38 @@ void tst_QVariant::mutableView()
     QCOMPARE(extracted.text, nullptr);
 }
 
+template<typename T>
+void tst_QVariant::canViewAndView_ReturnFalseAndDefault_WhenConvertingBetweenPointerAndValue_impl(
+        const QByteArray &typeName)
+{
+    T instance{};
+
+    // Value -> Pointer
+    QVariant value = QVariant::fromValue(instance);
+    QVERIFY2(!value.canView<T *>(), typeName);
+    QCOMPARE(value.view<T *>(), nullptr); // Expect default constructed pointer
+
+    // Pointer -> Value
+    QVariant pointer = QVariant::fromValue(&instance);
+    QVERIFY2(!pointer.canView<T>(), typeName);
+    QCOMPARE(pointer.view<T>(), T{}); // Expect default constructed. Note: Weak test since instance
+                                      // is default constructed, but we detect data corruption
+}
+
+void tst_QVariant::canViewAndView_ReturnFalseAndDefault_WhenConvertingBetweenPointerAndValue()
+{
+#define ADD_TEST_IMPL(typeName, typeNameId, realType)                                         \
+    canViewAndView_ReturnFalseAndDefault_WhenConvertingBetweenPointerAndValue_impl<realType>( \
+            #typeName);
+
+    // Add tests for static primitive types
+    QT_FOR_EACH_STATIC_PRIMITIVE_NON_VOID_TYPE(ADD_TEST_IMPL)
+
+    // Add tests for static core types
+    QT_FOR_EACH_STATIC_CORE_CLASS(ADD_TEST_IMPL)
+#undef ADD_TEST_IMPL
+}
+
 struct MoveTester
 {
     bool wasMoved = false;
@@ -5743,6 +5878,13 @@ private:
     ~Indestructible() {}
 };
 
+struct NotCopyable
+{
+    NotCopyable() = default;
+    NotCopyable(const NotCopyable&) = delete;
+    NotCopyable &operator=(const NotCopyable &) = delete;
+};
+
 void tst_QVariant::constructFromIncompatibleMetaType_data()
 {
     QTest::addColumn<QMetaType>("type");
@@ -5753,6 +5895,7 @@ void tst_QVariant::constructFromIncompatibleMetaType_data()
     addRow(QMetaType::fromType<NonDefaultConstructible>());
     addRow(QMetaType::fromType<QObject>());
     addRow(QMetaType::fromType<Indestructible>());
+    addRow(QMetaType::fromType<NotCopyable>());
 }
 
 void tst_QVariant::constructFromIncompatibleMetaType()