Fix crashes and non-portable functionality in QDBusDemarshaller QByteArray extraction

QDBusArgument QByteArray extraction operator and QDBusDemarshaller that
implements the extraction do not check the type of the extracted value.
When extracting a QByteArray when the value actually is e.g. a struct of
mixed types the byte array extraction will crash as it attempts to extract
the struct data as a fixed array.

The fix adds DBus type checks to QDBusArgument byte array extraction
operator implementations.
The checks invalidate extracting arrays of other types than bytes to a
QByteArray that worked with the unchecked implementation. The rationale
for this restriction is
1) extracting a QByteArray to a variant checks already that the array
   element type is byte
2) Results of extracting arrays of types wider than a byte to a QByteArray
   are architecture-dependent making such code inherently non-portable.

Task-number: QTBUG-22840
Change-Id: Ie20f2adc06c697a68055c803215fb408568fdd90
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>

diff --git a/src/dbus/qdbusargument_p.h b/src/dbus/qdbusargument_p.h
index df2b2a0..4327e74 100644 (file)
--- a/src/dbus/qdbusargument_p.h
+++ b/src/dbus/qdbusargument_p.h
@@ -213,6 +213,7 @@ private:
     QDBusObjectPath toObjectPathUnchecked();
     QDBusSignature toSignatureUnchecked();
     QStringList toStringListUnchecked();
+    QByteArray toByteArrayUnchecked();
 };
 
 inline QDBusMarshaller *QDBusArgumentPrivate::marshaller()

diff --git a/src/dbus/qdbusdemarshaller.cpp b/src/dbus/qdbusdemarshaller.cpp
index b7e363a..96729fd 100644 (file)
--- a/src/dbus/qdbusdemarshaller.cpp
+++ b/src/dbus/qdbusdemarshaller.cpp
@@ -272,7 +272,7 @@ QVariant QDBusDemarshaller::toVariantInternal()
         switch (q_dbus_message_iter_get_element_type(&iterator)) {
         case DBUS_TYPE_BYTE:
             // QByteArray
-            return toByteArray();
+            return toByteArrayUnchecked();
         case DBUS_TYPE_STRING:
             return toStringListUnchecked();
         case DBUS_TYPE_DICT_ENTRY:
@@ -339,7 +339,7 @@ QStringList QDBusDemarshaller::toStringList()
         return QStringList();
 }
 
-QByteArray QDBusDemarshaller::toByteArray()
+QByteArray QDBusDemarshaller::toByteArrayUnchecked()
 {
     DBusMessageIter sub;
     q_dbus_message_iter_recurse(&iterator, &sub);
@@ -350,6 +350,15 @@ QByteArray QDBusDemarshaller::toByteArray()
     return QByteArray(data,len);
 }
 
+QByteArray QDBusDemarshaller::toByteArray()
+{
+    if (q_dbus_message_iter_get_arg_type(&iterator) == DBUS_TYPE_ARRAY
+            && q_dbus_message_iter_get_element_type(&iterator) == DBUS_TYPE_BYTE) {
+        return toByteArrayUnchecked();
+    }
+    return QByteArray();
+}
+
 bool QDBusDemarshaller::atEnd()
 {
     // dbus_message_iter_has_next is broken if the list has one single element

diff --git a/tests/auto/dbus/qdbusmarshall/tst_qdbusmarshall.cpp b/tests/auto/dbus/qdbusmarshall/tst_qdbusmarshall.cpp
index 8ce456b..1886c8f 100644 (file)
--- a/tests/auto/dbus/qdbusmarshall/tst_qdbusmarshall.cpp
+++ b/tests/auto/dbus/qdbusmarshall/tst_qdbusmarshall.cpp
@@ -102,6 +102,9 @@ private slots:
     void demarshallInvalidStringList_data();
     void demarshallInvalidStringList();
 
+    void demarshallInvalidByteArray_data();
+    void demarshallInvalidByteArray();
+
 private:
     int fileDescriptorForTest();
 
@@ -1427,5 +1430,55 @@ void tst_QDBusMarshall::demarshallInvalidStringList()
     QVERIFY(receiveArg.atEnd());
 }
 
+void tst_QDBusMarshall::demarshallInvalidByteArray_data()
+{
+    addBasicTypesColumns();
+
+    // None of the basic types should demarshall to a QByteArray
+    basicNumericTypes_data();
+    basicStringTypes_data();
+
+    // Arrays of other types than byte should not demarshall to a QByteArray
+    QList<bool> bools;
+    QTest::newRow("empty array of bool") << qVariantFromValue(bools);
+    bools << true << false << true;
+    QTest::newRow("non-empty array of bool") << qVariantFromValue(bools);
+
+    // Structures should not demarshall to a QByteArray
+    QTest::newRow("struct of bytes")
+            << qVariantFromValue(QVariantList() << uchar(1) << uchar(2));
+
+    QTest::newRow("struct of mixed types")
+            << qVariantFromValue(QVariantList() << int(42) << QString("foo") << double(3.14));
+}
+
+void tst_QDBusMarshall::demarshallInvalidByteArray()
+{
+    QFETCH(QVariant, value);
+
+    QDBusConnection con = QDBusConnection::sessionBus();
+
+    QVERIFY(con.isConnected());
+
+    QDBusMessage msg = QDBusMessage::createMethodCall(serviceName, objectPath,
+                                                      interfaceName, "ping");
+    QDBusArgument sendArg;
+    sendArg.beginStructure();
+    sendArg.appendVariant(value);
+    sendArg.endStructure();
+    msg.setArguments(QVariantList() << qVariantFromValue(sendArg));
+    QDBusMessage reply = con.call(msg);
+
+    const QDBusArgument receiveArg = qvariant_cast<QDBusArgument>(reply.arguments().at(0));
+    receiveArg.beginStructure();
+
+    QByteArray receiveValue;
+    receiveArg >> receiveValue;
+    QCOMPARE(receiveValue, QByteArray());
+
+    receiveArg.endStructure();
+    QVERIFY(receiveArg.atEnd());
+}
+
 QTEST_MAIN(tst_QDBusMarshall)
 #include "tst_qdbusmarshall.moc"