diff --git a/quic/QuicConstants.h b/quic/QuicConstants.h index 27e4f4435..b644030a6 100644 --- a/quic/QuicConstants.h +++ b/quic/QuicConstants.h @@ -94,7 +94,8 @@ enum class FrameType : uint8_t { PATH_CHALLENGE = 0x1A, PATH_RESPONSE = 0x1B, CONNECTION_CLOSE = 0x1C, - APPLICATION_CLOSE = 0x1D, + // CONNECTION_CLOSE_APP_ERR frametype is use to indicate application errors + CONNECTION_CLOSE_APP_ERR = 0x1D, MIN_STREAM_DATA = 0xFE, // subject to change (https://fburl.com/qpr) EXPIRED_STREAM_DATA = 0xFF, // subject to change (https://fburl.com/qpr) }; diff --git a/quic/api/QuicTransportFunctions.cpp b/quic/api/QuicTransportFunctions.cpp index 091a6db04..34bf02720 100644 --- a/quic/api/QuicTransportFunctions.cpp +++ b/quic/api/QuicTransportFunctions.cpp @@ -743,29 +743,33 @@ void writeCloseCommon( if (!closeDetails) { written = writeFrame( ConnectionCloseFrame( - TransportErrorCode::NO_ERROR, std::string("No error")), + QuicErrorCode(TransportErrorCode::NO_ERROR), + std::string("No error")), packetBuilder); } else { switch (closeDetails->first.type()) { case QuicErrorCode::Type::ApplicationErrorCode_E: written = writeFrame( - ApplicationCloseFrame( - *closeDetails->first.asApplicationErrorCode(), - closeDetails->second), + ConnectionCloseFrame( + QuicErrorCode(*closeDetails->first.asApplicationErrorCode()), + closeDetails->second, + quic::FrameType::CONNECTION_CLOSE_APP_ERR), packetBuilder); break; case QuicErrorCode::Type::TransportErrorCode_E: written = writeFrame( ConnectionCloseFrame( - *closeDetails->first.asTransportErrorCode(), - closeDetails->second), + QuicErrorCode(*closeDetails->first.asTransportErrorCode()), + closeDetails->second, + quic::FrameType::CONNECTION_CLOSE), packetBuilder); break; case QuicErrorCode::Type::LocalErrorCode_E: written = writeFrame( ConnectionCloseFrame( - TransportErrorCode::INTERNAL_ERROR, - std::string("Internal error")), + QuicErrorCode(TransportErrorCode::INTERNAL_ERROR), + std::string("Internal error"), + quic::FrameType::CONNECTION_CLOSE), packetBuilder); break; } diff --git a/quic/api/test/QuicPacketSchedulerTest.cpp b/quic/api/test/QuicPacketSchedulerTest.cpp index 37f32ca15..43626cf78 100644 --- a/quic/api/test/QuicPacketSchedulerTest.cpp +++ b/quic/api/test/QuicPacketSchedulerTest.cpp @@ -364,7 +364,8 @@ TEST_F(QuicPacketSchedulerTest, WriteOnlyOutstandingPacketsTest) { // Create few frames ConnectionCloseFrame connCloseFrame( - TransportErrorCode::FRAME_ENCODING_ERROR, "The sun is in the sky."); + QuicErrorCode(TransportErrorCode::FRAME_ENCODING_ERROR), + "The sun is in the sky."); MaxStreamsFrame maxStreamFrame(999, true); PingFrame pingFrame; IntervalSet ackBlocks; diff --git a/quic/client/QuicClientTransport.cpp b/quic/client/QuicClientTransport.cpp index 7431bfd8f..88a5d808f 100644 --- a/quic/client/QuicClientTransport.cpp +++ b/quic/client/QuicClientTransport.cpp @@ -486,21 +486,6 @@ void QuicClientTransport::processPacketData( "Peer closed", TransportErrorCode::NO_ERROR); break; } - case QuicFrame::Type::ApplicationCloseFrame_E: { - ApplicationCloseFrame& appClose = *quicFrame.asApplicationCloseFrame(); - auto errMsg = folly::to( - "Client closed by peer reason=", appClose.reasonPhrase); - VLOG(4) << errMsg << " " << *this; - if (conn_->qLogger) { - conn_->qLogger->addTransportStateUpdate(getPeerClose(errMsg)); - } - QUIC_TRACE(recvd_close, *conn_, errMsg.c_str()); - conn_->peerConnectionError = std::make_pair( - QuicErrorCode(appClose.errorCode), std::move(errMsg)); - throw QuicTransportException( - "Peer closed", TransportErrorCode::NO_ERROR); - break; - } case QuicFrame::Type::PaddingFrame_E: { break; } diff --git a/quic/client/test/QuicClientTransportTest.cpp b/quic/client/test/QuicClientTransportTest.cpp index 6b25a5152..9115ce015 100644 --- a/quic/client/test/QuicClientTransportTest.cpp +++ b/quic/client/test/QuicClientTransportTest.cpp @@ -3207,7 +3207,7 @@ TEST_P( EXPECT_TRUE(verifyFramePresent( socketWrites, *makeHandshakeCodec(), - QuicFrame::Type::ApplicationCloseFrame_E)); + QuicFrame::Type::ConnectionCloseFrame_E)); std::vector indices = getQLogEventIndices(QLogEventType::ConnectionClose, qLogger); @@ -3376,7 +3376,7 @@ TEST_P(QuicClientTransportAfterStartTestClose, CloseConnectionWithError) { EXPECT_TRUE(verifyFramePresent( socketWrites, *makeEncryptedCodec(), - QuicFrame::Type::ApplicationCloseFrame_E)); + QuicFrame::Type::ConnectionCloseFrame_E)); } else { client->close(folly::none); EXPECT_TRUE(verifyFramePresent( @@ -3589,7 +3589,7 @@ TEST_F(QuicClientTransportAfterStartTest, IdleTimeoutExpired) { auto serverCodec = makeEncryptedCodec(); // We expect a conn close in a cleartext packet. EXPECT_FALSE(verifyFramePresent( - socketWrites, *serverCodec, QuicFrame::Type::ApplicationCloseFrame_E)); + socketWrites, *serverCodec, QuicFrame::Type::ConnectionCloseFrame_E)); EXPECT_FALSE(verifyFramePresent( socketWrites, *serverCodec, QuicFrame::Type::ConnectionCloseFrame_E)); EXPECT_TRUE(socketWrites.empty()); @@ -4520,7 +4520,8 @@ TEST_F(QuicClientTransportAfterStartTest, ReceiveConnectionClose) { 0, QuicVersion::MVFST); ConnectionCloseFrame connClose( - TransportErrorCode::NO_ERROR, "Stand clear of the closing doors, please"); + QuicErrorCode(TransportErrorCode::NO_ERROR), + "Stand clear of the closing doors, please"); writeFrame(std::move(connClose), builder); auto packet = packetToBuf(std::move(builder).buildPacket()); EXPECT_CALL(clientConnCallback, onConnectionEnd()); @@ -4547,8 +4548,8 @@ TEST_F(QuicClientTransportAfterStartTest, ReceiveApplicationClose) { std::move(header), 0, QuicVersion::MVFST); - ApplicationCloseFrame appClose( - GenericApplicationErrorCode::UNKNOWN, + ConnectionCloseFrame appClose( + QuicErrorCode(GenericApplicationErrorCode::UNKNOWN), "Stand clear of the closing doors, please"); writeFrame(std::move(appClose), builder); auto packet = packetToBuf(std::move(builder).buildPacket()); diff --git a/quic/codec/Decode.cpp b/quic/codec/Decode.cpp index 1a2400406..85087ef63 100644 --- a/quic/codec/Decode.cpp +++ b/quic/codec/Decode.cpp @@ -582,10 +582,10 @@ ConnectionCloseFrame decodeConnectionCloseFrame( auto reasonPhrase = cursor.readFixedString(folly::to(reasonPhraseLength->first)); return ConnectionCloseFrame( - errorCode, std::move(reasonPhrase), triggeringFrameType); + QuicErrorCode(errorCode), std::move(reasonPhrase), triggeringFrameType); } -ApplicationCloseFrame decodeApplicationCloseFrame( +ConnectionCloseFrame decodeApplicationClose( folly::io::Cursor& cursor, const CodecParameters& params) { ApplicationErrorCode errorCode{}; @@ -600,9 +600,19 @@ ApplicationCloseFrame decodeApplicationCloseFrame( throw QuicTransportException( "Failed to parse error code.", quic::TransportErrorCode::FRAME_ENCODING_ERROR, - quic::FrameType::APPLICATION_CLOSE); + quic::FrameType::CONNECTION_CLOSE_APP_ERR); } } + + auto frameTypeField = decodeQuicInteger(cursor); + if (UNLIKELY(!frameTypeField || frameTypeField->second != sizeof(uint8_t))) { + throw QuicTransportException( + "Bad connection close triggering frame type value", + quic::TransportErrorCode::FRAME_ENCODING_ERROR, + quic::FrameType::CONNECTION_CLOSE); + } + auto triggeringFrameType = static_cast(frameTypeField->first); + auto reasonPhraseLength = decodeQuicInteger(cursor); if (UNLIKELY( !reasonPhraseLength || @@ -610,11 +620,13 @@ ApplicationCloseFrame decodeApplicationCloseFrame( throw QuicTransportException( "Bad reason phrase length", quic::TransportErrorCode::FRAME_ENCODING_ERROR, - quic::FrameType::APPLICATION_CLOSE); + quic::FrameType::CONNECTION_CLOSE_APP_ERR); } + auto reasonPhrase = cursor.readFixedString(folly::to(reasonPhraseLength->first)); - return ApplicationCloseFrame(errorCode, std::move(reasonPhrase)); + return ConnectionCloseFrame( + QuicErrorCode(errorCode), std::move(reasonPhrase), triggeringFrameType); } MinStreamDataFrame decodeMinStreamDataFrame(folly::io::Cursor& cursor) { @@ -734,8 +746,8 @@ QuicFrame parseFrame( return QuicFrame(decodePathResponseFrame(cursor)); case FrameType::CONNECTION_CLOSE: return QuicFrame(decodeConnectionCloseFrame(cursor, params)); - case FrameType::APPLICATION_CLOSE: - return QuicFrame(decodeApplicationCloseFrame(cursor, params)); + case FrameType::CONNECTION_CLOSE_APP_ERR: + return QuicFrame(decodeApplicationClose(cursor, params)); case FrameType::MIN_STREAM_DATA: return QuicFrame(decodeMinStreamDataFrame(cursor)); case FrameType::EXPIRED_STREAM_DATA: diff --git a/quic/codec/Decode.h b/quic/codec/Decode.h index 2b99189dc..cf220368b 100644 --- a/quic/codec/Decode.h +++ b/quic/codec/Decode.h @@ -82,7 +82,7 @@ ConnectionCloseFrame decodeConnectionCloseFrame( folly::io::Cursor& cursor, const CodecParameters& params); -ApplicationCloseFrame decodeApplicationCloseFrame( +ConnectionCloseFrame decodeApplicationClose( folly::io::Cursor& cursor, const CodecParameters& params); diff --git a/quic/codec/QuicWriteCodec.cpp b/quic/codec/QuicWriteCodec.cpp index f8be9bb53..a3b862b00 100644 --- a/quic/codec/QuicWriteCodec.cpp +++ b/quic/codec/QuicWriteCodec.cpp @@ -619,13 +619,25 @@ size_t writeFrame(QuicWriteFrame&& frame, PacketBuilderInterface& builder) { case QuicWriteFrame::Type::ConnectionCloseFrame_E: { ConnectionCloseFrame& connectionCloseFrame = *frame.asConnectionCloseFrame(); - QuicInteger intFrameType( - static_cast(FrameType::CONNECTION_CLOSE)); + // Need to distinguish between CONNECTION_CLOSE & CONNECTINO_CLOSE_APP_ERR + const TransportErrorCode* isTransportErrorCode = + connectionCloseFrame.errorCode.asTransportErrorCode(); + const ApplicationErrorCode* isApplicationErrorCode = + connectionCloseFrame.errorCode.asApplicationErrorCode(); + + QuicInteger intFrameType(static_cast( + isTransportErrorCode ? FrameType::CONNECTION_CLOSE + : FrameType::CONNECTION_CLOSE_APP_ERR)); + QuicInteger reasonLength(connectionCloseFrame.reasonPhrase.size()); QuicInteger closingFrameType( static_cast(connectionCloseFrame.closingFrameType)); + QuicInteger errorCode( - static_cast(connectionCloseFrame.errorCode)); + isTransportErrorCode + ? static_cast(TransportErrorCode(*isTransportErrorCode)) + : static_cast( + ApplicationErrorCode(*isApplicationErrorCode))); auto version = builder.getVersion(); size_t errorSize = version == QuicVersion::MVFST_OLD ? sizeof(TransportErrorCode) @@ -637,8 +649,10 @@ size_t writeFrame(QuicWriteFrame&& frame, PacketBuilderInterface& builder) { builder.write(intFrameType); if (version == QuicVersion::MVFST_OLD) { builder.writeBE( - static_cast::type>( - connectionCloseFrame.errorCode)); + isTransportErrorCode ? static_cast(TransportErrorCode( + *isTransportErrorCode)) + : static_cast(ApplicationErrorCode( + *isApplicationErrorCode))); } else { builder.write(errorCode); } @@ -653,38 +667,6 @@ size_t writeFrame(QuicWriteFrame&& frame, PacketBuilderInterface& builder) { // no space left in packet return size_t(0); } - case QuicWriteFrame::Type::ApplicationCloseFrame_E: { - ApplicationCloseFrame& applicationCloseFrame = - *frame.asApplicationCloseFrame(); - QuicInteger intFrameType( - static_cast(FrameType::APPLICATION_CLOSE)); - QuicInteger reasonLength(applicationCloseFrame.reasonPhrase.size()); - QuicInteger errorCode( - static_cast(applicationCloseFrame.errorCode)); - auto version = builder.getVersion(); - size_t errorSize = version == QuicVersion::MVFST_OLD - ? sizeof(ApplicationErrorCode) - : errorCode.getSize(); - auto applicationCloseFrameSize = intFrameType.getSize() + errorSize + - reasonLength.getSize() + applicationCloseFrame.reasonPhrase.size(); - if (packetSpaceCheck(spaceLeft, applicationCloseFrameSize)) { - builder.write(intFrameType); - if (version == QuicVersion::MVFST_OLD) { - builder.writeBE(static_cast( - applicationCloseFrame.errorCode)); - } else { - builder.write(errorCode); - } - builder.write(reasonLength); - builder.push( - (const uint8_t*)applicationCloseFrame.reasonPhrase.data(), - applicationCloseFrame.reasonPhrase.size()); - builder.appendFrame(std::move(applicationCloseFrame)); - return applicationCloseFrameSize; - } - // no space left in packet - return size_t(0); - } case QuicWriteFrame::Type::QuicSimpleFrame_E: { return writeSimpleFrame(std::move(*frame.asQuicSimpleFrame()), builder); } diff --git a/quic/codec/Types.cpp b/quic/codec/Types.cpp index b2cb7598c..4e9005add 100644 --- a/quic/codec/Types.cpp +++ b/quic/codec/Types.cpp @@ -442,7 +442,7 @@ std::string toString(FrameType frame) { return "PATH_RESPONSE"; case FrameType::CONNECTION_CLOSE: return "CONNECTION_CLOSE"; - case FrameType::APPLICATION_CLOSE: + case FrameType::CONNECTION_CLOSE_APP_ERR: return "APPLICATION_CLOSE"; case FrameType::MIN_STREAM_DATA: return "MIN_STREAM_DATA"; diff --git a/quic/codec/Types.h b/quic/codec/Types.h index 55c0aef4a..94a1e2d45 100644 --- a/quic/codec/Types.h +++ b/quic/codec/Types.h @@ -505,17 +505,17 @@ struct PathResponseFrame { struct ConnectionCloseFrame { // Members are not const to allow this to be movable. - TransportErrorCode errorCode; + QuicErrorCode errorCode; std::string reasonPhrase; // Per QUIC specification: type of frame that triggered the (close) error. // A value of 0 (PADDING frame) implies the frame type is unknown FrameType closingFrameType; ConnectionCloseFrame( - TransportErrorCode errorCodeIn, + QuicErrorCode errorCodeIn, std::string reasonPhraseIn, FrameType closingFrameTypeIn = FrameType::PADDING) - : errorCode(errorCodeIn), + : errorCode(std::move(errorCodeIn)), reasonPhrase(std::move(reasonPhraseIn)), closingFrameType(closingFrameTypeIn) {} @@ -528,21 +528,6 @@ struct ConnectionCloseFrame { } }; -struct ApplicationCloseFrame { - // Members are not const to allow this to be movable. - ApplicationErrorCode errorCode; - std::string reasonPhrase; - - ApplicationCloseFrame( - ApplicationErrorCode errorCodeIn, - std::string reasonPhraseIn) - : errorCode(errorCodeIn), reasonPhrase(std::move(reasonPhraseIn)) {} - - bool operator==(const ApplicationCloseFrame& rhs) const { - return errorCode == rhs.errorCode && reasonPhrase == rhs.reasonPhrase; - } -}; - // Frame to represent ones we skip struct NoopFrame { bool operator==(const NoopFrame&) const { @@ -574,7 +559,6 @@ DECLARE_VARIANT_TYPE(QuicSimpleFrame, QUIC_SIMPLE_FRAME) F(PaddingFrame, __VA_ARGS__) \ F(RstStreamFrame, __VA_ARGS__) \ F(ConnectionCloseFrame, __VA_ARGS__) \ - F(ApplicationCloseFrame, __VA_ARGS__) \ F(MaxDataFrame, __VA_ARGS__) \ F(MaxStreamDataFrame, __VA_ARGS__) \ F(DataBlockedFrame, __VA_ARGS__) \ @@ -593,7 +577,6 @@ DECLARE_VARIANT_TYPE(QuicFrame, QUIC_FRAME) F(PaddingFrame, __VA_ARGS__) \ F(RstStreamFrame, __VA_ARGS__) \ F(ConnectionCloseFrame, __VA_ARGS__) \ - F(ApplicationCloseFrame, __VA_ARGS__) \ F(MaxDataFrame, __VA_ARGS__) \ F(MaxStreamDataFrame, __VA_ARGS__) \ F(DataBlockedFrame, __VA_ARGS__) \ diff --git a/quic/codec/test/QuicPacketRebuilderTest.cpp b/quic/codec/test/QuicPacketRebuilderTest.cpp index e11977f15..3c6f0196d 100644 --- a/quic/codec/test/QuicPacketRebuilderTest.cpp +++ b/quic/codec/test/QuicPacketRebuilderTest.cpp @@ -59,7 +59,7 @@ TEST_F(QuicPacketRebuilderTest, RebuildPacket) { // Get a bunch frames ConnectionCloseFrame connCloseFrame( - TransportErrorCode::FRAME_ENCODING_ERROR, + QuicErrorCode(TransportErrorCode::FRAME_ENCODING_ERROR), "The sun is in the sky.", FrameType::ACK); MaxStreamsFrame maxStreamsFrame(4321, true); @@ -129,8 +129,10 @@ TEST_F(QuicPacketRebuilderTest, RebuildPacket) { case QuicWriteFrame::Type::ConnectionCloseFrame_E: { const ConnectionCloseFrame& closeFrame = *frame.asConnectionCloseFrame(); + const TransportErrorCode* transportErrorCode = + closeFrame.errorCode.asTransportErrorCode(); EXPECT_EQ( - TransportErrorCode::FRAME_ENCODING_ERROR, closeFrame.errorCode); + TransportErrorCode::FRAME_ENCODING_ERROR, *transportErrorCode); EXPECT_EQ("The sun is in the sky.", closeFrame.reasonPhrase); EXPECT_EQ(FrameType::ACK, closeFrame.closingFrameType); break; @@ -363,7 +365,7 @@ TEST_F(QuicPacketRebuilderTest, CannotRebuild) { // Get a bunch frames ConnectionCloseFrame connCloseFrame( - TransportErrorCode::FRAME_ENCODING_ERROR, + QuicErrorCode(TransportErrorCode::FRAME_ENCODING_ERROR), "The sun is in the sky.", FrameType::ACK); StreamsBlockedFrame maxStreamIdFrame(0x1024, true); diff --git a/quic/codec/test/QuicWriteCodecTest.cpp b/quic/codec/test/QuicWriteCodecTest.cpp index cd74f27ca..6d073bf65 100644 --- a/quic/codec/test/QuicWriteCodecTest.cpp +++ b/quic/codec/test/QuicWriteCodecTest.cpp @@ -1033,7 +1033,7 @@ TEST_F(QuicWriteCodecTest, WriteConnClose) { setupCommonExpects(pktBuilder); std::string reasonPhrase("You are fired"); ConnectionCloseFrame connectionCloseFrame( - TransportErrorCode::PROTOCOL_VIOLATION, reasonPhrase); + QuicErrorCode(TransportErrorCode::PROTOCOL_VIOLATION), reasonPhrase); auto connCloseBytesWritten = writeFrame(std::move(connectionCloseFrame), pktBuilder); @@ -1043,16 +1043,18 @@ TEST_F(QuicWriteCodecTest, WriteConnClose) { EXPECT_EQ(4 + reasonPhrase.size(), connCloseBytesWritten); auto& resultConnCloseFrame = *regularPacket.frames[0].asConnectionCloseFrame(); - EXPECT_EQ( - TransportErrorCode::PROTOCOL_VIOLATION, resultConnCloseFrame.errorCode); + const TransportErrorCode* transportErrorCode = + resultConnCloseFrame.errorCode.asTransportErrorCode(); + EXPECT_EQ(TransportErrorCode::PROTOCOL_VIOLATION, *transportErrorCode); EXPECT_EQ("You are fired", resultConnCloseFrame.reasonPhrase); auto wireBuf = std::move(builtOut.second); folly::io::Cursor cursor(wireBuf.get()); QuicFrame decodedCloseFrame = parseQuicFrame(cursor); auto& wireConnCloseFrame = *decodedCloseFrame.asConnectionCloseFrame(); - EXPECT_EQ( - TransportErrorCode::PROTOCOL_VIOLATION, wireConnCloseFrame.errorCode); + const TransportErrorCode* protocolViolationCode = + wireConnCloseFrame.errorCode.asTransportErrorCode(); + EXPECT_EQ(TransportErrorCode::PROTOCOL_VIOLATION, *protocolViolationCode); EXPECT_EQ("You are fired", wireConnCloseFrame.reasonPhrase); // At last, verify there is nothing left in the wire format bytes: @@ -1065,14 +1067,15 @@ TEST_F(QuicWriteCodecTest, DecodeConnCloseLarge) { std::string reasonPhrase; reasonPhrase.resize(kMaxReasonPhraseLength + 10); ConnectionCloseFrame connectionCloseFrame( - TransportErrorCode::PROTOCOL_VIOLATION, reasonPhrase); + QuicErrorCode(TransportErrorCode::PROTOCOL_VIOLATION), reasonPhrase); writeFrame(connectionCloseFrame, pktBuilder); auto builtOut = std::move(pktBuilder).buildPacket(); auto regularPacket = builtOut.first; auto& resultConnCloseFrame = *regularPacket.frames[0].asConnectionCloseFrame(); - EXPECT_EQ( - TransportErrorCode::PROTOCOL_VIOLATION, resultConnCloseFrame.errorCode); + const TransportErrorCode* protocolViolationCode = + resultConnCloseFrame.errorCode.asTransportErrorCode(); + EXPECT_EQ(TransportErrorCode::PROTOCOL_VIOLATION, *protocolViolationCode); EXPECT_EQ(resultConnCloseFrame.reasonPhrase, reasonPhrase); auto wireBuf = std::move(builtOut.second); @@ -1086,7 +1089,7 @@ TEST_F(QuicWriteCodecTest, NoSpaceConnClose) { setupCommonExpects(pktBuilder); std::string reasonPhrase("You are all fired"); ConnectionCloseFrame connCloseFrame( - TransportErrorCode::PROTOCOL_VIOLATION, reasonPhrase); + QuicErrorCode(TransportErrorCode::PROTOCOL_VIOLATION), reasonPhrase); EXPECT_EQ(0, writeFrame(std::move(connCloseFrame), pktBuilder)); } @@ -1095,16 +1098,18 @@ TEST_F(QuicWriteCodecTest, DecodeAppCloseLarge) { setupCommonExpects(pktBuilder); std::string reasonPhrase; reasonPhrase.resize(kMaxReasonPhraseLength + 10); - ApplicationCloseFrame applicationCloseFrame( - GenericApplicationErrorCode::UNKNOWN, reasonPhrase); + ConnectionCloseFrame applicationCloseFrame( + QuicErrorCode(GenericApplicationErrorCode::UNKNOWN), + reasonPhrase, + quic::FrameType::CONNECTION_CLOSE_APP_ERR); writeFrame(std::move(applicationCloseFrame), pktBuilder); auto builtOut = std::move(pktBuilder).buildPacket(); auto regularPacket = builtOut.first; - auto& resultAppCloseFrame = - *regularPacket.frames[0].asApplicationCloseFrame(); + auto& resultAppCloseFrame = *regularPacket.frames[0].asConnectionCloseFrame(); EXPECT_EQ( - GenericApplicationErrorCode::UNKNOWN, resultAppCloseFrame.errorCode); + quic::FrameType::CONNECTION_CLOSE_APP_ERR, + resultAppCloseFrame.closingFrameType); EXPECT_EQ(resultAppCloseFrame.reasonPhrase, reasonPhrase); auto wireBuf = std::move(builtOut.second); diff --git a/quic/logging/BaseQLogger.cpp b/quic/logging/BaseQLogger.cpp index ed3470b8a..f0fa58026 100644 --- a/quic/logging/BaseQLogger.cpp +++ b/quic/logging/BaseQLogger.cpp @@ -111,12 +111,6 @@ std::unique_ptr BaseQLogger::createPacketEvent( frame.errorCode, frame.reasonPhrase, frame.closingFrameType)); break; } - case QuicFrame::Type::ApplicationCloseFrame_E: { - const auto& frame = *quicFrame.asApplicationCloseFrame(); - event->frames.push_back(std::make_unique( - frame.errorCode, frame.reasonPhrase)); - break; - } case QuicFrame::Type::MaxDataFrame_E: { const auto& frame = *quicFrame.asMaxDataFrame(); event->frames.push_back( @@ -221,13 +215,6 @@ std::unique_ptr BaseQLogger::createPacketEvent( frame.errorCode, frame.reasonPhrase, frame.closingFrameType)); break; } - case QuicWriteFrame::Type::ApplicationCloseFrame_E: { - const ApplicationCloseFrame& frame = - *quicFrame.asApplicationCloseFrame(); - event->frames.push_back(std::make_unique( - frame.errorCode, frame.reasonPhrase)); - break; - } case QuicWriteFrame::Type::MaxDataFrame_E: { const MaxDataFrame& frame = *quicFrame.asMaxDataFrame(); event->frames.push_back( diff --git a/quic/logging/QLoggerTypes.cpp b/quic/logging/QLoggerTypes.cpp index 65a9d7325..9340dba5f 100644 --- a/quic/logging/QLoggerTypes.cpp +++ b/quic/logging/QLoggerTypes.cpp @@ -6,6 +6,7 @@ * */ #include +#include #include namespace quic { @@ -28,21 +29,23 @@ folly::dynamic RstStreamFrameLog::toDynamic() const { folly::dynamic ConnectionCloseFrameLog::toDynamic() const { folly::dynamic d = folly::dynamic::object(); - d["frame_type"] = toString(FrameType::CONNECTION_CLOSE); + + auto isTransportErrorCode = errorCode.asTransportErrorCode(); + auto isApplicationErrorCode = errorCode.asApplicationErrorCode(); + auto isLocalErrorCode = errorCode.asLocalErrorCode(); + + if (isTransportErrorCode || isLocalErrorCode) { + d["frame_type"] = toString(FrameType::CONNECTION_CLOSE); + } else if (isApplicationErrorCode) { + d["frame_type"] = toString(FrameType::CONNECTION_CLOSE_APP_ERR); + } + d["error_code"] = toString(errorCode); d["reason_phrase"] = reasonPhrase; d["closing_frame_type"] = toString(closingFrameType); return d; } -folly::dynamic ApplicationCloseFrameLog::toDynamic() const { - folly::dynamic d = folly::dynamic::object(); - d["frame_type"] = toString(FrameType::APPLICATION_CLOSE); - d["error_code"] = errorCode; - d["reason_phrase"] = reasonPhrase; - return d; -} - folly::dynamic MaxDataFrameLog::toDynamic() const { folly::dynamic d = folly::dynamic::object(); d["frame_type"] = toString(FrameType::MAX_DATA); diff --git a/quic/logging/QLoggerTypes.h b/quic/logging/QLoggerTypes.h index 73863a98a..ed6ae1829 100644 --- a/quic/logging/QLoggerTypes.h +++ b/quic/logging/QLoggerTypes.h @@ -51,15 +51,15 @@ class RstStreamFrameLog : public QLogFrame { class ConnectionCloseFrameLog : public QLogFrame { public: - TransportErrorCode errorCode; + QuicErrorCode errorCode; std::string reasonPhrase; FrameType closingFrameType; ConnectionCloseFrameLog( - TransportErrorCode errorCodeIn, + QuicErrorCode errorCodeIn, std::string reasonPhraseIn, FrameType closingFrameTypeIn) - : errorCode{errorCodeIn}, + : errorCode{std::move(errorCodeIn)}, reasonPhrase{std::move(reasonPhraseIn)}, closingFrameType{closingFrameTypeIn} {} @@ -67,20 +67,6 @@ class ConnectionCloseFrameLog : public QLogFrame { folly::dynamic toDynamic() const override; }; -class ApplicationCloseFrameLog : public QLogFrame { - public: - ApplicationErrorCode errorCode; - std::string reasonPhrase; - - ApplicationCloseFrameLog( - ApplicationErrorCode errorCodeIn, - std::string reasonPhraseIn) - : errorCode{errorCodeIn}, reasonPhrase{std::move(reasonPhraseIn)} {} - - ~ApplicationCloseFrameLog() override = default; - folly::dynamic toDynamic() const override; -}; - class MaxDataFrameLog : public QLogFrame { public: uint64_t maximumData; diff --git a/quic/server/state/ServerStateMachine.cpp b/quic/server/state/ServerStateMachine.cpp index 6ba2d2ace..b8f7acec1 100644 --- a/quic/server/state/ServerStateMachine.cpp +++ b/quic/server/state/ServerStateMachine.cpp @@ -951,25 +951,6 @@ void onServerReadDataFromOpen( "Peer closed", TransportErrorCode::NO_ERROR); break; } - case QuicFrame::Type::ApplicationCloseFrame_E: { - isNonProbingPacket = true; - ApplicationCloseFrame& appClose = - *quicFrame.asApplicationCloseFrame(); - auto errMsg = folly::to( - "Server closed by peer reason=", appClose.reasonPhrase); - VLOG(10) << errMsg << " " << conn; - // we want to deliver app callbacks with the peer supplied error, - // but send a NO_ERROR to the peer. - QUIC_TRACE(recvd_close, conn, errMsg.c_str()); - if (conn.qLogger) { - conn.qLogger->addTransportStateUpdate(getPeerClose(errMsg)); - } - conn.peerConnectionError = std::make_pair( - QuicErrorCode(appClose.errorCode), std::move(errMsg)); - throw QuicTransportException( - "Peer closed", TransportErrorCode::NO_ERROR); - break; - } case QuicFrame::Type::PaddingFrame_E: { break; } @@ -1140,7 +1121,6 @@ void onServerReadDataFromClosed( } auto& regularPacket = *regularOptional; - auto protectionLevel = regularPacket.header.getProtectionType(); auto packetNum = regularPacket.header.getPacketSequenceNum(); auto pnSpace = regularPacket.header.getPacketNumberSpace(); if (conn.qLogger) { @@ -1148,9 +1128,6 @@ void onServerReadDataFromClosed( } QUIC_TRACE(packet_recvd, conn, packetNum, packetSize); - bool isProtectedPacket = protectionLevel == ProtectionType::ZeroRtt || - protectionLevel == ProtectionType::KeyPhaseZero || - protectionLevel == ProtectionType::KeyPhaseOne; // Only process the close frames in the packet for (auto& quicFrame : regularPacket.frames) { @@ -1170,25 +1147,6 @@ void onServerReadDataFromClosed( QuicErrorCode(connFrame.errorCode), std::move(errMsg)); break; } - case QuicFrame::Type::ApplicationCloseFrame_E: { - if (!isProtectedPacket) { - return; - } - ApplicationCloseFrame& appClose = *quicFrame.asApplicationCloseFrame(); - auto errMsg = folly::to( - "Server closed by peer reason=", appClose.reasonPhrase); - VLOG(10) << errMsg << " " << conn; - if (conn.qLogger) { - conn.qLogger->addTransportStateUpdate(getPeerClose(errMsg)); - } - // we want to deliver app callbacks with the peer supplied error, - // but send a NO_ERROR to the peer. - QUIC_TRACE(recvd_close, conn, errMsg.c_str()); - conn.peerConnectionError = std::make_pair( - QuicErrorCode(appClose.errorCode), std::move(errMsg)); - - break; - } default: break; } diff --git a/quic/server/test/QuicServerTransportTest.cpp b/quic/server/test/QuicServerTransportTest.cpp index 20d924b34..2b26786e7 100644 --- a/quic/server/test/QuicServerTransportTest.cpp +++ b/quic/server/test/QuicServerTransportTest.cpp @@ -870,9 +870,7 @@ TEST_F(QuicServerTransportTest, IdleTimeoutExpired) { EXPECT_TRUE(server->isClosed()); auto serverReadCodec = makeClientEncryptedCodec(); EXPECT_FALSE(verifyFramePresent( - serverWrites, - *serverReadCodec, - QuicFrame::Type::ApplicationCloseFrame_E)); + serverWrites, *serverReadCodec, QuicFrame::Type::ConnectionCloseFrame_E)); EXPECT_FALSE(verifyFramePresent( serverWrites, *serverReadCodec, QuicFrame::Type::ConnectionCloseFrame_E)); } @@ -901,7 +899,7 @@ TEST_F(QuicServerTransportTest, TestCloseConnectionWithError) { EXPECT_TRUE(verifyFramePresent( serverWrites, *makeClientEncryptedCodec(), - QuicFrame::Type::ApplicationCloseFrame_E)); + QuicFrame::Type::ConnectionCloseFrame_E)); } TEST_F(QuicServerTransportTest, TestCloseConnectionWithNoError) { @@ -911,7 +909,7 @@ TEST_F(QuicServerTransportTest, TestCloseConnectionWithNoError) { EXPECT_TRUE(verifyFramePresent( serverWrites, *makeClientEncryptedCodec(), - QuicFrame::Type::ApplicationCloseFrame_E)); + QuicFrame::Type::ConnectionCloseFrame_E)); } TEST_F(QuicServerTransportTest, TestClientAddressChanges) { @@ -968,7 +966,7 @@ TEST_F(QuicServerTransportTest, TestCloseConnectionWithNoErrorPendingStreams) { EXPECT_TRUE(verifyFramePresent( serverWrites, *makeClientEncryptedCodec(), - QuicFrame::Type::ApplicationCloseFrame_E)); + QuicFrame::Type::ConnectionCloseFrame_E)); } TEST_F(QuicServerTransportTest, ReceivePacketAfterLocalError) { @@ -1052,7 +1050,8 @@ TEST_F(QuicServerTransportTest, ReceiveCloseAfterLocalError) { std::move(header2), 0 /* largestAcked */); std::string errMsg = "Mind the gap"; - ConnectionCloseFrame connClose(TransportErrorCode::NO_ERROR, errMsg); + ConnectionCloseFrame connClose( + QuicErrorCode(TransportErrorCode::NO_ERROR), errMsg); writeFrame(std::move(connClose), builder2); auto packet2 = std::move(builder2).buildPacket(); @@ -1101,7 +1100,8 @@ TEST_F(QuicServerTransportTest, NoDataExceptCloseProcessedAfterClosing) { true); writeStreamFrameData(builder, buf->clone(), buf->computeChainDataLength()); std::string errMsg = "Mind the gap"; - ConnectionCloseFrame connClose(TransportErrorCode::NO_ERROR, errMsg); + ConnectionCloseFrame connClose( + QuicErrorCode(TransportErrorCode::NO_ERROR), errMsg); writeFrame(std::move(connClose), builder); auto packet = std::move(builder).buildPacket(); @@ -1112,7 +1112,7 @@ TEST_F(QuicServerTransportTest, NoDataExceptCloseProcessedAfterClosing) { EXPECT_TRUE(verifyFramePresent( serverWrites, *makeClientEncryptedCodec(), - QuicFrame::Type::ApplicationCloseFrame_E)); + QuicFrame::Type::ConnectionCloseFrame_E)); EXPECT_TRUE(hasNotReceivedNewPacketsSinceLastCloseSent(server->getConn())); serverWrites.clear(); @@ -1129,7 +1129,7 @@ TEST_F(QuicServerTransportTest, NoDataExceptCloseProcessedAfterClosing) { EXPECT_FALSE(verifyFramePresent( serverWrites, *makeClientEncryptedCodec(), - QuicFrame::Type::ApplicationCloseFrame_E)); + QuicFrame::Type::ConnectionCloseFrame_E)); EXPECT_EQ(server->getConn().streamManager->streamCount(), 0); } @@ -1842,7 +1842,8 @@ TEST_F(QuicServerTransportTest, ReceiveConnectionClose) { 0 /* largestAcked */, QuicVersion::MVFST); std::string errMsg = "Stand clear of the closing doors, please"; - ConnectionCloseFrame connClose(TransportErrorCode::NO_ERROR, errMsg); + ConnectionCloseFrame connClose( + QuicErrorCode(TransportErrorCode::NO_ERROR), errMsg); writeFrame(std::move(connClose), builder); auto packet = std::move(builder).buildPacket(); EXPECT_CALL(connCallback, onConnectionEnd()); @@ -1878,8 +1879,10 @@ TEST_F(QuicServerTransportTest, ReceiveApplicationClose) { std::move(header), 0 /* largestAcked */, QuicVersion::MVFST); + std::string errMsg = "Stand clear of the closing doors, please"; - ApplicationCloseFrame appClose(GenericApplicationErrorCode::UNKNOWN, errMsg); + ConnectionCloseFrame appClose( + QuicErrorCode(GenericApplicationErrorCode::UNKNOWN), errMsg); writeFrame(std::move(appClose), builder); auto packet = std::move(builder).buildPacket(); @@ -1918,7 +1921,8 @@ TEST_F(QuicServerTransportTest, ReceiveConnectionCloseTwice) { 0 /* largestAcked */, QuicVersion::MVFST); std::string errMsg = "Mind the gap"; - ConnectionCloseFrame connClose(TransportErrorCode::NO_ERROR, errMsg); + ConnectionCloseFrame connClose( + QuicErrorCode(TransportErrorCode::NO_ERROR), errMsg); writeFrame(std::move(connClose), builder); auto packet = std::move(builder).buildPacket(); EXPECT_CALL(connCallback, onConnectionEnd());