diff --git a/proxygen/lib/http/codec/HQControlCodec.cpp b/proxygen/lib/http/codec/HQControlCodec.cpp index a7a04765e..b97f50031 100644 --- a/proxygen/lib/http/codec/HQControlCodec.cpp +++ b/proxygen/lib/http/codec/HQControlCodec.cpp @@ -6,6 +6,7 @@ * LICENSE file in the root directory of this source tree. */ +#include #include #include @@ -36,6 +37,9 @@ ParseResult HQControlCodec::checkFrameAllowed(FrameType type) { case hq::FrameType::HEADERS: case hq::FrameType::PUSH_PROMISE: case hq::FrameType::WEBTRANSPORT_BIDI: + setParseErrorContext( + "data-frame-on-control-stream", + folly::to("type=", getFrameTypeString(type))); return HTTP3::ErrorCode::HTTP_FRAME_UNEXPECTED; default: break; @@ -44,16 +48,21 @@ ParseResult HQControlCodec::checkFrameAllowed(FrameType type) { if (getStreamType() == hq::UnidirectionalStreamType::CONTROL) { // SETTINGS MUST be the first frame on an HQ Control Stream if (!receivedSettings_ && type != hq::FrameType::SETTINGS) { + setParseErrorContext( + "settings-not-first-frame", + folly::to("type=", getFrameTypeString(type))); return HTTP3::ErrorCode::HTTP_MISSING_SETTINGS; } // multiple SETTINGS frames are not allowed if (receivedSettings_ && type == hq::FrameType::SETTINGS) { + setParseErrorContext("duplicate-settings"); return HTTP3::ErrorCode::HTTP_FRAME_UNEXPECTED; } // A client MUST treat the receipt of a MAX_PUSH_ID frame as a connection // error of type HTTP_FRAME_UNEXPECTED if (transportDirection_ == TransportDirection::UPSTREAM && type == hq::FrameType::MAX_PUSH_ID) { + setParseErrorContext("max-push-id-on-upstream-control-stream"); return HTTP3::ErrorCode::HTTP_FRAME_UNEXPECTED; } @@ -63,6 +72,9 @@ ParseResult HQControlCodec::checkFrameAllowed(FrameType type) { type == hq::FrameType::PUSH_PRIORITY_UPDATE || type == hq::FrameType::FB_PUSH_PRIORITY_UPDATE || type == hq::FrameType::FB_PRIORITY_UPDATE)) { + setParseErrorContext( + "priority-update-on-upstream-control-stream", + folly::to("type=", getFrameTypeString(type))); return HTTP3::ErrorCode::HTTP_FRAME_UNEXPECTED; } } @@ -107,6 +119,12 @@ ParseResult HQControlCodec::parseSettings(Cursor& cursor, case hq::SettingId::WT_ENABLED: // only 0/1 are legal if (setting.second > 1) { + setParseErrorContext( + "boolean-setting-out-of-range", + folly::to("id=", + static_cast(setting.first), + " value=", + setting.second)); return HTTP3::ErrorCode::HTTP_SETTINGS_ERROR; } break; diff --git a/proxygen/lib/http/codec/HQFramedCodec.cpp b/proxygen/lib/http/codec/HQFramedCodec.cpp index 2c7f67181..31a2c46b2 100644 --- a/proxygen/lib/http/codec/HQFramedCodec.cpp +++ b/proxygen/lib/http/codec/HQFramedCodec.cpp @@ -76,11 +76,20 @@ size_t HQFramedCodec::onFramedIngress(const IOBuf& buf) { } curHeader_.type = FrameType(type->first); parsed += type->second; + parseErrorContext_ = {}; + parseErrorAdditionalInfo_ = {}; auto res = checkFrameAllowed(curHeader_.type); if (res) { VLOG(4) << "Frame not allowed: 0x" << std::setfill('0') << std::setw(sizeof(uint64_t) * 2) << std::hex << (uint64_t)curHeader_.type << " on streamID=" << streamId_; + // Only generic if the derived codec did not name a specific reason. + if (parseErrorContext_.empty()) { + setParseErrorContext( + "frame-not-allowed", + folly::to("type=", + getFrameTypeString(curHeader_.type))); + } connError_ = res; break; } @@ -157,6 +166,9 @@ bool HQFramedCodec::onFramedIngressEOF() { return false; } else if (frameState_ != FrameState::FRAME_HEADER_TYPE) { VLOG(3) << "Stream ended in the middle of a frame type=" << curHeader_.type; + setParseErrorContext( + "eof-in-middle-of-frame", + folly::to("type=", getFrameTypeString(curHeader_.type))); connError_ = HTTP3::ErrorCode::HTTP_FRAME_ERROR; checkConnectionError(connError_, nullptr); return false; @@ -168,14 +180,23 @@ bool HQFramedCodec::onFramedIngressEOF() { bool HQFramedCodec::checkConnectionError(ParseResult err, const folly::IOBuf* buf) { if (err != folly::none) { - LOG(ERROR) << "Connection error with ingress="; + std::string errorMessage("Connection error"); + if (!parseErrorContext_.empty()) { + errorMessage = folly::to( + errorMessage, " [Context]=", parseErrorContext_); + if (!parseErrorAdditionalInfo_.empty()) { + errorMessage = folly::to( + errorMessage, " ", parseErrorAdditionalInfo_); + } + } + LOG(ERROR) << errorMessage << " with ingress="; if (buf) { VLOG(3) << IOBufPrinter::printHexFolly(buf, true); } setParserPaused(true); if (callback_) { HTTPException ex(HTTPException::Direction::INGRESS_AND_EGRESS, - "Connection error"); + errorMessage); ex.setHttp3ErrorCode(err.value()); callback_->onError(kSessionStreamId, ex, false); } diff --git a/proxygen/lib/http/codec/HQFramedCodec.h b/proxygen/lib/http/codec/HQFramedCodec.h index 8f5870f57..8d3180d6d 100644 --- a/proxygen/lib/http/codec/HQFramedCodec.h +++ b/proxygen/lib/http/codec/HQFramedCodec.h @@ -264,6 +264,20 @@ class HQFramedCodec : public HTTPCodec { } protected: + /** + * Record why framing was rejected. HQ frame errors reach + * checkConnectionError as a bare HTTP3::ErrorCode, so without this every + * failure in the frame layer reports the same "Connection error" text. + */ + void setParseErrorContext(std::string context, + std::string additionalInfo = {}) { + parseErrorContext_ = std::move(context); + parseErrorAdditionalInfo_ = std::move(additionalInfo); + VLOG(4) << "parse error context=" << parseErrorContext_ + << (parseErrorAdditionalInfo_.empty() ? "" : " ") + << parseErrorAdditionalInfo_; + } + virtual ParseResult checkFrameAllowed(FrameType type) = 0; virtual ParseResult parseData(folly::io::Cursor& /*cursor*/, @@ -352,6 +366,8 @@ class HQFramedCodec : public HTTPCodec { }; FrameState frameState_ : 3; ParseResult connError_{folly::none}; + std::string parseErrorContext_; + std::string parseErrorAdditionalInfo_; uint64_t totalBytesParsed_{0}; folly::Function resumeHook_; }; diff --git a/proxygen/lib/http/codec/HQFramer.cpp b/proxygen/lib/http/codec/HQFramer.cpp index 467a27f2c..e7b0aa358 100644 --- a/proxygen/lib/http/codec/HQFramer.cpp +++ b/proxygen/lib/http/codec/HQFramer.cpp @@ -387,7 +387,7 @@ WriteResult writeStreamPreface(folly::IOBufQueue& writeBuf, return *streamPrefaceSize; } -static const char* getFrameTypeString(FrameType type) { +const char* getFrameTypeString(FrameType type) { switch (type) { case FrameType::DATA: return "DATA"; diff --git a/proxygen/lib/http/codec/HQFramer.h b/proxygen/lib/http/codec/HQFramer.h index 51ea06854..cd9af4d7f 100644 --- a/proxygen/lib/http/codec/HQFramer.h +++ b/proxygen/lib/http/codec/HQFramer.h @@ -86,6 +86,8 @@ enum class FrameType : uint64_t { }; +const char* getFrameTypeString(FrameType type); + std::ostream& operator<<(std::ostream& os, FrameType type); struct FrameHeader { diff --git a/proxygen/lib/http/codec/HQStreamCodec.cpp b/proxygen/lib/http/codec/HQStreamCodec.cpp index b14e04555..4a87c5a15 100644 --- a/proxygen/lib/http/codec/HQStreamCodec.cpp +++ b/proxygen/lib/http/codec/HQStreamCodec.cpp @@ -42,6 +42,9 @@ HQStreamCodec::~HQStreamCodec() = default; ParseResult HQStreamCodec::checkFrameAllowed(FrameType type) { if (isConnect_ && type != hq::FrameType::DATA) { + setParseErrorContext( + "non-data-frame-on-connect", + folly::to("type=", getFrameTypeString(type))); return HTTP3::ErrorCode::HTTP_FRAME_UNEXPECTED; } switch (type) { @@ -54,9 +57,13 @@ ParseResult HQStreamCodec::checkFrameAllowed(FrameType type) { case hq::FrameType::FB_PRIORITY_UPDATE: case hq::FrameType::FB_PUSH_PRIORITY_UPDATE: case hq::FrameType::WEBTRANSPORT_BIDI: + setParseErrorContext( + "control-frame-on-request-stream", + folly::to("type=", getFrameTypeString(type))); return HTTP3::ErrorCode::HTTP_FRAME_UNEXPECTED; case hq::FrameType::PUSH_PROMISE: if (transportDirection_ == TransportDirection::DOWNSTREAM) { + setParseErrorContext("received-push-promise-on-downstream-codec"); return HTTP3::ErrorCode::HTTP_FRAME_UNEXPECTED; } break; diff --git a/proxygen/lib/http/codec/test/HQCodecTest.cpp b/proxygen/lib/http/codec/test/HQCodecTest.cpp index 1fc9f64b5..f46fb645c 100644 --- a/proxygen/lib/http/codec/test/HQCodecTest.cpp +++ b/proxygen/lib/http/codec/test/HQCodecTest.cpp @@ -1143,6 +1143,8 @@ TEST_P(HQCodecTestFrameBeforeSettings, FrameAllowedOnControlCodec) { } else { EXPECT_EQ(callbacks_.lastParseError->getHttp3ErrorCode(), HTTP3::ErrorCode::HTTP_MISSING_SETTINGS); + EXPECT_THAT(callbacks_.lastParseError->what(), + HasSubstr("[Context]=settings-not-first-frame")); } } }