http2frame - do not rely on a socket's buffer

- Whenever we have a read notification, read this data, even if it's not
  enough for a frame's header - otherwise, QNAM can use a socket
  in an Ubuffered mode and FrameReader can potentially fail to read
  anything correctly.

- Do not call/rely on bytesAvailable and do not try to invoke
  _q_receiveReply in Qt::QueuedConnection mode, instead try to read
  until we end up in an incomplete frame or some error.

Change-Id: I7f44ba9e34bc64f3e26bd29080f0050da635b3ae
Reviewed-by: Alex Trotsenko <alex1973tr@gmail.com>
Reviewed-by: Edward Welbourne <edward.welbourne@qt.io>

3 files changed, 115 insertions, 85 deletions

diff --git a/src/network/access/http2/http2frames.cpp b/src/network/access/http2/http2frames.cpp
index 55e9f93b19..95f00dd286 100644
--- a/src/network/access/http2/http2frames.cpp
+++ b/src/network/access/http2/http2frames.cpp
@@ -148,7 +148,7 @@ FrameStatus validate_frame_payload(FrameType type, FrameFlags flags,
 
 
 FrameReader::FrameReader(FrameReader &&rhs)
-    : framePayload(std::move(rhs.framePayload))
+    : frameBuffer(std::move(rhs.frameBuffer))
 {
     type = rhs.type;
     rhs.type = FrameType::LAST_FRAME_TYPE;
@@ -162,8 +162,8 @@ FrameReader::FrameReader(FrameReader &&rhs)
     payloadSize = rhs.payloadSize;
     rhs.payloadSize = 0;
 
-    incompleteRead = rhs.incompleteRead;
-    rhs.incompleteRead = false;
+    state = rhs.state;
+    rhs.state = Idle;
 
     offset = rhs.offset;
     rhs.offset = 0;
@@ -171,7 +171,7 @@ FrameReader::FrameReader(FrameReader &&rhs)
 
 FrameReader &FrameReader::operator = (FrameReader &&rhs)
 {
-    framePayload = std::move(rhs.framePayload);
+    frameBuffer = std::move(rhs.frameBuffer);
 
     type = rhs.type;
     rhs.type = FrameType::LAST_FRAME_TYPE;
@@ -185,8 +185,8 @@ FrameReader &FrameReader::operator = (FrameReader &&rhs)
     payloadSize = rhs.payloadSize;
     rhs.payloadSize = 0;
 
-    incompleteRead = rhs.incompleteRead;
-    rhs.incompleteRead = false;
+    state = rhs.state;
+    rhs.state = Idle;
 
     offset = rhs.offset;
     rhs.offset = 0;
@@ -196,10 +196,13 @@ FrameReader &FrameReader::operator = (FrameReader &&rhs)
 
 FrameStatus FrameReader::read(QAbstractSocket &socket)
 {
-    if (!incompleteRead) {
+    if (state != ReadingPayload) {
+        // Either Idle or ReadingHeader:
         if (!readHeader(socket))
             return FrameStatus::incompleteFrame;
 
+        Q_ASSERT(state == Idle);
+
         const auto status = validate_frame_header(type, flags, payloadSize);
         if (status != FrameStatus::goodFrame) {
             // No need to read any payload.
@@ -209,16 +212,17 @@ FrameStatus FrameReader::read(QAbstractSocket &socket)
         if (Http2PredefinedParameters::maxFrameSize < payloadSize)
             return FrameStatus::sizeError;
 
-        framePayload.resize(payloadSize);
+        frameBuffer.resize(payloadSize);
         offset = 0;
     }
 
-    if (framePayload.size()) {
+    if (frameBuffer.size()) {
         if (!readPayload(socket))
             return FrameStatus::incompleteFrame;
+        Q_ASSERT(state == Idle);
     }
 
-    return validate_frame_payload(type, flags, framePayload);
+    return validate_frame_payload(type, flags, frameBuffer);
 }
 
 bool FrameReader::padded(uchar *pad) const
@@ -231,8 +235,8 @@ bool FrameReader::padded(uchar *pad) const
     if (type == FrameType::DATA
         || type == FrameType::PUSH_PROMISE
         || type == FrameType::HEADERS) {
-        Q_ASSERT(framePayload.size() >= 1);
-        *pad = framePayload[0];
+        Q_ASSERT(frameBuffer.size() >= 1);
+        *pad = frameBuffer[0];
         return true;
     }
 
@@ -244,10 +248,10 @@ bool FrameReader::priority(quint32 *streamID, uchar *weight) const
     Q_ASSERT(streamID);
     Q_ASSERT(weight);
 
-    if (!framePayload.size())
+    if (!frameBuffer.size())
         return false;
 
-    const uchar *src = &framePayload[0];
+    const uchar *src = &frameBuffer[0];
     if (type == FrameType::HEADERS && flags.testFlag(FrameFlag::PADDED))
         ++src;
 
@@ -263,7 +267,7 @@ bool FrameReader::priority(quint32 *streamID, uchar *weight) const
 
 quint32 FrameReader::dataSize() const
 {
-    quint32 size = quint32(framePayload.size());
+    quint32 size = quint32(frameBuffer.size());
     uchar pad = 0;
     if (padded(&pad)) {
         // + 1 one for a byte with padding number itself:
@@ -280,10 +284,10 @@ quint32 FrameReader::dataSize() const
 
 const uchar *FrameReader::dataBegin() const
 {
-    if (!framePayload.size())
+    if (!frameBuffer.size())
         return nullptr;
 
-    const uchar *src = &framePayload[0];
+    const uchar *src = &frameBuffer[0];
     uchar dummyPad = 0;
     if (padded(&dummyPad))
         ++src;
@@ -298,14 +302,30 @@ const uchar *FrameReader::dataBegin() const
 
 bool FrameReader::readHeader(QAbstractSocket &socket)
 {
-    if (socket.bytesAvailable() < frameHeaderSize)
+    Q_ASSERT(state != ReadingPayload);
+
+    if (state == Idle) {
+        offset = 0;
+        frameBuffer.resize(frameHeaderSize);
+        state = ReadingHeader;
+    }
+
+    Q_ASSERT(offset < frameHeaderSize);
+
+    const auto chunkSize = socket.read(reinterpret_cast<char *>(&frameBuffer[offset]),
+                                       frameHeaderSize - offset);
+    if (chunkSize > 0)
+        offset += chunkSize;
+
+    if (offset < frameHeaderSize)
         return false;
 
-    uchar src[frameHeaderSize] = {};
-    socket.read(reinterpret_cast<char*>(src), frameHeaderSize);
+    // We got a complete frame header:
+    state = Idle;
 
-    payloadSize = src[0] << 16 | src[1] << 8 | src[2];
+    const uchar *src = &frameBuffer[0];
 
+    payloadSize = src[0] << 16 | src[1] << 8 | src[2];
     type = FrameType(src[3]);
     if (int(type) >= int(FrameType::LAST_FRAME_TYPE))
         type = FrameType::LAST_FRAME_TYPE; // To be ignored, 5.1
@@ -318,16 +338,23 @@ bool FrameReader::readHeader(QAbstractSocket &socket)
 
 bool FrameReader::readPayload(QAbstractSocket &socket)
 {
-    Q_ASSERT(offset <= framePayload.size());
+    Q_ASSERT(offset < frameBuffer.size());
+    Q_ASSERT(state != ReadingHeader);
+
+    state = ReadingPayload;
 
     // Casts and ugliness - to deal with MSVC. Values are guaranteed to fit into quint32.
-    if (const auto residue = std::min(qint64(framePayload.size() - offset), socket.bytesAvailable())) {
-        socket.read(reinterpret_cast<char *>(&framePayload[offset]), residue);
-        offset += quint32(residue);
-    }
+    const auto residue = qint64(frameBuffer.size() - offset);
+    const auto chunkSize = socket.read(reinterpret_cast<char *>(&frameBuffer[offset]), residue);
+    if (chunkSize > 0)
+        offset += quint32(chunkSize);
 
-    incompleteRead = offset < framePayload.size();
-    return !incompleteRead;
+    if (offset < frameBuffer.size())
+        return false;
+
+    // Complete payload read:
+    state = Idle;
+    return true;
 }
 
 
diff --git a/src/network/access/http2/http2frames_p.h b/src/network/access/http2/http2frames_p.h
index 6abed315ca..c85be57a2e 100644
--- a/src/network/access/http2/http2frames_p.h
+++ b/src/network/access/http2/http2frames_p.h
@@ -101,13 +101,20 @@ private:
     bool readHeader(QAbstractSocket &socket);
     bool readPayload(QAbstractSocket &socket);
 
+    enum ReaderState {
+        Idle,
+        ReadingHeader,
+        ReadingPayload
+    };
+
+    ReaderState state = Idle;
+
     // As soon as we got a header, we
     // know payload size, offset is
     // needed if we do not have enough
     // data and will read the next chunk.
-    bool incompleteRead = false;
     quint32 offset = 0;
-    std::vector<uchar> framePayload;
+    std::vector<uchar> frameBuffer;
 };
 
 class Q_AUTOTEST_EXPORT FrameWriter
diff --git a/src/network/access/qhttp2protocolhandler.cpp b/src/network/access/qhttp2protocolhandler.cpp
index 937686920c..89d4a24e37 100644
--- a/src/network/access/qhttp2protocolhandler.cpp
+++ b/src/network/access/qhttp2protocolhandler.cpp
@@ -170,64 +170,60 @@ void QHttp2ProtocolHandler::_q_receiveReply()
     Q_ASSERT(m_socket);
     Q_ASSERT(m_channel);
 
-    const auto result = inboundFrame.read(*m_socket);
-    switch (result) {
-    case FrameStatus::incompleteFrame:
-        return;
-    case FrameStatus::protocolError:
-        return connectionError(PROTOCOL_ERROR, "invalid frame");
-    case FrameStatus::sizeError:
-        return connectionError(FRAME_SIZE_ERROR, "invalid frame size");
-    default:
-        break;
-    }
-
-    Q_ASSERT(result == FrameStatus::goodFrame);
-
-    if (continuationExpected && inboundFrame.type != FrameType::CONTINUATION)
-        return connectionError(PROTOCOL_ERROR, "CONTINUATION expected");
+    do {
+        const auto result = inboundFrame.read(*m_socket);
+        switch (result) {
+        case FrameStatus::incompleteFrame:
+            return;
+        case FrameStatus::protocolError:
+            return connectionError(PROTOCOL_ERROR, "invalid frame");
+        case FrameStatus::sizeError:
+            return connectionError(FRAME_SIZE_ERROR, "invalid frame size");
+        default:
+            break;
+        }
 
-    switch (inboundFrame.type) {
-    case FrameType::DATA:
-        handleDATA();
-        break;
-    case FrameType::HEADERS:
-        handleHEADERS();
-        break;
-    case FrameType::PRIORITY:
-        handlePRIORITY();
-        break;
-    case FrameType::RST_STREAM:
-        handleRST_STREAM();
-        break;
-    case FrameType::SETTINGS:
-        handleSETTINGS();
-        break;
-    case FrameType::PUSH_PROMISE:
-        handlePUSH_PROMISE();
-        break;
-    case FrameType::PING:
-        handlePING();
-        break;
-    case FrameType::GOAWAY:
-        handleGOAWAY();
-        break;
-    case FrameType::WINDOW_UPDATE:
-        handleWINDOW_UPDATE();
-        break;
-    case FrameType::CONTINUATION:
-        handleCONTINUATION();
-        break;
-    case FrameType::LAST_FRAME_TYPE:
-        // 5.1 - ignore unknown frames.
-        break;
-    }
+        Q_ASSERT(result == FrameStatus::goodFrame);
 
-    if (goingAway && !activeStreams.size())
-        return;
+        if (continuationExpected && inboundFrame.type != FrameType::CONTINUATION)
+            return connectionError(PROTOCOL_ERROR, "CONTINUATION expected");
 
-    if (m_socket->bytesAvailable())
-        QMetaObject::invokeMethod(m_channel, "_q_receiveReply", Qt::QueuedConnection);
+        switch (inboundFrame.type) {
+        case FrameType::DATA:
+            handleDATA();
+            break;
+        case FrameType::HEADERS:
+            handleHEADERS();
+            break;
+        case FrameType::PRIORITY:
+            handlePRIORITY();
+            break;
+        case FrameType::RST_STREAM:
+            handleRST_STREAM();
+            break;
+        case FrameType::SETTINGS:
+            handleSETTINGS();
+            break;
+        case FrameType::PUSH_PROMISE:
+            handlePUSH_PROMISE();
+            break;
+        case FrameType::PING:
+            handlePING();
+            break;
+        case FrameType::GOAWAY:
+            handleGOAWAY();
+            break;
+        case FrameType::WINDOW_UPDATE:
+            handleWINDOW_UPDATE();
+            break;
+        case FrameType::CONTINUATION:
+            handleCONTINUATION();
+            break;
+        case FrameType::LAST_FRAME_TYPE:
+            // 5.1 - ignore unknown frames.
+            break;
+        }
+    } while (!goingAway || activeStreams.size());
 }
 
 bool QHttp2ProtocolHandler::sendRequest()