diff --git a/proxygen/lib/http/codec/HTTP2Codec.cpp b/proxygen/lib/http/codec/HTTP2Codec.cpp index b2cda1fd8..f03f99fc2 100644 --- a/proxygen/lib/http/codec/HTTP2Codec.cpp +++ b/proxygen/lib/http/codec/HTTP2Codec.cpp @@ -10,6 +10,7 @@ #include #include +#include #include #include @@ -28,6 +29,14 @@ namespace { const size_t kDefaultGrowth = 4000; } // namespace +#define RETURN_IF_ERROR_WITH_CTX(errExpr, context) \ + do { \ + if (auto ctxErr = (errExpr); ctxErr != ErrorCode::NO_ERROR) { \ + setParseErrorContext(context); \ + return ctxErr; \ + } \ + } while (0) + namespace proxygen { HTTP2Codec::HTTP2Codec(TransportDirection direction) @@ -66,8 +75,7 @@ size_t HTTP2Codec::onIngress(const folly::IOBuf& buf) noexcept { auto test = cursor.readFixedString(http2::kConnectionPreface.length()); parsed += http2::kConnectionPreface.length(); if (test != http2::kConnectionPreface) { - goawayErrorMessage_ = "missing connection preface"; - VLOG(4) << goawayErrorMessage_; + setParseErrorContext("missing-connection-preface"); connError = ErrorCode::PROTOCOL_ERROR; } frameState_ = FrameState::EXPECT_FIRST_SETTINGS; @@ -79,22 +87,27 @@ size_t HTTP2Codec::onIngress(const folly::IOBuf& buf) noexcept { // Waiting to parse the common frame header if (remaining >= http2::kFrameHeaderSize) { connError = parseFrameHeader(cursor, curHeader_); + if (connError != ErrorCode::NO_ERROR) { + setParseErrorContext("invalid-frame-header"); + } parsed += http2::kFrameHeaderSize; if (frameState_ == FrameState::EXPECT_FIRST_SETTINGS && curHeader_.type != http2::FrameType::SETTINGS) { - goawayErrorMessage_ = folly::to( - "GOAWAY error: got invalid connection preface frame type=", - getFrameTypeString(curHeader_.type), - "(", - curHeader_.type, - ")", - " for streamID=", - curHeader_.stream); - VLOG(4) << goawayErrorMessage_; + setParseErrorContext( + "invalid-connection-preface-frame-type", + folly::to("type=", + getFrameTypeString(curHeader_.type), + "(", + curHeader_.type, + ") stream=", + curHeader_.stream)); connError = ErrorCode::PROTOCOL_ERROR; } if (curHeader_.length > maxRecvFrameSize()) { - VLOG(4) << "Excessively large frame len=" << curHeader_.length; + setParseErrorContext( + "frame-exceeds-max-size", + folly::to( + "len=", curHeader_.length, " max=", maxRecvFrameSize())); connError = ErrorCode::FRAME_SIZE_ERROR; } @@ -152,25 +165,25 @@ size_t HTTP2Codec::onIngress(const folly::IOBuf& buf) noexcept { ErrorCode HTTP2Codec::parseFrame(folly::io::Cursor& cursor) { FOLLY_SCOPED_TRACE_SECTION("HTTP2Codec - parseFrame"); + parseErrorContext_.clear(); + parseErrorAdditionalInfo_.clear(); if (expectedContinuationStream_ != 0 && (curHeader_.type != http2::FrameType::CONTINUATION || expectedContinuationStream_ != curHeader_.stream)) { - goawayErrorMessage_ = folly::to( - "GOAWAY error: while expected CONTINUATION with stream=", - expectedContinuationStream_, - ", received streamID=", - curHeader_.stream, - " of type=", - getFrameTypeString(curHeader_.type)); - VLOG(4) << goawayErrorMessage_; + setParseErrorContext( + "continuation-stream-mismatch", + folly::to("expected=", + expectedContinuationStream_, + " received=", + curHeader_.stream, + " type=", + getFrameTypeString(curHeader_.type))); return ErrorCode::PROTOCOL_ERROR; } if (expectedContinuationStream_ == 0 && curHeader_.type == http2::FrameType::CONTINUATION) { - goawayErrorMessage_ = folly::to( - "GOAWAY error: unexpected CONTINUATION received with streamID=", - curHeader_.stream); - VLOG(4) << goawayErrorMessage_; + setParseErrorContext("unexpected-continuation-frame", + folly::to("stream=", curHeader_.stream)); return ErrorCode::PROTOCOL_ERROR; } if (frameAffectsCompression(curHeader_.type) && @@ -185,18 +198,20 @@ ErrorCode HTTP2Codec::parseFrame(folly::io::Cursor& cursor) { // block to keep the connection state consistent without consuming // memory, and fail just the request per the HTTP/2 spec (section // 10.3) - goawayErrorMessage_ = folly::to( - "Failing connection due to excessively large headers"); - LOG(ERROR) << goawayErrorMessage_; + setParseErrorContext( + "header-block-exceeds-max-size", + folly::to( + "len=", + curHeaderBlock_.chainLength() + curHeader_.length, + " max=", + egressSettings_.getSetting(SettingsId::MAX_HEADER_LIST_SIZE, 0))); return ErrorCode::PROTOCOL_ERROR; } if (curHeader_.type == http2::FrameType::CONTINUATION) { if (continuationFramesLeftInHeaderBlock_ == 0) { - goawayErrorMessage_ = folly::to( - "GOAWAY error: too many CONTINUATION frames for stream=", - curHeader_.stream); - LOG(ERROR) << goawayErrorMessage_; + setParseErrorContext("too-many-continuation-frames", + folly::to("stream=", curHeader_.stream)); return ErrorCode::ENHANCE_YOUR_CALM; } --continuationFramesLeftInHeaderBlock_; @@ -284,7 +299,7 @@ ErrorCode HTTP2Codec::parseAllData(Cursor& cursor) { VLOG(10) << "parsing all frame DATA bytes for stream=" << curHeader_.stream << " length=" << curHeader_.length; auto ret = http2::parseData(cursor, curHeader_, outData, padding); - RETURN_IF_ERROR(ret); + RETURN_IF_ERROR_WITH_CTX(ret, "parse-data-frame"); if (callback_ && (padding > 0 || (outData && !outData->empty()))) { if (!outData) { @@ -324,7 +339,7 @@ ErrorCode HTTP2Codec::parseDataFrameData(Cursor& cursor, return ErrorCode::NO_ERROR; } const auto ret = http2::parseDataBegin(cursor, curHeader_, parsed, padding); - RETURN_IF_ERROR(ret); + RETURN_IF_ERROR_WITH_CTX(ret, "parse-data-frame-begin"); if (padding > 0) { pendingDataFramePaddingBytes_ = padding - 1; pendingDataFrameBytes_--; @@ -357,7 +372,7 @@ ErrorCode HTTP2Codec::parseDataFrameData(Cursor& cursor, size_t toSkip = 0; auto ret = http2::parseDataEnd( cursor, bufLen, pendingDataFramePaddingBytes_, toSkip); - RETURN_IF_ERROR(ret); + RETURN_IF_ERROR_WITH_CTX(ret, "parse-data-frame-end"); pendingDataFrameBytes_ -= toSkip; pendingDataFramePaddingBytes_ -= toSkip; parsed += toSkip; @@ -387,10 +402,11 @@ ErrorCode HTTP2Codec::parseHeaders(Cursor& cursor) { VLOG(4) << "parsing HEADERS frame for stream=" << curHeader_.stream << " length=" << curHeader_.length; auto err = http2::parseHeaders(cursor, curHeader_, headerBuf); - RETURN_IF_ERROR(err); + RETURN_IF_ERROR_WITH_CTX(err, "parse-headers-frame"); if (isDownstream(transportDirection_)) { - RETURN_IF_ERROR( - checkNewStream(curHeader_.stream, true /* trailersAllowed */)); + auto newStreamErr = + checkNewStream(curHeader_.stream, true /* trailersAllowed */); + RETURN_IF_ERROR(newStreamErr); } err = parseHeadersImpl(cursor, std::move(headerBuf), folly::none); return err; @@ -401,7 +417,7 @@ ErrorCode HTTP2Codec::parseContinuation(Cursor& cursor) { VLOG(4) << "parsing CONTINUATION frame for stream=" << curHeader_.stream << " length=" << curHeader_.length; auto err = http2::parseContinuation(cursor, curHeader_, headerBuf); - RETURN_IF_ERROR(err); + RETURN_IF_ERROR_WITH_CTX(err, "parse-continuation-frame"); err = parseHeadersImpl(cursor, std::move(headerBuf), folly::none); return err; } @@ -502,9 +518,8 @@ ErrorCode HTTP2Codec::parseHeadersImpl( } else { if (isUpstream(transportDirection_) && curHeader_.stream & 0x01 && curHeader_.stream >= nextEgressStreamID_) { - goawayErrorMessage_ = folly::to( - "HEADERS on idle upstream stream=", curHeader_.stream); - LOG(ERROR) << goawayErrorMessage_; + setParseErrorContext("headers-on-idle-upstream-stream", + folly::to("stream=", curHeader_.stream)); return ErrorCode::PROTOCOL_ERROR; } deliverCallbackIfAllowed(&HTTPCodec::Callback::onHeadersComplete, @@ -538,22 +553,21 @@ HTTP2Codec::parseHeadersDecodeFrames() { auto g = folly::makeGuard([this] { curHeaderBlock_.move(); }); // Check decoding error if (decodeInfo_.decodeError != HPACK::DecodeError::NONE) { - constexpr std::string_view kDecodeErrorMessage = - "Failed decoding header block for stream="; // Avoid logging header blocks that have failed decoding due to being // excessively large. if (decodeInfo_.decodeError != HPACK::DecodeError::HEADERS_TOO_LARGE) { - goawayErrorMessage_ = - folly::to(kDecodeErrorMessage, - curHeader_.stream, - ": decompression error=", - uint32_t(decodeInfo_.decodeError)); - LOG(ERROR) << goawayErrorMessage_ + setParseErrorContext( + "header-block-decode-failed", + folly::to("stream=", + curHeader_.stream, + " decompression-error=", + uint32_t(decodeInfo_.decodeError))); + LOG(ERROR) << "Failed decoding header block " << parseErrorAdditionalInfo_ << (VLOG_IS_ON(3) ? ", header block=" : ""); VLOG(3) << IOBufPrinter::printHexFolly(curHeaderBlock_.front(), true); } else { - goawayErrorMessage_ = folly::to( - kDecodeErrorMessage, curHeader_.stream, ": headers too large"); + setParseErrorContext("header-block-too-large", + folly::to("stream=", curHeader_.stream)); if (debugLevel_ > 0 && msg) { LOG(ERROR) << "HPACK Headers too large" << CodecUtil::debugString(*msg, debugLevel_) @@ -599,7 +613,9 @@ HTTP2Codec::parseHeadersDecodeFrames() { ErrorCode::PROTOCOL_ERROR, false, folly::to("Field section parsing failed txn=", - curHeader_.stream), + curHeader_.stream, + " error: ", + decodeInfo_.parsingError), std::move(msg))); } } @@ -701,7 +717,7 @@ ErrorCode HTTP2Codec::parseRFC9218Priority(Cursor& cursor) { std::string pri; uint32_t priStream = 0; auto err = http2::parseRFC9218Priority(cursor, curHeader_, priStream, pri); - RETURN_IF_ERROR(err); + RETURN_IF_ERROR_WITH_CTX(err, "parse-priority-frame"); auto httpPri = httpPriorityFromString(pri); auto onPriFunc = static_cast( @@ -718,7 +734,7 @@ ErrorCode HTTP2Codec::parseRstStream(Cursor& cursor) { upgradedStreams_.erase(curHeader_.stream); ErrorCode statusCode = ErrorCode::NO_ERROR; auto err = http2::parseRstStream(cursor, curHeader_, statusCode); - RETURN_IF_ERROR(err); + RETURN_IF_ERROR_WITH_CTX(err, "parse-rst-stream-frame"); if (statusCode == ErrorCode::PROTOCOL_ERROR) { VLOG(3) << "RST_STREAM with code=" << getErrorCodeString(statusCode) << " for streamID=" << curHeader_.stream @@ -734,7 +750,7 @@ ErrorCode HTTP2Codec::parseSettings(Cursor& cursor) { << " length=" << curHeader_.length; std::deque settings; auto err = http2::parseSettings(cursor, curHeader_, settings); - RETURN_IF_ERROR(err); + RETURN_IF_ERROR_WITH_CTX(err, "parse-settings-frame"); if (curHeader_.flags & http2::ACK) { handleSettingsAck(); return ErrorCode::NO_ERROR; @@ -779,48 +795,40 @@ ErrorCode HTTP2Codec::handleSettings(const std::deque& settings) { case SettingsId::ENABLE_PUSH: if ((setting.second != 0 && setting.second != 1) || (setting.second == 1 && isUpstream(transportDirection_))) { - goawayErrorMessage_ = - folly::to("GOAWAY error: ENABLE_PUSH invalid setting=", - setting.second, - " for streamID=", - curHeader_.stream); - VLOG(4) << goawayErrorMessage_; + setParseErrorContext( + "settings-invalid-enable-push", + folly::to( + "value=", setting.second, " stream=", curHeader_.stream)); return ErrorCode::PROTOCOL_ERROR; } break; case SettingsId::INITIAL_WINDOW_SIZE: if (setting.second == 0 || setting.second > http2::kMaxWindowUpdateSize) { - goawayErrorMessage_ = folly::to( - "GOAWAY error: INITIAL_WINDOW_SIZE invalid size=", - setting.second, - " for streamID=", - curHeader_.stream); - VLOG(4) << goawayErrorMessage_; + setParseErrorContext( + "settings-invalid-initial-window-size", + folly::to( + "value=", setting.second, " stream=", curHeader_.stream)); return ErrorCode::PROTOCOL_ERROR; } break; case SettingsId::MAX_FRAME_SIZE: if (setting.second < http2::kMaxFramePayloadLengthMin || setting.second > http2::kMaxFramePayloadLength) { - goawayErrorMessage_ = - folly::to("GOAWAY error: MAX_FRAME_SIZE invalid size=", - setting.second, - " for streamID=", - curHeader_.stream); - VLOG(4) << goawayErrorMessage_; + setParseErrorContext( + "settings-invalid-max-frame-size", + folly::to( + "value=", setting.second, " stream=", curHeader_.stream)); return ErrorCode::PROTOCOL_ERROR; } ingressSettings_.setSetting(SettingsId::MAX_FRAME_SIZE, setting.second); break; case SettingsId::ENABLE_CONNECT_PROTOCOL: if (setting.second > 1) { - goawayErrorMessage_ = folly::to( - "GOAWAY error: ENABLE_CONNECT_PROTOCOL invalid number=", - setting.second, - " for streamID=", - curHeader_.stream); - VLOG(4) << goawayErrorMessage_; + setParseErrorContext( + "settings-invalid-enable-connect-protocol", + folly::to( + "value=", setting.second, " stream=", curHeader_.stream)); return ErrorCode::PROTOCOL_ERROR; } break; @@ -855,15 +863,11 @@ ErrorCode HTTP2Codec::parsePushPromise(Cursor& cursor) { created before the RST_STREAM frame is received and processed. */ if (isDownstream(transportDirection_)) { - goawayErrorMessage_ = - folly::to("Received PUSH_PROMISE on DOWNSTREAM codec"); - VLOG(2) << goawayErrorMessage_; + setParseErrorContext("received-push-promise-on-downstream-codec"); return ErrorCode::PROTOCOL_ERROR; } if (egressSettings_.getSetting(SettingsId::ENABLE_PUSH, -1) != 1) { - goawayErrorMessage_ = - folly::to("Received PUSH_PROMISE on codec with push disabled"); - VLOG(2) << goawayErrorMessage_; + setParseErrorContext("received-push-promise-with-push-disabled"); return ErrorCode::PROTOCOL_ERROR; } VLOG(4) << "parsing PUSH_PROMISE frame for stream=" << curHeader_.stream @@ -872,8 +876,10 @@ ErrorCode HTTP2Codec::parsePushPromise(Cursor& cursor) { std::unique_ptr headerBlockFragment; auto err = http2::parsePushPromise( cursor, curHeader_, promisedStream, headerBlockFragment); - RETURN_IF_ERROR(err); - RETURN_IF_ERROR(checkNewStream(promisedStream, false /* trailersAllowed */)); + RETURN_IF_ERROR_WITH_CTX(err, "parse-push-promise-frame"); + auto newStreamErr = + checkNewStream(promisedStream, false /* trailersAllowed */); + RETURN_IF_ERROR(newStreamErr); err = parseHeadersImpl(cursor, std::move(headerBlockFragment), promisedStream); return err; @@ -883,7 +889,7 @@ ErrorCode HTTP2Codec::parsePing(Cursor& cursor) { VLOG(4) << "parsing PING frame length=" << curHeader_.length; uint64_t opaqueData = 0; auto err = http2::parsePing(cursor, curHeader_, opaqueData); - RETURN_IF_ERROR(err); + RETURN_IF_ERROR_WITH_CTX(err, "parse-ping-frame"); if (callback_) { if (curHeader_.flags & http2::ACK) { callback_->onPingReply(opaqueData); @@ -908,7 +914,7 @@ ErrorCode HTTP2Codec::parseGoaway(Cursor& cursor) { << " debugData=" << ((debugData) ? debugData->to() : empty_string); } - RETURN_IF_ERROR(err); + RETURN_IF_ERROR_WITH_CTX(err, "parse-goaway-frame"); if (lastGoodStream < ingressGoawayAck_) { ingressGoawayAck_ = lastGoodStream; // Drain all streams <= lastGoodStream @@ -927,22 +933,22 @@ ErrorCode HTTP2Codec::parseWindowUpdate(Cursor& cursor) { << " length=" << curHeader_.length; uint32_t delta = 0; auto err = http2::parseWindowUpdate(cursor, curHeader_, delta); - RETURN_IF_ERROR(err); + RETURN_IF_ERROR_WITH_CTX(err, "parse-window-update-frame"); if (delta == 0) { VLOG(4) << "Invalid 0 length delta for stream=" << curHeader_.stream; if (curHeader_.stream == 0) { - goawayErrorMessage_ = folly::to( - "GOAWAY error: invalid/0 length delta for streamID=", - curHeader_.stream); + setParseErrorContext("window-update-zero-delta-connection", "stream=0"); return ErrorCode::PROTOCOL_ERROR; } else { // Parsing a zero delta window update should cause a protocol error // and send a rst stream - goawayErrorMessage_ = folly::to( - "streamID=", curHeader_.stream, " with window update delta=", delta); - VLOG(4) << goawayErrorMessage_; - streamError(goawayErrorMessage_, ErrorCode::PROTOCOL_ERROR); + auto additionalInfo = folly::to("stream=", curHeader_.stream); + streamError( + folly::to("window-update-zero-delta-stream ", additionalInfo), + ErrorCode::PROTOCOL_ERROR); // Stream error and protocol error + setParseErrorContext("window-update-zero-delta-stream", + std::move(additionalInfo)); return ErrorCode::PROTOCOL_ERROR; } } @@ -957,13 +963,14 @@ ErrorCode HTTP2Codec::parseWindowUpdate(Cursor& cursor) { ErrorCode HTTP2Codec::checkNewStream(uint32_t streamId, bool trailersAllowed) { bool existingStream = (streamId <= lastStreamID_); - if (streamId == 0 || (!trailersAllowed && existingStream)) { - goawayErrorMessage_ = - folly::to("GOAWAY error: received streamID=", - streamId, - " as invalid new stream for lastStreamID_=", - lastStreamID_); - VLOG(4) << goawayErrorMessage_; + if (streamId == 0) { + setParseErrorContext("new-stream-zero-id"); + return ErrorCode::PROTOCOL_ERROR; + } + if (!trailersAllowed && existingStream) { + setParseErrorContext( + "new-stream-not-monotonic", + folly::to("stream=", streamId, " last=", lastStreamID_)); return ErrorCode::PROTOCOL_ERROR; } parsingDownstreamTrailers_ = trailersAllowed && existingStream; @@ -977,9 +984,8 @@ ErrorCode HTTP2Codec::checkNewStream(uint32_t streamId, bool trailersAllowed) { if (isInitiatedStream(streamId)) { // this stream should be initiated by us, not by peer - goawayErrorMessage_ = folly::to( - "GOAWAY error: invalid new stream received with streamID=", streamId); - VLOG(4) << goawayErrorMessage_; + setParseErrorContext("new-stream-wrong-initiator", + folly::to("stream=", streamId)); return ErrorCode::PROTOCOL_ERROR; } else { return ErrorCode::NO_ERROR; @@ -1537,14 +1543,22 @@ size_t HTTP2Codec::generatePriority(folly::IOBufQueue& writeBuf, bool HTTP2Codec::checkConnectionError(ErrorCode err, const folly::IOBuf* buf) { if (err != ErrorCode::NO_ERROR) { - std::string errorDescription = - goawayErrorMessage_.empty() ? "Connection error" : goawayErrorMessage_; + std::string context; + if (!parseErrorContext_.empty()) { + context = folly::to("[Context]=", parseErrorContext_); + if (!parseErrorAdditionalInfo_.empty()) { + context = + folly::to(context, " ", parseErrorAdditionalInfo_); + } + } LOG(ERROR) << "Connection error " << getErrorCodeString(err) << " " - << errorDescription << " with ingress="; + << context << (context.empty() ? "" : " ") << "with ingress="; VLOG(3) << IOBufPrinter::printHexFolly(buf, true); if (callback_) { HTTPException ex(HTTPException::Direction::INGRESS_AND_EGRESS, - errorDescription); + context.empty() ? std::string("Connection error") + : folly::to( + "Connection error ", context)); ex.setCodecStatusCode(err); callback_->onError(0, ex, false); } @@ -1553,6 +1567,15 @@ bool HTTP2Codec::checkConnectionError(ErrorCode err, const folly::IOBuf* buf) { return false; } +void HTTP2Codec::setParseErrorContext(std::string context, + std::string additionalInfo) { + parseErrorContext_ = std::move(context); + parseErrorAdditionalInfo_ = std::move(additionalInfo); + VLOG(4) << "parse error context=" << parseErrorContext_ + << (parseErrorAdditionalInfo_.empty() ? "" : " ") + << parseErrorAdditionalInfo_; +} + void HTTP2Codec::streamError(const std::string& msg, ErrorCode code, bool newTxn, diff --git a/proxygen/lib/http/codec/HTTP2Codec.h b/proxygen/lib/http/codec/HTTP2Codec.h index bf24a9f08..65f392125 100644 --- a/proxygen/lib/http/codec/HTTP2Codec.h +++ b/proxygen/lib/http/codec/HTTP2Codec.h @@ -251,6 +251,8 @@ class HTTP2Codec ErrorCode handleEndStream(); ErrorCode checkNewStream(uint32_t stream, bool trailersAllowed); bool checkConnectionError(ErrorCode, const folly::IOBuf* buf); + void setParseErrorContext(std::string context, + std::string additionalInfo = {}); ErrorCode handleSettings(const std::deque& settings); void handleSettingsAck(); size_t maxSendFrameSize() const { @@ -273,6 +275,8 @@ class HTTP2Codec // Current frame state http2::FrameHeader curHeader_; + std::string parseErrorContext_; + std::string parseErrorAdditionalInfo_; StreamID expectedContinuationStream_{0}; // Used for parsing PUSH_PROMISE+CONTINUATION folly::Optional promisedStream_; diff --git a/proxygen/lib/http/codec/HTTPParallelCodec.h b/proxygen/lib/http/codec/HTTPParallelCodec.h index aab31b190..fb3cbf065 100644 --- a/proxygen/lib/http/codec/HTTPParallelCodec.h +++ b/proxygen/lib/http/codec/HTTPParallelCodec.h @@ -99,7 +99,6 @@ class HTTPParallelCodec : public HTTPCodec { HTTPCodec::Callback* callback_{nullptr}; StreamID ingressGoawayAck_{std::numeric_limits::max()}; StreamID egressGoawayAck_{std::numeric_limits::max()}; - std::string goawayErrorMessage_; enum ClosingState { OPEN = 0, diff --git a/proxygen/lib/http/codec/HeaderDecodeInfo.cpp b/proxygen/lib/http/codec/HeaderDecodeInfo.cpp index 5a538cbdc..ec41e9d10 100644 --- a/proxygen/lib/http/codec/HeaderDecodeInfo.cpp +++ b/proxygen/lib/http/codec/HeaderDecodeInfo.cpp @@ -31,8 +31,11 @@ bool HeaderDecodeInfo::onHeader(const HPACKHeaderName& name, if (nameSp.startsWith(':')) { pseudoHeaderSeen_ = true; + if (firstPseudoHeader_.empty()) { + firstPseudoHeader_ = nameSp.str(); + } if (regularHeaderSeen_) { - parsingError = folly::to("Illegal pseudo header name=", nameSp); + parsingError = folly::to("illegal-pseudo-header name=", nameSp); return false; } if (isRequest_) { @@ -69,7 +72,8 @@ bool HeaderDecodeInfo::onHeader(const HPACKHeaderName& name, } break; default: - parsingError = folly::to("Invalid req header name=", nameSp); + parsingError = + folly::to("invalid-request-pseudo-header name=", nameSp); return false; } if (!ok) { @@ -78,7 +82,10 @@ bool HeaderDecodeInfo::onHeader(const HPACKHeaderName& name, } else { if (headerCode == HTTP_HEADER_COLON_STATUS) { if (hasStatus_) { - parsingError = string("Duplicate status"); + parsingError = folly::to("duplicate-status existing=", + msg->getStatusCode(), + " new=", + valueSp); return false; } hasStatus_ = true; @@ -89,11 +96,12 @@ bool HeaderDecodeInfo::onHeader(const HPACKHeaderName& name, msg->setStatusCode(code); msg->setStatusMessage(HTTPMessage::getDefaultReason(code)); } else { - parsingError = folly::to("Malformed status code=", valueSp); + parsingError = folly::to("malformed-status code=", valueSp); return false; } } else { - parsingError = folly::to("Invalid resp header name=", nameSp); + parsingError = + folly::to("invalid-response-pseudo-header name=", nameSp); return false; } } @@ -101,14 +109,16 @@ bool HeaderDecodeInfo::onHeader(const HPACKHeaderName& name, regularHeaderSeen_ = true; switch (headerCode) { case HTTP_HEADER_CONNECTION: - parsingError = string("HTTP/2 Message with Connection header"); + parsingError = + folly::to("connection-header-forbidden value=", valueSp); return false; case HTTP_HEADER_CONTENT_LENGTH: { const auto cl = headers.getSingleOrNullptr(HTTP_HEADER_CONTENT_LENGTH); if (cl) { bool ok = *cl == valueSp; if (!ok) { - parsingError = string("Multiple content-length headers"); + parsingError = folly::to( + "multiple-content-length existing=", *cl, " new=", valueSp); } return ok; // skips adding if already present and equal } @@ -118,7 +128,11 @@ bool HeaderDecodeInfo::onHeader(const HPACKHeaderName& name, if (verifier.hasAuthority()) { // HTTP_HEADER_HOST already present bool ok = headers.getSingleOrEmpty(HTTP_HEADER_HOST) == valueSp; if (!ok) { - parsingError = ":authority/Host header mismatch"; + parsingError = + folly::to("authority-host-mismatch existing=", + headers.getSingleOrEmpty(HTTP_HEADER_HOST), + " new=", + valueSp); } return ok; // skips adding if already present and equal } @@ -133,16 +147,30 @@ bool HeaderDecodeInfo::onHeader(const HPACKHeaderName& name, nameSp, strictValidation_ ? CodecUtil::HEADER_NAME_STRICT : CodecUtil::HEADER_NAME_STRICT_COMPAT); - bool valueOk = - !validate_ || - CodecUtil::validateHeaderValue( - valueSp, - strictValidation_ ? CodecUtil::CtlEscapeMode::STRICT - : CodecUtil::CtlEscapeMode::STRICT_COMPAT); - if (!nameOk || !valueOk) { + auto valueError = + validate_ + ? CodecUtil::validateHeaderValueDetail( + valueSp, + strictValidation_ ? CodecUtil::CtlEscapeMode::STRICT + : CodecUtil::CtlEscapeMode::STRICT_COMPAT) + : CodecUtil::HeaderValueError::None; + if (!nameOk || valueError != CodecUtil::HeaderValueError::None) { proxygenError = nameSp.empty() ? kErrorParseHeader : kErrorHeaderContentValidation; - parsingError = folly::to("Invalid header name=", nameSp); + std::string reason = + nameSp.empty() ? "empty-name" : (!nameOk ? "invalid-name-char" : ""); + if (valueError != CodecUtil::HeaderValueError::None) { + if (!reason.empty()) { + reason += ","; + } + reason += CodecUtil::describeHeaderValueError(valueError); + } + parsingError = folly::to(isRequestTrailers_ ? "invalid-trailer " + : "invalid-header ", + "name=", + nameSp, + " reason=", + reason); headerErrorValue = valueSp; return false; } @@ -170,7 +198,8 @@ void HeaderDecodeInfo::onHeadersComplete(HTTPHeaderSize decodedSize) { bool isResponseTrailers = (!isRequest_ && !hasStatus_); if ((isRequestTrailers_ || isResponseTrailers) && pseudoHeaderSeen_) { - parsingError = "Pseudo headers forbidden in trailers."; + parsingError = folly::to("pseudo-header-in-trailers name=", + firstPseudoHeader_); return; } diff --git a/proxygen/lib/http/codec/HeaderDecodeInfo.h b/proxygen/lib/http/codec/HeaderDecodeInfo.h index d0f5315fd..22009d471 100644 --- a/proxygen/lib/http/codec/HeaderDecodeInfo.h +++ b/proxygen/lib/http/codec/HeaderDecodeInfo.h @@ -36,6 +36,7 @@ class HeaderDecodeInfo { pseudoHeaderSeen_ = false; parsingError.clear(); headerErrorValue.clear(); + firstPseudoHeader_.clear(); decodeError = HPACK::DecodeError::NONE; strictValidation_ = strictValidation; allowEmptyPath_ = allowEmptyPath; @@ -67,6 +68,7 @@ class HeaderDecodeInfo { // Default to false for now to match existing behavior bool strictValidation_{false}; bool allowEmptyPath_{false}; + std::string firstPseudoHeader_; }; } // namespace proxygen diff --git a/proxygen/lib/http/codec/test/HTTP2CodecTest.cpp b/proxygen/lib/http/codec/test/HTTP2CodecTest.cpp index b2a190d6f..f9b8102ec 100644 --- a/proxygen/lib/http/codec/test/HTTP2CodecTest.cpp +++ b/proxygen/lib/http/codec/test/HTTP2CodecTest.cpp @@ -176,6 +176,27 @@ TEST_F(HTTP2CodecTestOmitParsePreface, OmitSettingsAfterConnPrefaceError) { EXPECT_EQ(callbacks_.sessionErrors, 1); EXPECT_EQ(callbacks_.lastParseError->getCodecStatusCode(), ErrorCode::PROTOCOL_ERROR); + EXPECT_THAT(callbacks_.lastParseError->what(), + HasSubstr("[Context]=invalid-connection-preface-frame-type")); + // The numeric type is what identifies a frame the name mapping doesn't know. + EXPECT_THAT(callbacks_.lastParseError->what(), HasSubstr("type=HEADERS(1)")); +} + +TEST_F(HTTP2CodecTestOmitParsePreface, OversizedNonSettingsPrefaceFrame) { + // The preface check and the frame-size check are independent ifs, so a first + // frame that is both non-SETTINGS and oversized trips both. The reported + // message and context must describe the same failure. + writeFrameHeaderManual( + output_, 1 << 15, (uint8_t)http2::FrameType::HEADERS, 0, 1); + + parse(); + EXPECT_EQ(callbacks_.sessionErrors, 1); + EXPECT_EQ(callbacks_.lastParseError->getCodecStatusCode(), + ErrorCode::FRAME_SIZE_ERROR); + EXPECT_THAT(callbacks_.lastParseError->what(), + HasSubstr("[Context]=frame-exceeds-max-size")); + EXPECT_THAT(callbacks_.lastParseError->what(), + Not(HasSubstr("invalid connection preface"))); } TEST_F(HTTP2CodecTest, BadHeaders) { @@ -954,6 +975,8 @@ TEST_F(HTTP2CodecTest, FrameTooLarge) { EXPECT_TRUE(callbacks_.lastParseError->hasCodecStatusCode()); EXPECT_EQ(callbacks_.lastParseError->getCodecStatusCode(), ErrorCode::FRAME_SIZE_ERROR); + EXPECT_THAT(callbacks_.lastParseError->what(), + HasSubstr("[Context]=frame-exceeds-max-size")); } TEST_F(HTTP2CodecTest, DataFrameZeroLengthWithEOM) { diff --git a/proxygen/lib/http/session/test/HTTPDownstreamSessionTest.cpp b/proxygen/lib/http/session/test/HTTPDownstreamSessionTest.cpp index d7a24f0ea..1ba36090a 100644 --- a/proxygen/lib/http/session/test/HTTPDownstreamSessionTest.cpp +++ b/proxygen/lib/http/session/test/HTTPDownstreamSessionTest.cpp @@ -3033,7 +3033,8 @@ TEST_F(HTTP2DownstreamSessionTest, ZeroDeltaWindowUpdate) { handler->expectHeaders(); handler->expectError([&](const HTTPException& ex) { ASSERT_EQ(ex.getCodecStatusCode(), ErrorCode::PROTOCOL_ERROR); - ASSERT_EQ("streamID=1 with window update delta=0", std::string(ex.what())); + ASSERT_EQ("window-update-zero-delta-stream stream=1", + std::string(ex.what())); }); handler->expectDetachTransaction(); expectDetachSession();