QDataStream: unify deserialization of containers

Serialization of the Qt container classes is accomplished by breaking
up the data into primitive units. On the receiver side, these units
should be read atomically to guarantee integrity of the container.

Deserialization procedures for QHash and QMap were already implemented
in accordance with this strategy and have the following behavior:

 - a previously latched error status is saved for the caller. This
   overrides possible different errors in the current read. This
   is consistent with the treatment of primitive types.
 - if an error occurs during the deserialization, the container is
   cleared.

To make the API consistent, this patch adjusts the behavior of QList,
QLinkedList, QVector, and QSet deserialization. On the implementation
side we accomplish this with a private StreamStateSaver RAII class that
consolidates the handling of the stream status for all containers.

[ChangeLog][Important Behavior Changes][QtCore][QDataStream] Incomplete
reads of Qt containers are now handled same way as for primitive types,
meaning that previous errors are latched.

Task-number: QTBUG-54022
Change-Id: I5c77257fe2a4637e8a7e6cf3cd43091c8469340e
Reviewed-by: Oswald Buddenhagen <oswald.buddenhagen@theqtcompany.com>
Reviewed-by: Edward Welbourne <edward.welbourne@qt.io>

2 files changed, 102 insertions, 23 deletions

diff --git a/src/corelib/io/qdatastream.h b/src/corelib/io/qdatastream.h
index e0a9cccdad..260dd519e3 100644
--- a/src/corelib/io/qdatastream.h
+++ b/src/corelib/io/qdatastream.h
@@ -194,6 +194,29 @@ private:
     int readBlock(char *data, int len);
 };
 
+namespace QtPrivate {
+
+class StreamStateSaver
+{
+public:
+    inline StreamStateSaver(QDataStream *s) : stream(s), oldStatus(s->status())
+    {
+        stream->resetStatus();
+    }
+    inline ~StreamStateSaver()
+    {
+        if (oldStatus != QDataStream::Ok) {
+            stream->resetStatus();
+            stream->setStatus(oldStatus);
+        }
+    }
+
+private:
+    QDataStream *stream;
+    QDataStream::Status oldStatus;
+};
+
+} // QtPrivate namespace
 
 /*****************************************************************************
   QDataStream inline functions
@@ -238,6 +261,8 @@ inline QDataStream &QDataStream::operator<<(quint64 i)
 template <typename T>
 QDataStream& operator>>(QDataStream& s, QList<T>& l)
 {
+    QtPrivate::StreamStateSaver stateSaver(&s);
+
     l.clear();
     quint32 c;
     s >> c;
@@ -246,10 +271,13 @@ QDataStream& operator>>(QDataStream& s, QList<T>& l)
     {
         T t;
         s >> t;
-        l.append(t);
-        if (s.atEnd())
+        if (s.status() != QDataStream::Ok) {
+            l.clear();
             break;
+        }
+        l.append(t);
     }
+
     return s;
 }
 
@@ -265,6 +293,8 @@ QDataStream& operator<<(QDataStream& s, const QList<T>& l)
 template <typename T>
 QDataStream& operator>>(QDataStream& s, QLinkedList<T>& l)
 {
+    QtPrivate::StreamStateSaver stateSaver(&s);
+
     l.clear();
     quint32 c;
     s >> c;
@@ -272,10 +302,13 @@ QDataStream& operator>>(QDataStream& s, QLinkedList<T>& l)
     {
         T t;
         s >> t;
-        l.append(t);
-        if (s.atEnd())
+        if (s.status() != QDataStream::Ok) {
+            l.clear();
             break;
+        }
+        l.append(t);
     }
+
     return s;
 }
 
@@ -292,6 +325,8 @@ QDataStream& operator<<(QDataStream& s, const QLinkedList<T>& l)
 template<typename T>
 QDataStream& operator>>(QDataStream& s, QVector<T>& v)
 {
+    QtPrivate::StreamStateSaver stateSaver(&s);
+
     v.clear();
     quint32 c;
     s >> c;
@@ -299,8 +334,13 @@ QDataStream& operator>>(QDataStream& s, QVector<T>& v)
     for(quint32 i = 0; i < c; ++i) {
         T t;
         s >> t;
+        if (s.status() != QDataStream::Ok) {
+            v.clear();
+            break;
+        }
         v[i] = t;
     }
+
     return s;
 }
 
@@ -316,16 +356,21 @@ QDataStream& operator<<(QDataStream& s, const QVector<T>& v)
 template <typename T>
 QDataStream &operator>>(QDataStream &in, QSet<T> &set)
 {
+    QtPrivate::StreamStateSaver stateSaver(&in);
+
     set.clear();
     quint32 c;
     in >> c;
     for (quint32 i = 0; i < c; ++i) {
         T t;
         in >> t;
-        set << t;
-        if (in.atEnd())
+        if (in.status() != QDataStream::Ok) {
+            set.clear();
             break;
+        }
+        set << t;
     }
+
     return in;
 }
 
@@ -344,10 +389,9 @@ QDataStream& operator<<(QDataStream &out, const QSet<T> &set)
 template <class Key, class T>
 Q_OUTOFLINE_TEMPLATE QDataStream &operator>>(QDataStream &in, QHash<Key, T> &hash)
 {
-    QDataStream::Status oldStatus = in.status();
-    in.resetStatus();
-    hash.clear();
+    QtPrivate::StreamStateSaver stateSaver(&in);
 
+    hash.clear();
     quint32 n;
     in >> n;
 
@@ -363,10 +407,6 @@ Q_OUTOFLINE_TEMPLATE QDataStream &operator>>(QDataStream &in, QHash<Key, T> &has
 
     if (in.status() != QDataStream::Ok)
         hash.clear();
-    if (oldStatus != QDataStream::Ok) {
-        in.resetStatus();
-        in.setStatus(oldStatus);
-    }
     return in;
 }
 
@@ -390,10 +430,9 @@ template <class aKey, class aT>
 Q_OUTOFLINE_TEMPLATE QDataStream &operator>>(QDataStream &in, QMap<aKey, aT> &map)
 #endif
 {
-    QDataStream::Status oldStatus = in.status();
-    in.resetStatus();
-    map.clear();
+    QtPrivate::StreamStateSaver stateSaver(&in);
 
+    map.clear();
     quint32 n;
     in >> n;
 
@@ -409,10 +448,6 @@ Q_OUTOFLINE_TEMPLATE QDataStream &operator>>(QDataStream &in, QMap<aKey, aT> &ma
     }
     if (in.status() != QDataStream::Ok)
         map.clear();
-    if (oldStatus != QDataStream::Ok) {
-        in.resetStatus();
-        in.setStatus(oldStatus);
-    }
     return in;
 }
 
diff --git a/tests/auto/corelib/io/qdatastream/tst_qdatastream.cpp b/tests/auto/corelib/io/qdatastream/tst_qdatastream.cpp
index ea4c557574..fefe08db30 100644
--- a/tests/auto/corelib/io/qdatastream/tst_qdatastream.cpp
+++ b/tests/auto/corelib/io/qdatastream/tst_qdatastream.cpp
@@ -2813,10 +2813,11 @@ void tst_QDataStream::status_QHash_QMap()
     MAP_TEST(QByteArray("\x00\x00\x00\x01\x00\x00\x00\x01", 8), QDataStream::ReadPastEnd, QDataStream::ReadPastEnd, StringHash());
 }
 
-#define LIST_TEST(byteArray, expectedStatus, expectedList) \
+#define LIST_TEST(byteArray, initialStatus, expectedStatus, expectedList) \
     { \
         QByteArray ba = byteArray; \
         QDataStream stream(&ba, QIODevice::ReadOnly); \
+        stream.setStatus(initialStatus); \
         stream >> list; \
         QCOMPARE((int)stream.status(), (int)expectedStatus); \
         QCOMPARE(list.size(), expectedList.size()); \
@@ -2828,6 +2829,7 @@ void tst_QDataStream::status_QHash_QMap()
             expectedLinkedList << expectedList.at(i); \
         QByteArray ba = byteArray; \
         QDataStream stream(&ba, QIODevice::ReadOnly); \
+        stream.setStatus(initialStatus); \
         stream >> linkedList; \
         QCOMPARE((int)stream.status(), (int)expectedStatus); \
         QCOMPARE(linkedList.size(), expectedLinkedList.size()); \
@@ -2839,6 +2841,7 @@ void tst_QDataStream::status_QHash_QMap()
             expectedVector << expectedList.at(i); \
         QByteArray ba = byteArray; \
         QDataStream stream(&ba, QIODevice::ReadOnly); \
+        stream.setStatus(initialStatus); \
         stream >> vector; \
         QCOMPARE((int)stream.status(), (int)expectedStatus); \
         QCOMPARE(vector.size(), expectedVector.size()); \
@@ -2854,8 +2857,49 @@ void tst_QDataStream::status_QLinkedList_QList_QVector()
     List list;
     Vector vector;
 
-    LIST_TEST(QByteArray(), QDataStream::ReadPastEnd, List());
-    LIST_TEST(QByteArray("\x00\x00\x00\x00", 4), QDataStream::Ok, List());
+    // ok
+    {
+        List listWithEmptyString;
+        listWithEmptyString.append("");
+
+        List someList;
+        someList.append("J");
+        someList.append("MN");
+
+        LIST_TEST(QByteArray("\x00\x00\x00\x00", 4), QDataStream::Ok, QDataStream::Ok, List());
+        LIST_TEST(QByteArray("\x00\x00\x00\x01\x00\x00\x00\x00", 8), QDataStream::Ok, QDataStream::Ok, listWithEmptyString);
+        LIST_TEST(QByteArray("\x00\x00\x00\x02\x00\x00\x00\x02\x00J"
+                             "\x00\x00\x00\x04\x00M\x00N", 18), QDataStream::Ok, QDataStream::Ok, someList);
+    }
+
+    // past end
+    {
+        LIST_TEST(QByteArray(), QDataStream::Ok, QDataStream::ReadPastEnd, List());
+        LIST_TEST(QByteArray("\x00", 1), QDataStream::Ok, QDataStream::ReadPastEnd, List());
+        LIST_TEST(QByteArray("\x00\x00", 2), QDataStream::Ok, QDataStream::ReadPastEnd, List());
+        LIST_TEST(QByteArray("\x00\x00\x00", 3), QDataStream::Ok, QDataStream::ReadPastEnd, List());
+        LIST_TEST(QByteArray("\x00\x00\x00\x01", 4), QDataStream::Ok, QDataStream::ReadPastEnd, List());
+        for (int i = 4; i < 12; ++i) {
+            LIST_TEST(QByteArray("\x00\x00\x00\x02\x00\x00\x00\x00\x00\x00\x00\x00", i), QDataStream::Ok, QDataStream::ReadPastEnd, List());
+        }
+    }
+
+    // corrupt data
+    {
+        LIST_TEST(QByteArray("\x00\x00\x00\x01\x00\x00\x00\x01", 8), QDataStream::Ok, QDataStream::ReadCorruptData, List());
+        LIST_TEST(QByteArray("\x00\x00\x00\x02\x00\x00\x00\x01\x00J"
+                             "\x00\x00\x00\x02\x00M\x00N", 18), QDataStream::Ok, QDataStream::ReadCorruptData, List());
+    }
+
+    // test the previously latched error status is not affected by reading
+    {
+        List listWithEmptyString;
+        listWithEmptyString.append("");
+
+        LIST_TEST(QByteArray("\x00\x00\x00\x01\x00\x00\x00\x00", 8), QDataStream::ReadPastEnd, QDataStream::ReadPastEnd, listWithEmptyString);
+        LIST_TEST(QByteArray("\x00\x00\x00\x01", 4), QDataStream::ReadCorruptData, QDataStream::ReadCorruptData, List());
+        LIST_TEST(QByteArray("\x00\x00\x00\x01\x00\x00\x00\x01", 8), QDataStream::ReadPastEnd, QDataStream::ReadPastEnd, List());
+    }
 }
 
 void tst_QDataStream::streamToAndFromQByteArray()