diff --git a/src/network/access/qhttp2connection.cpp b/src/network/access/qhttp2connection.cpp index d56d235bdf7..61bd0d3174f 100644 --- a/src/network/access/qhttp2connection.cpp +++ b/src/network/access/qhttp2connection.cpp @@ -147,7 +147,7 @@ QHttp2Stream::~QHttp2Stream() noexcept = default; This signal is emitted when the stream has finished sending all the data from the upload device. - If the END_STREAM flag was set for sendData() then the stream will be + If the END_STREAM flag was set for sendDATA() then the stream will be closed for further writes before this signal is emitted. */ @@ -212,6 +212,17 @@ void QHttp2Stream::finishWithError(Http2::Http2Error errorCode) finishWithError(errorCode, message); } + +void QHttp2Stream::streamError(Http2::Http2Error errorCode, + QLatin1StringView message) +{ + qCDebug(qHttp2ConnectionLog, "[%p] stream %u finished with error: %ls (error code: %u)", + getConnection(), m_streamID, qUtf16Printable(message), errorCode); + + sendRST_STREAM(errorCode); + emit errorOccurred(errorCode, message); +} + /*! Sends a RST_STREAM frame with the given \a errorCode. This closes the stream for both sides, any further frames will be dropped. @@ -223,6 +234,13 @@ bool QHttp2Stream::sendRST_STREAM(Http2::Http2Error errorCode) { if (m_state == State::Closed || m_state == State::Idle) return false; + // Never respond to a RST_STREAM with a RST_STREAM or looping might occur. + if (m_RST_STREAM_received.has_value()) + return false; + + getConnection()->registerStreamAsResetLocally(streamID()); + + m_RST_STREAM_sent = errorCode; qCDebug(qHttp2ConnectionLog, "[%p] sending RST_STREAM on stream %u, code: %u", getConnection(), m_streamID, errorCode); transitionState(StateTransition::RST); @@ -249,15 +267,19 @@ void QHttp2Stream::sendDATA(QIODevice *device, bool endStream) Q_ASSERT(!m_uploadDevice); Q_ASSERT(!m_uploadByteDevice); Q_ASSERT(device); - if (m_state != State::Open && m_state != State::HalfClosedRemote) + if (m_state != State::Open && m_state != State::HalfClosedRemote) { + qCWarning(qHttp2ConnectionLog, "[%p] attempt to sendDATA on closed stream %u, " + "of device: %p.", + getConnection(), m_streamID, device); return; + } + qCDebug(qHttp2ConnectionLog, "[%p] starting sendDATA on stream %u, of device: %p", + getConnection(), m_streamID, device); auto *byteDevice = QNonContiguousByteDeviceFactory::create(device); connect(this, &QHttp2Stream::uploadFinished, byteDevice, &QObject::deleteLater); byteDevice->setParent(this); m_uploadDevice = device; - qCDebug(qHttp2ConnectionLog, "[%p] starting sendDATA on stream %u, of IODevice: %p", - getConnection(), m_streamID, device); sendDATA(byteDevice, endStream); } @@ -275,12 +297,15 @@ void QHttp2Stream::sendDATA(QNonContiguousByteDevice *device, bool endStream) { Q_ASSERT(!m_uploadByteDevice); Q_ASSERT(device); - if (m_state != State::Open && m_state != State::HalfClosedRemote) + if (m_state != State::Open && m_state != State::HalfClosedRemote) { + qCWarning(qHttp2ConnectionLog, "[%p] attempt to sendDATA on closed stream %u, " + "of device: %p.", + getConnection(), m_streamID, device); return; + } qCDebug(qHttp2ConnectionLog, "[%p] starting sendDATA on stream %u, of device: %p", getConnection(), m_streamID, device); - m_uploadByteDevice = device; m_endStreamAfterDATA = endStream; connect(m_uploadByteDevice, &QNonContiguousByteDevice::readyRead, this, @@ -358,8 +383,7 @@ void QHttp2Stream::internalSendDATA() if (!frameWriter.write(*socket)) { qCDebug(qHttp2ConnectionLog, "[%p] stream %u, failed to write to socket", connection, m_streamID); - finishWithError(INTERNAL_ERROR, "failed to write to socket"_L1); - return; + return finishWithError(INTERNAL_ERROR, "failed to write to socket"_L1); } totalBytesWritten += bytesWritten; @@ -500,7 +524,7 @@ void QHttp2Stream::uploadDeviceDestroyed() if (isUploadingDATA()) { // We're in the middle of sending DATA frames, we need to abort // the stream. - sendRST_STREAM(CANCEL); + streamError(CANCEL, QLatin1String("Upload device destroyed while uploading")); emit uploadDeviceError("Upload device destroyed while uploading"_L1); } m_uploadDevice = nullptr; @@ -570,15 +594,26 @@ void QHttp2Stream::handleDATA(const Frame &inboundFrame) qCDebug(qHttp2ConnectionLog, "[%p] stream %u, received DATA frame with payload of %u bytes", connection, m_streamID, inboundFrame.payloadSize()); + // RFC 9113, 6.1: If a DATA frame is received whose stream is not in the "open" or "half-closed + // (local)" state, the recipient MUST respond with a stream error (Section 5.4.2) of type + // STREAM_CLOSED; + // checked in QHttp2Connection + Q_ASSERT(state() != State::HalfClosedRemote && state() != State::Closed); + if (qint32(inboundFrame.payloadSize()) > m_recvWindow) { qCDebug(qHttp2ConnectionLog, "[%p] stream %u, received DATA frame with payload size %u, " "but recvWindow is %d, sending FLOW_CONTROL_ERROR", connection, m_streamID, inboundFrame.payloadSize(), m_recvWindow); - finishWithError(FLOW_CONTROL_ERROR, "flow control error"_L1); - sendRST_STREAM(FLOW_CONTROL_ERROR); - return; + return streamError(FLOW_CONTROL_ERROR, QLatin1String("data bigger than window size")); } + // RFC 9113, 6.1: The total number of padding octets is determined by the value of the Pad + // Length field. If the length of the padding is the length of the frame payload or greater, + // the recipient MUST treat this as a connection error (Section 5.4.1) of type PROTOCOL_ERROR. + // checked in Framereader + Q_ASSERT(inboundFrame.buffer.size() >= frameHeaderSize); + Q_ASSERT(inboundFrame.payloadSize() + frameHeaderSize == inboundFrame.buffer.size()); + m_recvWindow -= qint32(inboundFrame.payloadSize()); const bool endStream = inboundFrame.flags().testFlag(FrameFlag::END_STREAM); // Uncompress data if needed and append it ... @@ -614,13 +649,13 @@ void QHttp2Stream::handleHEADERS(Http2::FrameFlags frameFlags, const HPack::Http void QHttp2Stream::handleRST_STREAM(const Frame &inboundFrame) { transitionState(StateTransition::RST); - m_RST_STREAM_code = qFromBigEndian(inboundFrame.dataBegin()); + m_RST_STREAM_received = qFromBigEndian(inboundFrame.dataBegin()); if (isUploadingDATA()) { disconnect(m_uploadByteDevice, nullptr, this, nullptr); m_uploadDevice = nullptr; m_uploadByteDevice = nullptr; } - finishWithError(Http2Error(*m_RST_STREAM_code)); + finishWithError(Http2Error(*m_RST_STREAM_received)); } void QHttp2Stream::handleWINDOW_UPDATE(const Frame &inboundFrame) @@ -633,9 +668,7 @@ void QHttp2Stream::handleWINDOW_UPDATE(const Frame &inboundFrame) "[%p] stream %u, received WINDOW_UPDATE frame with invalid delta %u, sending " "PROTOCOL_ERROR", getConnection(), m_streamID, delta); - finishWithError(PROTOCOL_ERROR, "invalid WINDOW_UPDATE delta"_L1); - sendRST_STREAM(PROTOCOL_ERROR); - return; + return streamError(PROTOCOL_ERROR, "invalid WINDOW_UPDATE delta"_L1); } m_sendWindow = sum; // Stream may have been unblocked, so maybe try to write again @@ -1000,8 +1033,30 @@ void QHttp2Connection::handleReadyRead() return; case FrameStatus::protocolError: return connectionError(PROTOCOL_ERROR, "invalid frame"); - case FrameStatus::sizeError: - return connectionError(FRAME_SIZE_ERROR, "invalid frame size"); + case FrameStatus::sizeError: { + const auto streamID = frameReader.inboundFrame().streamID(); + const auto frameType = frameReader.inboundFrame().type(); + auto stream = getStream(streamID); + // RFC 9113, 4.2: A frame size error in a frame that could alter the state of the + // entire connection MUST be treated as a connection error (Section 5.4.1); this + // includes any frame carrying a field block (Section 4.3) (that is, HEADERS, + // PUSH_PROMISE, and CONTINUATION), a SETTINGS frame, and any frame with a stream + // identifier of 0. + if (frameType == FrameType::HEADERS || + frameType == FrameType::SETTINGS || + frameType == FrameType::PUSH_PROMISE || + frameType == FrameType::CONTINUATION || + // never reply RST_STREAM with RST_STREAM + frameType == FrameType::RST_STREAM || + streamID == connectionStreamID) + return connectionError(FRAME_SIZE_ERROR, "invalid frame size"); + // DATA; PRIORITY; WINDOW_UPDATE + if (stream) + return stream->streamError(Http2Error::FRAME_SIZE_ERROR, + QLatin1String("invalid frame size")); + else + return; // most likely a closed and deleted stream. Can be ignored. + } default: break; } @@ -1013,6 +1068,11 @@ void QHttp2Connection::handleReadyRead() const auto frameType = inboundFrame.type(); qCDebug(qHttp2ConnectionLog, "[%p] Successfully read a frame, with type: %d", this, int(frameType)); + + // RFC 9113, 6.2/6.6: A HEADERS/PUSH_PROMISE frame without the END_HEADERS flag set MUST be + // followed by a CONTINUATION frame for the same stream. A receiver MUST treat the + // receipt of any other type of frame or a frame on a different stream as a + // connection error if (continuationExpected && frameType != FrameType::CONTINUATION) return connectionError(PROTOCOL_ERROR, "CONTINUATION expected"); @@ -1093,12 +1153,16 @@ void QHttp2Connection::setH2Configuration(QHttp2Configuration config) void QHttp2Connection::connectionError(Http2Error errorCode, const char *message) { Q_ASSERT(message); + // RFC 9113, 6.8: An endpoint MAY send multiple GOAWAY frames if circumstances change. + // Anyway, we do not send multiple GOAWAY frames. if (m_goingAway) return; qCCritical(qHttp2ConnectionLog, "[%p] Connection error: %s (%d)", this, message, int(errorCode)); + // RFC 9113, 6.8: Endpoints SHOULD always send a GOAWAY frame before closing a connection so + // that the remote peer can know whether a stream has been partially processed or not. m_goingAway = true; sendGOAWAY(errorCode); auto messageView = QLatin1StringView(message); @@ -1116,15 +1180,28 @@ void QHttp2Connection::closeSession() emit connectionClosed(); } -bool QHttp2Connection::streamWasReset(quint32 streamID) noexcept +bool QHttp2Connection::streamWasResetLocally(quint32 streamID) noexcept { return m_resetStreamIDs.contains(streamID); } +void QHttp2Connection::registerStreamAsResetLocally(quint32 streamID) +{ + // RFC 9113, 6.4: However, after sending the RST_STREAM, the sending endpoint MUST be prepared + // to receive and process additional frames sent on the stream that might have been sent by the + // peer prior to the arrival of the RST_STREAM. + + // Store the last 100 stream ids that were reset locally. Frames received on these streams + // are still considered valid for some time (Until 100 other streams are reset locally). + m_resetStreamIDs.append(streamID); + while (m_resetStreamIDs.size() > 100) + m_resetStreamIDs.takeFirst(); +} + bool QHttp2Connection::isInvalidStream(quint32 streamID) noexcept { auto stream = m_streams.value(streamID, nullptr); - return !stream && !streamWasReset(streamID); + return (!stream || stream->wasResetbyPeer()) && !streamWasResetLocally(streamID); } bool QHttp2Connection::sendClientPreface() @@ -1205,12 +1282,24 @@ void QHttp2Connection::handleDATA() Q_ASSERT(inboundFrame.type() == FrameType::DATA); const auto streamID = inboundFrame.streamID(); + + // RFC9113, 6.1: An endpoint that receives an unexpected stream identifier MUST respond + // with a connection error. if (streamID == connectionStreamID) return connectionError(PROTOCOL_ERROR, "DATA on the connection stream"); if (isInvalidStream(streamID)) return connectionError(ENHANCE_YOUR_CALM, "DATA on invalid stream"); + // RFC9113, 6.1: If a DATA frame is received whose stream is not in the "open" or + // "half-closed (local)" state, the recipient MUST respond with a stream error. + auto stream = getStream(streamID); + if (stream->state() == QHttp2Stream::State::HalfClosedRemote + || stream->state() == QHttp2Stream::State::Closed) { + return stream->streamError(Http2Error::STREAM_CLOSED, + QLatin1String("Data on closed stream")); + } + if (qint32(inboundFrame.payloadSize()) > sessionReceiveWindowSize) { qCDebug(qHttp2ConnectionLog, "[%p] Received DATA frame with payload size %u, " @@ -1225,6 +1314,9 @@ void QHttp2Connection::handleDATA() if (it != m_streams.cend() && it.value()) it.value()->handleDATA(inboundFrame); + if (inboundFrame.flags().testFlag(FrameFlag::END_STREAM)) + emit receivedEND_STREAM(streamID); + if (sessionReceiveWindowSize < maxSessionReceiveWindowSize / 2) { // @future[consider]: emit signal instead QMetaObject::invokeMethod(this, &QHttp2Connection::sendWINDOW_UPDATE, Qt::QueuedConnection, @@ -1241,6 +1333,8 @@ void QHttp2Connection::handleHEADERS() const auto streamID = inboundFrame.streamID(); qCDebug(qHttp2ConnectionLog, "[%p] Received HEADERS frame on stream %d", this, streamID); + // RFC 9113, 6.2: If a HEADERS frame is received whose Stream Identifier field is 0x00, the + // recipient MUST respond with a connection error. if (streamID == connectionStreamID) return connectionError(PROTOCOL_ERROR, "HEADERS on 0x0 stream"); @@ -1255,10 +1349,14 @@ void QHttp2Connection::handleHEADERS() qCDebug(qHttp2ConnectionLog, "[%p] Created new incoming stream %d", this, streamID); emit newIncomingStream(newStream); } else if (auto it = m_streams.constFind(streamID); it == m_streams.cend()) { + // RFC 9113, 6.2: HEADERS frames MUST be associated with a stream. + // A connection error is not required but it seems to be the right thing to do. qCDebug(qHttp2ConnectionLog, "[%p] Received HEADERS on non-existent stream %d", this, streamID); return connectionError(PROTOCOL_ERROR, "HEADERS on invalid stream"); - } else if (!*it || (*it)->wasReset()) { + } else if (isInvalidStream(streamID)) { + // RFC 9113 6.4: After receiving a RST_STREAM on a stream, the receiver MUST NOT send + // additional frames for that stream qCDebug(qHttp2ConnectionLog, "[%p] Received HEADERS on reset stream %d", this, streamID); return connectionError(ENHANCE_YOUR_CALM, "HEADERS on invalid stream"); } @@ -1289,12 +1387,21 @@ void QHttp2Connection::handlePRIORITY() || inboundFrame.type() == FrameType::HEADERS); const auto streamID = inboundFrame.streamID(); + // RFC 9913, 6.3: If a PRIORITY frame is received with a stream identifier of 0x00, the + // recipient MUST respond with a connection error if (streamID == connectionStreamID) return connectionError(PROTOCOL_ERROR, "PRIORITY on 0x0 stream"); + // RFC 9113 6.4: After receiving a RST_STREAM on a stream, the receiver MUST NOT send + // additional frames for that stream if (isInvalidStream(streamID)) return connectionError(ENHANCE_YOUR_CALM, "PRIORITY on invalid stream"); + // RFC 9913, 6.3: A PRIORITY frame with a length other than 5 octets MUST be treated as a + // stream error (Section 5.4.2) of type FRAME_SIZE_ERROR. + // checked in Frame::validateHeader() + Q_ASSERT(inboundFrame.type() != FrameType::PRIORITY || inboundFrame.payloadSize() == 5); + quint32 streamDependency = 0; uchar weight = 0; const bool noErr = inboundFrame.priority(&streamDependency, &weight); @@ -1314,7 +1421,7 @@ void QHttp2Connection::handleRST_STREAM() { Q_ASSERT(inboundFrame.type() == FrameType::RST_STREAM); - // "RST_STREAM frames MUST be associated with a stream. + // RFC 9113, 6.4: RST_STREAM frames MUST be associated with a stream. // If a RST_STREAM frame is received with a stream identifier of 0x0, // the recipient MUST treat this as a connection error (Section 5.4.1) // of type PROTOCOL_ERROR. @@ -1322,12 +1429,14 @@ void QHttp2Connection::handleRST_STREAM() if (streamID == connectionStreamID) return connectionError(PROTOCOL_ERROR, "RST_STREAM on 0x0"); - if (!(streamID & 0x1)) { // @future[server]: must be updated for server-side handling - // RST_STREAM on a promised stream: - // since we do not keep track of such streams, - // just ignore. - return; - } + // RFC 9113, 6.4: A RST_STREAM frame with a length other than 4 octets MUST be treated as a + // connection error (Section 5.4.1) of type FRAME_SIZE_ERROR. + // checked in Frame::validateHeader() + Q_ASSERT(inboundFrame.payloadSize() == 4); + + const auto error = qFromBigEndian(inboundFrame.dataBegin()); + if (QPointer stream = m_streams[streamID]) + emit stream->rstFrameRecived(error); // Verify that whatever stream is being RST'd is not in the idle state: const quint32 lastRelevantStreamID = [this, streamID]() { @@ -1352,10 +1461,16 @@ void QHttp2Connection::handleSETTINGS() // 6.5 SETTINGS. Q_ASSERT(inboundFrame.type() == FrameType::SETTINGS); + // RFC 9113, 6.5: If an endpoint receives a SETTINGS frame whose Stream Identifier field is + // anything other than 0x00, the endpoint MUST respond with a connection error if (inboundFrame.streamID() != connectionStreamID) return connectionError(PROTOCOL_ERROR, "SETTINGS on invalid stream"); if (inboundFrame.flags().testFlag(FrameFlag::ACK)) { + // RFC 9113, 6.5: Receipt of a SETTINGS frame with the ACK flag set and a length field + // value other than 0 MUST be treated as a connection error + if (inboundFrame.payloadSize ()) + return connectionError(FRAME_SIZE_ERROR, "SETTINGS ACK with data"); if (!waitingForSettingsACK) return connectionError(PROTOCOL_ERROR, "unexpected SETTINGS ACK"); qCDebug(qHttp2ConnectionLog, "[%p] Received SETTINGS ACK", this); @@ -1365,6 +1480,11 @@ void QHttp2Connection::handleSETTINGS() qCDebug(qHttp2ConnectionLog, "[%p] Received SETTINGS frame", this); if (inboundFrame.dataSize()) { + // RFC 9113, 6.5: A SETTINGS frame with a length other than a multiple of 6 octets MUST be + // treated as a connection error (Section 5.4.1) of type FRAME_SIZE_ERROR. + // checked in Frame::validateHeader() + Q_ASSERT (inboundFrame.payloadSize() % 6 == 0); + auto src = inboundFrame.dataBegin(); for (const uchar *end = src + inboundFrame.dataSize(); src != end; src += 6) { const Settings identifier = Settings(qFromBigEndian(src)); @@ -1388,12 +1508,17 @@ void QHttp2Connection::handlePUSH_PROMISE() // 6.6 PUSH_PROMISE. Q_ASSERT(inboundFrame.type() == FrameType::PUSH_PROMISE); + // RFC 9113, 6.6: PUSH_PROMISE MUST NOT be sent if the SETTINGS_ENABLE_PUSH setting of the peer + // endpoint is set to 0. An endpoint that has set this setting and has received acknowledgment + // MUST treat the receipt of a PUSH_PROMISE frame as a connection error if (!pushPromiseEnabled && !waitingForSettingsACK) { // This means, server ACKed our 'NO PUSH', // but sent us PUSH_PROMISE anyway. return connectionError(PROTOCOL_ERROR, "unexpected PUSH_PROMISE frame"); } + // RFC 9113, 6.6: If the Stream Identifier field specifies the value 0x00, a recipient MUST + // respond with a connection error. const auto streamID = inboundFrame.streamID(); if (streamID == connectionStreamID) return connectionError(PROTOCOL_ERROR, "PUSH_PROMISE with invalid associated stream (0x0)"); @@ -1409,25 +1534,45 @@ void QHttp2Connection::handlePUSH_PROMISE() } } #endif + // RFC 9113, 6.6: PUSH_PROMISE frames MUST only be sent on a peer-initiated stream that + // is in either the "open" or "half-closed (remote)" state. if (it == m_streams.constEnd()) return connectionError(ENHANCE_YOUR_CALM, "PUSH_PROMISE with invalid associated stream"); + if ((m_connectionType == Type::Client && (streamID & 1) == 1) || + (m_connectionType == Type::Server && (streamID & 1) == 0)) { + return connectionError(ENHANCE_YOUR_CALM, "PUSH_PROMISE with invalid associated stream"); + } + if ((*it)->state() != QHttp2Stream::State::Open && + (*it)->state() != QHttp2Stream::State::HalfClosedRemote) { + return connectionError(ENHANCE_YOUR_CALM, "PUSH_PROMISE with invalid associated stream"); + } + // RFC 9113, 6.6: The promised stream identifier MUST be a valid choice for the + // next stream sent by the sender const auto reservedID = qFromBigEndian(inboundFrame.dataBegin()); if ((reservedID & 1) || reservedID <= m_lastIncomingStreamID || reservedID > lastValidStreamID) return connectionError(PROTOCOL_ERROR, "PUSH_PROMISE with invalid promised stream ID"); + // RFC 9113, 6.6: A receiver MUST treat the receipt of a PUSH_PROMISE that promises an + // illegal stream identifier (Section 5.1.1) as a connection error auto *stream = createStreamInternal_impl(reservedID); if (!stream) return connectionError(PROTOCOL_ERROR, "PUSH_PROMISE with already active stream ID"); m_lastIncomingStreamID = reservedID; stream->setState(QHttp2Stream::State::ReservedRemote); + // "ignoring a PUSH_PROMISE frame causes the stream state to become + // indeterminate" - let's send RST_STREAM frame with REFUSE_STREAM code. if (!pushPromiseEnabled) { - // "ignoring a PUSH_PROMISE frame causes the stream state to become - // indeterminate" - let's send RST_STREAM frame with REFUSE_STREAM code. - stream->sendRST_STREAM(REFUSE_STREAM); + return stream->streamError(REFUSE_STREAM, + QLatin1String("PUSH_PROMISE not enabled but ignored")); } + // RFC 9113, 6.6: The total number of padding octets is determined by the value of the Pad + // Length field. If the length of the padding is the length of the frame payload or greater, + // the recipient MUST treat this as a connection error (Section 5.4.1) of type PROTOCOL_ERROR. + // checked in Frame::validateHeader() + Q_ASSERT(inboundFrame.dataSize() > qFromBigEndian(inboundFrame.dataBegin() + 32)); const bool endHeaders = inboundFrame.flags().testFlag(FrameFlag::END_HEADERS); continuedFrames.clear(); continuedFrames.push_back(std::move(inboundFrame)); @@ -1443,11 +1588,18 @@ void QHttp2Connection::handlePUSH_PROMISE() void QHttp2Connection::handlePING() { Q_ASSERT(inboundFrame.type() == FrameType::PING); - Q_ASSERT(inboundFrame.dataSize() == 8); + // RFC 9113, 6.7: PING frames are not associated with any individual stream. If a PING frame is + // received with a Stream Identifier field value other than 0x00, the recipient MUST respond + // with a connection error if (inboundFrame.streamID() != connectionStreamID) return connectionError(PROTOCOL_ERROR, "PING on invalid stream"); + // Receipt of a PING frame with a length field value other than 8 MUST be treated + // as a connection error (Section 5.4.1) of type FRAME_SIZE_ERROR. + // checked in Frame::validateHeader() + Q_ASSERT(inboundFrame.payloadSize() == 8); + if (inboundFrame.flags() & FrameFlag::ACK) { QByteArrayView pingSignature(reinterpret_cast(inboundFrame.dataBegin()), 8); if (!m_lastPingSignature.has_value()) { @@ -1477,11 +1629,16 @@ void QHttp2Connection::handleGOAWAY() // 6.8 GOAWAY Q_ASSERT(inboundFrame.type() == FrameType::GOAWAY); - // "An endpoint MUST treat a GOAWAY frame with a stream identifier - // other than 0x0 as a connection error (Section 5.4.1) of type PROTOCOL_ERROR." + // RFC 9113, 6.8: An endpoint MUST treat a GOAWAY frame with a stream identifier + // other than 0x0 as a connection error (Section 5.4.1) of type PROTOCOL_ERROR. if (inboundFrame.streamID() != connectionStreamID) return connectionError(PROTOCOL_ERROR, "GOAWAY on invalid stream"); + // RFC 9113, 6.8: + // Reserved (1) + Last-Stream-ID (31) + Error Code (32) + Additional Debug Data (..) + // checked in Frame::validateHeader() + Q_ASSERT(inboundFrame.payloadSize() >= 8); + const uchar *const src = inboundFrame.dataBegin(); quint32 lastStreamID = qFromBigEndian(src); const Http2Error errorCode = Http2Error(qFromBigEndian(src + 4)); @@ -1523,9 +1680,18 @@ void QHttp2Connection::handleWINDOW_UPDATE() Q_ASSERT(inboundFrame.type() == FrameType::WINDOW_UPDATE); const quint32 delta = qFromBigEndian(inboundFrame.dataBegin()); + // RFC 9113, 6.9: A receiver MUST treat the receipt of a WINDOW_UPDATE frame with a + // flow-control window increment of 0 as a stream error (Section 5.4.2) of type PROTOCOL_ERROR; + // errors on the connection flow-control window MUST be treated as a connection error const bool valid = delta && delta <= quint32(std::numeric_limits::max()); const auto streamID = inboundFrame.streamID(); + + // RFC 9113, 6.9: A WINDOW_UPDATE frame with a length other than 4 octets MUST be treated + // as a connection error (Section 5.4.1) of type FRAME_SIZE_ERROR. + // checked in Frame::validateHeader() + Q_ASSERT(inboundFrame.payloadSize() == 4); + qCDebug(qHttp2ConnectionLog(), "[%p] Received WINDOW_UPDATE, stream %d, delta %d", this, streamID, delta); if (streamID == connectionStreamID) { @@ -1548,6 +1714,9 @@ void QHttp2Connection::handleWINDOW_UPDATE() qCDebug(qHttp2ConnectionLog, "[%p] Received WINDOW_UPDATE on closed stream %d", this, streamID); return; + } else if (!valid) { + return stream->streamError(PROTOCOL_ERROR, + QLatin1String("WINDOW_UPDATE invalid delta")); } stream->handleWINDOW_UPDATE(inboundFrame); } @@ -1598,10 +1767,7 @@ void QHttp2Connection::handleContinuedHEADERS() // We can receive HEADERS on streams initiated by our requests // (these streams are in halfClosedLocal or open state) or // remote-reserved streams from a server's PUSH_PROMISE. - stream->finishWithError(PROTOCOL_ERROR, - "HEADERS on invalid stream"_L1); - stream->sendRST_STREAM(CANCEL); - return; + return stream->streamError(PROTOCOL_ERROR, "HEADERS on invalid stream"_L1); } } // Else: we cannot just ignore our peer's HEADERS frames - they change @@ -1625,8 +1791,10 @@ void QHttp2Connection::handleContinuedHEADERS() // not include a complete and valid set of header fields or the :method // pseudo-header field identifies a method that is not safe, it MUST // respond with a stream error (Section 5.4.2) of type PROTOCOL_ERROR." - if (streamIt != m_streams.cend()) - (*streamIt)->sendRST_STREAM(PROTOCOL_ERROR); + if (streamIt != m_streams.cend()) { + (*streamIt)->streamError(PROTOCOL_ERROR, + QLatin1String("PUSH_PROMISE with incomplete headers")); + } return; } @@ -1696,9 +1864,7 @@ bool QHttp2Connection::acceptSetting(Http2::Settings identifier, quint32 newValu continue; qint32 sum = 0; if (qAddOverflow(stream->m_sendWindow, delta, &sum)) { - stream->sendRST_STREAM(PROTOCOL_ERROR); - stream->finishWithError(PROTOCOL_ERROR, - "SETTINGS window overflow"_L1); + stream->streamError(PROTOCOL_ERROR, "SETTINGS window overflow"_L1); continue; } stream->m_sendWindow = sum; diff --git a/src/network/access/qhttp2connection_p.h b/src/network/access/qhttp2connection_p.h index 028860c0d68..c5ea94a6572 100644 --- a/src/network/access/qhttp2connection_p.h +++ b/src/network/access/qhttp2connection_p.h @@ -34,6 +34,8 @@ #include #include +class tst_QHttp2Connection; + QT_BEGIN_NAMESPACE template @@ -108,8 +110,11 @@ public: State state() const noexcept { return m_state; } bool isActive() const noexcept { return m_state != State::Closed && m_state != State::Idle; } bool isPromisedStream() const noexcept { return m_isReserved; } - bool wasReset() const noexcept { return m_RST_STREAM_code.has_value(); } - quint32 RST_STREAM_code() const noexcept { return m_RST_STREAM_code.value_or(0); } + bool wasReset() const noexcept { return m_RST_STREAM_received.has_value() || + m_RST_STREAM_sent.has_value(); } + bool wasResetbyPeer() const noexcept { return m_RST_STREAM_received.has_value(); } + quint32 RST_STREAMCodeReceived() const noexcept { return m_RST_STREAM_received.value_or(0); } + quint32 RST_STREAMCodeSent() const noexcept { return m_RST_STREAM_sent.value_or(0); } // Just the list of headers, as received, may contain duplicates: HPack::HttpHeader receivedHeaders() const noexcept { return m_headers; } @@ -125,6 +130,7 @@ Q_SIGNALS: void promisedStreamReceived(quint32 newStreamID); void uploadBlocked(); void dataReceived(const QByteArray &data, bool endStream); + void rstFrameRecived(quint32 errorCode); void bytesWritten(qint64 bytesWritten); void uploadDeviceError(const QString &errorString); @@ -172,12 +178,16 @@ private: void finishWithError(Http2::Http2Error errorCode, const QString &message); void finishWithError(Http2::Http2Error errorCode); + void streamError(Http2::Http2Error errorCode, + QLatin1StringView message); + // Keep it const since it never changes after creation const quint32 m_streamID = 0; qint32 m_recvWindow = 0; qint32 m_sendWindow = 0; bool m_endStreamAfterDATA = false; - std::optional m_RST_STREAM_code; + std::optional m_RST_STREAM_received; + std::optional m_RST_STREAM_sent; QIODevice *m_uploadDevice = nullptr; QNonContiguousByteDevice *m_uploadByteDevice = nullptr; @@ -186,6 +196,8 @@ private: State m_state = State::Idle; HPack::HttpHeader m_headers; bool m_isReserved = false; + + friend tst_QHttp2Connection; }; class Q_NETWORK_EXPORT QHttp2Connection : public QObject @@ -246,6 +258,7 @@ Q_SIGNALS: void pingFrameRecived(QHttp2Connection::PingState state); void errorOccurred(Http2::Http2Error errorCode, const QString &errorString); void receivedGOAWAY(Http2::Http2Error errorCode, quint32 lastStreamID); + void receivedEND_STREAM(quint32 streamID); public Q_SLOTS: bool sendPing(); bool sendPing(QByteArrayView data); @@ -260,12 +273,13 @@ private: QHttp2Stream *createStreamInternal_impl(quint32 streamID); bool isInvalidStream(quint32 streamID) noexcept; - bool streamWasReset(quint32 streamID) noexcept; + bool streamWasResetLocally(quint32 streamID) noexcept; void connectionError(Http2::Http2Error errorCode, const char *message); // Connection failed to be established? void setH2Configuration(QHttp2Configuration config); void closeSession(); + void registerStreamAsResetLocally(quint32 streamID); qsizetype numActiveStreamsImpl(quint32 mask) const noexcept; qsizetype numActiveRemoteStreams() const noexcept; qsizetype numActiveLocalStreams() const noexcept; @@ -310,7 +324,8 @@ private: QHttp2Configuration m_config; QHash> m_streams; QHash m_promisedStreams; - QVarLengthArray m_resetStreamIDs; + QList m_resetStreamIDs; + std::optional m_lastPingSignature = std::nullopt; quint32 m_nextStreamID = 1; @@ -367,6 +382,8 @@ private: // Server-side only: bool m_waitingForClientPreface = false; + + friend tst_QHttp2Connection; }; QT_END_NAMESPACE diff --git a/tests/auto/network/access/qhttp2connection/tst_qhttp2connection.cpp b/tests/auto/network/access/qhttp2connection/tst_qhttp2connection.cpp index f4e418501f2..34911643e24 100644 --- a/tests/auto/network/access/qhttp2connection/tst_qhttp2connection.cpp +++ b/tests/auto/network/access/qhttp2connection/tst_qhttp2connection.cpp @@ -21,6 +21,13 @@ private slots: void constructStream(); void testSETTINGSFrame(); void testPING(); + void testRSTServerSide(); + void testRSTClientSide(); + void testRSTReplyOnDATAEND(); + void testBadFrameSize_data(); + void testBadFrameSize(); + void testDataFrameAfterRSTIncoming(); + void testDataFrameAfterRSTOutgoing(); void connectToServer(); void WINDOW_UPDATE(); void testCONTINUATIONFrame(); @@ -180,7 +187,7 @@ void tst_QHttp2Connection::constructStream() QVERIFY(stream); QCOMPARE(stream->isPromisedStream(), false); QCOMPARE(stream->isActive(), false); - QCOMPARE(stream->RST_STREAM_code(), 0u); + QCOMPARE(stream->RST_STREAMCodeReceived(), 0u); QCOMPARE(stream->streamID(), 1u); QCOMPARE(stream->receivedHeaders(), {}); QCOMPARE(stream->state(), QHttp2Stream::State::Idle); @@ -288,6 +295,290 @@ void tst_QHttp2Connection::testPING() QCOMPARE(serverPingSpy.last().at(0).toInt(), int(QHttp2Connection::PingState::PongSignatureIdentical)); } + +void tst_QHttp2Connection::testRSTServerSide() +{ + auto [client, server] = makeFakeConnectedSockets(); + auto connection = makeHttp2Connection(client.get(), {}, Client); + auto serverConnection = makeHttp2Connection(server.get(), {}, Server); + + QVERIFY(waitForSettingsExchange(connection, serverConnection)); + + QSignalSpy newIncomingStreamSpy{ serverConnection, &QHttp2Connection::newIncomingStream }; + + QHttp2Stream *clientStream = connection->createStream().unwrap(); + QSignalSpy clientHeaderReceivedSpy{ clientStream, &QHttp2Stream::headersReceived }; + QVERIFY(clientStream); + HPack::HttpHeader headers = getRequiredHeaders(); + clientStream->sendHEADERS(headers, false); + + QVERIFY(newIncomingStreamSpy.wait()); + auto *serverStream = newIncomingStreamSpy.front().front().value(); + QCOMPARE(clientStream->streamID(), serverStream->streamID()); + + QSignalSpy rstClientSpy{ clientStream, &QHttp2Stream::rstFrameRecived }; + QSignalSpy rstServerSpy{ serverStream, &QHttp2Stream::rstFrameRecived }; + + QCOMPARE(clientStream->state(), QHttp2Stream::State::Open); + QCOMPARE(serverStream->state(), QHttp2Stream::State::Open); + + serverStream->sendRST_STREAM(Http2::INTERNAL_ERROR); + QCOMPARE(serverStream->state(), QHttp2Stream::State::Closed); + QVERIFY(rstClientSpy.wait()); + QCOMPARE(rstServerSpy.count(), 0); + QCOMPARE(clientStream->state(), QHttp2Stream::State::Closed); +} + +void tst_QHttp2Connection::testRSTClientSide() +{ + auto [client, server] = makeFakeConnectedSockets(); + auto connection = makeHttp2Connection(client.get(), {}, Client); + auto serverConnection = makeHttp2Connection(server.get(), {}, Server); + + QVERIFY(waitForSettingsExchange(connection, serverConnection)); + + QSignalSpy newIncomingStreamSpy{ serverConnection, &QHttp2Connection::newIncomingStream }; + + QHttp2Stream *clientStream = connection->createStream().unwrap(); + QSignalSpy clientHeaderReceivedSpy{ clientStream, &QHttp2Stream::headersReceived }; + QVERIFY(clientStream); + HPack::HttpHeader headers = getRequiredHeaders(); + clientStream->sendHEADERS(headers, false); + + QVERIFY(newIncomingStreamSpy.wait()); + auto *serverStream = newIncomingStreamSpy.front().front().value(); + QCOMPARE(clientStream->streamID(), serverStream->streamID()); + + QSignalSpy rstClientSpy{ clientStream, &QHttp2Stream::rstFrameRecived }; + QSignalSpy rstServerSpy{ serverStream, &QHttp2Stream::rstFrameRecived }; + + QCOMPARE(clientStream->state(), QHttp2Stream::State::Open); + QCOMPARE(serverStream->state(), QHttp2Stream::State::Open); + + clientStream->sendRST_STREAM(Http2::INTERNAL_ERROR); + QCOMPARE(clientStream->state(), QHttp2Stream::State::Closed); + QVERIFY(rstServerSpy.wait()); + QCOMPARE(rstClientSpy.count(), 0); + QCOMPARE(serverStream->state(), QHttp2Stream::State::Closed); +} + +void tst_QHttp2Connection::testRSTReplyOnDATAEND() +{ + auto [client, server] = makeFakeConnectedSockets(); + auto connection = makeHttp2Connection(client.get(), {}, Client); + auto serverConnection = makeHttp2Connection(server.get(), {}, Server); + + QVERIFY(waitForSettingsExchange(connection, serverConnection)); + + QSignalSpy newIncomingStreamSpy{ serverConnection, &QHttp2Connection::newIncomingStream }; + + QHttp2Stream *clientStream = connection->createStream().unwrap(); + QSignalSpy clientHeaderReceivedSpy{ clientStream, &QHttp2Stream::headersReceived }; + QVERIFY(clientStream); + HPack::HttpHeader headers = getRequiredHeaders(); + clientStream->sendHEADERS(headers, false); + + QVERIFY(newIncomingStreamSpy.wait()); + auto *serverStream = newIncomingStreamSpy.front().front().value(); + QCOMPARE(clientStream->streamID(), serverStream->streamID()); + + QSignalSpy rstClientSpy{ clientStream, &QHttp2Stream::rstFrameRecived }; + QSignalSpy rstServerSpy{ serverStream, &QHttp2Stream::rstFrameRecived }; + QSignalSpy endServerSpy{ serverConnection, &QHttp2Connection::receivedEND_STREAM }; + QSignalSpy errrorServerSpy{ serverStream, &QHttp2Stream::errorOccurred }; + + QCOMPARE(clientStream->state(), QHttp2Stream::State::Open); + QCOMPARE(serverStream->state(), QHttp2Stream::State::Open); + + // Send data with END_STREAM = true + + QBuffer *buffer = new QBuffer(clientStream); + QByteArray uploadedData = "Hello World"_ba.repeated(10); + buffer->setData(uploadedData); + buffer->open(QIODevice::ReadWrite); + // send data with endstream true + clientStream->sendDATA(buffer, true); + + QVERIFY(endServerSpy.wait()); + + QCOMPARE(clientStream->state(), QHttp2Stream::State::HalfClosedLocal); + QCOMPARE(serverStream->state(), QHttp2Stream::State::HalfClosedRemote); + + clientStream->setState(QHttp2Stream::State::Open); + buffer = new QBuffer(clientStream); + buffer->setData(uploadedData); + buffer->open(QIODevice::ReadWrite); + // send data on closed stream + clientStream->sendDATA(buffer, true); + + QVERIFY(rstClientSpy.wait()); + QCOMPARE(rstServerSpy.count(), 0); + QCOMPARE(errrorServerSpy.count(), 1); +} + +void tst_QHttp2Connection::testBadFrameSize_data() +{ + QTest::addColumn("frametype"); + QTest::addColumn("loadsize"); + QTest::addColumn("rst_received"); + QTest::addColumn("goaway_received"); + + QTest::newRow("priority_correct") << uchar(Http2::FrameType::PRIORITY) << 5 << false << 0; + QTest::newRow("priority_bad") << uchar(Http2::FrameType::PRIORITY) << 6 << true << 0; + QTest::newRow("ping_correct") << uchar(Http2::FrameType::PING) << 8 << false << 0; + QTest::newRow("ping_bad") << uchar(Http2::FrameType::PING) << 13 << false << 1; +} + +void tst_QHttp2Connection::testBadFrameSize() +{ + QFETCH(uchar, frametype); + QFETCH(int, loadsize); + QFETCH(bool, rst_received); + QFETCH(int, goaway_received); + + auto [client, server] = makeFakeConnectedSockets(); + auto connection = makeHttp2Connection(client.get(), {}, Client); + auto serverConnection = makeHttp2Connection(server.get(), {}, Server); + + QVERIFY(waitForSettingsExchange(connection, serverConnection)); + + QSignalSpy newIncomingStreamSpy{ serverConnection, &QHttp2Connection::newIncomingStream }; + + QHttp2Stream *clientStream = connection->createStream().unwrap(); + QSignalSpy clientHeaderReceivedSpy{ clientStream, &QHttp2Stream::headersReceived }; + QVERIFY(clientStream); + HPack::HttpHeader headers = getRequiredHeaders(); + clientStream->sendHEADERS(headers, false); + + QVERIFY(newIncomingStreamSpy.wait()); + auto *serverStream = newIncomingStreamSpy.front().front().value(); + QCOMPARE(clientStream->streamID(), serverStream->streamID()); + + QSignalSpy rstClientSpy{ clientStream, &QHttp2Stream::rstFrameRecived }; + QSignalSpy rstServerSpy{ serverStream, &QHttp2Stream::rstFrameRecived }; + QSignalSpy goawayClientSpy{ connection, &QHttp2Connection::receivedGOAWAY }; + QSignalSpy goawayServerSpy{ serverConnection, &QHttp2Connection::receivedGOAWAY }; + + QCOMPARE(clientStream->state(), QHttp2Stream::State::Open); + QCOMPARE(serverStream->state(), QHttp2Stream::State::Open); + + { + auto type = frametype; + auto flags = uchar(Http2::FrameFlag::EMPTY); + quint32 streamID = clientStream->streamID(); + + std::vector buffer; + + buffer.resize(Http2::frameHeaderSize); + //012 Length (24) = 0x05 (set below), + //3 Type (8) = 0x02, + //4 Unused Flags (8), + // Reserved (1), + //5 Stream Identifier (31), + buffer[3] = type; + buffer[4] = flags; + qToBigEndian(type == uchar(Http2::FrameType::PING) ? 0 : streamID, &buffer[5]); + + buffer.resize(buffer.size() + loadsize); + // RFC9113 4.1: The 9 octets of the frame header are not included in this value. + quint32 size = quint32(buffer.size() - Http2::frameHeaderSize); + buffer[0] = size >> 16; + buffer[1] = size >> 8; + buffer[2] = size; + + auto writtenN = connection->getSocket()->write(reinterpret_cast(&buffer[0]), buffer.size()); + QCOMPARE(writtenN, buffer.size()); + } + + QCOMPARE(rstClientSpy.wait(), rst_received); + QCOMPARE(rstServerSpy.count(), 0); + QCOMPARE(goawayClientSpy.count(), goaway_received); +} + +void tst_QHttp2Connection::testDataFrameAfterRSTIncoming() +{ + auto [client, server] = makeFakeConnectedSockets(); + auto connection = makeHttp2Connection(client.get(), {}, Client); + auto serverConnection = makeHttp2Connection(server.get(), {}, Server); + + QVERIFY(waitForSettingsExchange(connection, serverConnection)); + + QSignalSpy newIncomingStreamSpy{ serverConnection, &QHttp2Connection::newIncomingStream }; + + QHttp2Stream *clientStream = connection->createStream().unwrap(); + QSignalSpy clientHeaderReceivedSpy{ clientStream, &QHttp2Stream::headersReceived }; + QVERIFY(clientStream); + HPack::HttpHeader headers = getRequiredHeaders(); + clientStream->sendHEADERS(headers, false); + + QVERIFY(newIncomingStreamSpy.wait()); + auto *serverStream = newIncomingStreamSpy.front().front().value(); + QCOMPARE(clientStream->streamID(), serverStream->streamID()); + + QSignalSpy rstServerSpy{ serverStream, &QHttp2Stream::rstFrameRecived }; + + QCOMPARE(clientStream->state(), QHttp2Stream::State::Open); + QCOMPARE(serverStream->state(), QHttp2Stream::State::Open); + + // Reset the stream and wait until it the RST_STREAM frame is received + clientStream->sendRST_STREAM(Http2::Http2Error::STREAM_CLOSED); + QVERIFY(rstServerSpy.wait()); + + QSignalSpy closedClientSpy{ connection, &QHttp2Connection::connectionClosed }; + // Send data as if we didn't receive the RST_STREAM + // It should not trigger an error since we could have not received the RST_STREAM frame yet + serverStream->setState(QHttp2Stream::State::Open); + QBuffer *buffer = new QBuffer(clientStream); + QByteArray uploadedData = "Hello World"_ba.repeated(10); + buffer->setData(uploadedData); + buffer->open(QIODevice::ReadWrite); + serverStream->sendDATA(buffer, false); + + QVERIFY(!closedClientSpy.wait(std::chrono::seconds(1))); +} + +void tst_QHttp2Connection::testDataFrameAfterRSTOutgoing() +{ + auto [client, server] = makeFakeConnectedSockets(); + auto connection = makeHttp2Connection(client.get(), {}, Client); + auto serverConnection = makeHttp2Connection(server.get(), {}, Server); + + QVERIFY(waitForSettingsExchange(connection, serverConnection)); + + QSignalSpy newIncomingStreamSpy{ serverConnection, &QHttp2Connection::newIncomingStream }; + + QHttp2Stream *clientStream = connection->createStream().unwrap(); + QSignalSpy clientHeaderReceivedSpy{ clientStream, &QHttp2Stream::headersReceived }; + QVERIFY(clientStream); + HPack::HttpHeader headers = getRequiredHeaders(); + clientStream->sendHEADERS(headers, false); + + QVERIFY(newIncomingStreamSpy.wait()); + auto *serverStream = newIncomingStreamSpy.front().front().value(); + QCOMPARE(clientStream->streamID(), serverStream->streamID()); + + QSignalSpy rstServerSpy{ serverStream, &QHttp2Stream::rstFrameRecived }; + + QCOMPARE(clientStream->state(), QHttp2Stream::State::Open); + QCOMPARE(serverStream->state(), QHttp2Stream::State::Open); + + // Reset the stream and wait until it the RST_STREAM frame is received + clientStream->sendRST_STREAM(Http2::Http2Error::STREAM_CLOSED); + QVERIFY(rstServerSpy.wait()); + + QSignalSpy closedServerSpy{ serverConnection, &QHttp2Connection::connectionClosed }; + // Send data as if we didn't send the RST_STREAM + // It should trigger an error since we send the RST_STREAM frame ourself + clientStream->setState(QHttp2Stream::State::Open); + QBuffer *buffer = new QBuffer(serverStream); + QByteArray uploadedData = "Hello World"_ba.repeated(10); + buffer->setData(uploadedData); + buffer->open(QIODevice::ReadWrite); + clientStream->sendDATA(buffer, false); + + QVERIFY(closedServerSpy.wait()); +} + void tst_QHttp2Connection::connectToServer() { auto [client, server] = makeFakeConnectedSockets();