diff options
author | Timur Pocheptsov <timur.pocheptsov@theqtcompany.com> | 2016-08-09 19:22:48 +0200 |
---|---|---|
committer | Timur Pocheptsov <timur.pocheptsov@theqtcompany.com> | 2016-08-11 12:45:59 +0000 |
commit | 193abdfc0798cf10eac06769b650ac03e86eb55e (patch) | |
tree | 92ef3e1e58ea886a953bbb873fc7a9f23b3c9010 /src/network | |
parent | 615616b0699d98cfb9f4eeb67e005e3226398097 (diff) |
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>
Diffstat (limited to 'src/network')
-rw-r--r-- | src/network/access/http2/http2frames.cpp | 83 | ||||
-rw-r--r-- | src/network/access/http2/http2frames_p.h | 11 | ||||
-rw-r--r-- | src/network/access/qhttp2protocolhandler.cpp | 106 |
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() |