From 614ba5dcc9e57d324f55ebd407691d64a3c23260 Mon Sep 17 00:00:00 2001 From: Yang Song Date: Sat, 27 Jul 2019 09:08:34 +0800 Subject: [PATCH 1/3] add metadata size limit test Signed-off-by: Yang Song --- source/common/http/http2/codec_impl.cc | 25 ++++++++++++++++++++- test/integration/http2_integration_test.cc | 26 ++++++++++++++++++++++ 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index 475aa1d314e28..a67d5ea4b19e1 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -185,6 +185,7 @@ void ConnectionImpl::StreamImpl::pendingRecvBufferLowWatermark() { void ConnectionImpl::StreamImpl::decodeHeaders() { maybeTransformUpgradeFromH2ToH1(); + ASSERT(decoder_ != nullptr); decoder_->decodeHeaders(std::move(headers_), remote_end_stream_); } @@ -334,6 +335,7 @@ MetadataDecoder& ConnectionImpl::StreamImpl::getMetadataDecoder() { } void ConnectionImpl::StreamImpl::onMetadataDecoded(MetadataMapPtr&& metadata_map_ptr) { + ASSERT(decoder_ != nullptr); decoder_->decodeMetadata(std::move(metadata_map_ptr)); } @@ -430,6 +432,7 @@ int ConnectionImpl::onFrameReceived(const nghttp2_frame* frame) { if (stream->headers_->Status()->value() == "100") { ASSERT(!stream->remote_end_stream_); + ASSERT(stream->decoder_ != nullptr); stream->decoder_->decode100ContinueHeaders(std::move(stream->headers_)); } else { stream->decodeHeaders(); @@ -457,6 +460,7 @@ int ConnectionImpl::onFrameReceived(const nghttp2_frame* frame) { stats_.too_many_header_frames_.inc(); throw CodecProtocolException("Unexpected 'trailers' with no end stream."); } else { + ASSERT(stream->decoder_ != nullptr); stream->decoder_->decodeTrailers(std::move(stream->headers_)); } } else { @@ -487,6 +491,7 @@ int ConnectionImpl::onFrameReceived(const nghttp2_frame* frame) { // It's possible that we are waiting to send a deferred reset, so only raise data if local // is not complete. if (!stream->deferred_reset_) { + ASSERT(stream->decoder_ != nullptr); stream->decoder_->decodeData(stream->pending_recv_data_, stream->remote_end_stream_); } @@ -539,11 +544,11 @@ int ConnectionImpl::onInvalidFrame(int32_t stream_id, int error_code) { ENVOY_CONN_LOG(debug, "invalid frame: {} on stream {}", connection_, nghttp2_strerror(error_code), stream_id); + StreamImpl* stream = getStream(stream_id); // The stream is about to be closed due to an invalid header or messaging. Don't kill the // entire connection if one stream has bad headers or messaging. if (error_code == NGHTTP2_ERR_HTTP_HEADER || error_code == NGHTTP2_ERR_HTTP_MESSAGING) { stats_.rx_messaging_error_.inc(); - StreamImpl* stream = getStream(stream_id); if (stream != nullptr) { // See comment below in onStreamClose() for why we do this. stream->reset_due_to_messaging_error_ = true; @@ -551,6 +556,12 @@ int ConnectionImpl::onInvalidFrame(int32_t stream_id, int error_code) { return 0; } + if (stream != nullptr) { + // nghttp2 returns error, and ConnectionManager will call resetAllStreams(). Null out + // stream->decoder_ to avoid referring to it. + stream->decoder_ = nullptr; + } + // Cause dispatch to return with an error code. return NGHTTP2_ERR_CALLBACK_FAILURE; } @@ -606,6 +617,13 @@ int ConnectionImpl::onMetadataReceived(int32_t stream_id, const uint8_t* data, s } bool success = stream->getMetadataDecoder().receiveMetadata(data, len); + + if (!success) { + // nghttp2 returns error, and ConnectionManager will call resetAllStreams(). Null out + // stream->decoder_ to avoid referring to it. + stream->decoder_ = nullptr; + } + return success ? 0 : NGHTTP2_ERR_CALLBACK_FAILURE; } @@ -617,6 +635,11 @@ int ConnectionImpl::onMetadataFrameComplete(int32_t stream_id, bool end_metadata ASSERT(stream != nullptr); bool result = stream->getMetadataDecoder().onMetadataFrameComplete(end_metadata); + if (!result) { + // nghttp2 returns error, and ConnectionManager will call resetAllStreams(). Null out + // stream->decoder_ to avoid referring to it. + stream->decoder_ = nullptr; + } return result ? 0 : NGHTTP2_ERR_CALLBACK_FAILURE; } diff --git a/test/integration/http2_integration_test.cc b/test/integration/http2_integration_test.cc index 22929816f97f3..cc25f1698c39d 100644 --- a/test/integration/http2_integration_test.cc +++ b/test/integration/http2_integration_test.cc @@ -472,6 +472,32 @@ TEST_P(Http2MetadataIntegrationTest, ProxyLargeMetadataInRequest) { ASSERT_TRUE(response->complete()); } +TEST_P(Http2MetadataIntegrationTest, RequestMetadataReachSizeLimit) { + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + fake_upstreams_[0]->set_allow_unexpected_disconnects(true); + + auto encoder_decoder = codec_client_->startRequest(default_request_headers_); + request_encoder_ = &encoder_decoder.first; + auto response = std::move(encoder_decoder.second); + std::string value = std::string(10 * 1024, '1'); + Http::MetadataMap metadata_map = {{"key", value}}; + codec_client_->sendMetadata(*request_encoder_, metadata_map); + codec_client_->sendData(*request_encoder_, 1, false); + codec_client_->sendMetadata(*request_encoder_, metadata_map); + codec_client_->sendData(*request_encoder_, 1, false); + for (int i = 0; i < 200; i++) { + codec_client_->sendMetadata(*request_encoder_, metadata_map); + if (codec_client_->disconnected()) { + break; + } + } + + // Verifies client connection will be closed. + codec_client_->waitForDisconnect(); + ASSERT_FALSE(response->complete()); +} + static std::string request_metadata_filter = R"EOF( name: request-metadata-filter config: {} From d421e4548c7179ed53c7d4f3b577130f8499c3d2 Mon Sep 17 00:00:00 2001 From: Yang Song Date: Mon, 29 Jul 2019 10:56:00 +0800 Subject: [PATCH 2/3] change comments Signed-off-by: Yang Song --- source/common/http/http2/codec_impl.cc | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index a67d5ea4b19e1..6e5950fa85108 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -557,8 +557,8 @@ int ConnectionImpl::onInvalidFrame(int32_t stream_id, int error_code) { } if (stream != nullptr) { - // nghttp2 returns error, and ConnectionManager will call resetAllStreams(). Null out - // stream->decoder_ to avoid referring to it. + // nghttp2 returns error, and ConnectionManager will call resetAllStreams(). Should not refer to + // decoder_ from now on. Null out stream->decoder_. stream->decoder_ = nullptr; } @@ -619,8 +619,8 @@ int ConnectionImpl::onMetadataReceived(int32_t stream_id, const uint8_t* data, s bool success = stream->getMetadataDecoder().receiveMetadata(data, len); if (!success) { - // nghttp2 returns error, and ConnectionManager will call resetAllStreams(). Null out - // stream->decoder_ to avoid referring to it. + // nghttp2 returns error, and ConnectionManager will call resetAllStreams(). Should not refer to + // decoder_ from now on. Null out stream->decoder_. stream->decoder_ = nullptr; } @@ -636,8 +636,8 @@ int ConnectionImpl::onMetadataFrameComplete(int32_t stream_id, bool end_metadata bool result = stream->getMetadataDecoder().onMetadataFrameComplete(end_metadata); if (!result) { - // nghttp2 returns error, and ConnectionManager will call resetAllStreams(). Null out - // stream->decoder_ to avoid referring to it. + // nghttp2 returns error, and ConnectionManager will call resetAllStreams(). Should not refer to + // decoder_ from now on. Null out stream->decoder_. stream->decoder_ = nullptr; } return result ? 0 : NGHTTP2_ERR_CALLBACK_FAILURE; From b5de70ff895b58aea05a1e2540e433703273a305 Mon Sep 17 00:00:00 2001 From: Yang Song Date: Thu, 1 Aug 2019 14:16:08 +0800 Subject: [PATCH 3/3] Revert codec change. Signed-off-by: Yang Song --- source/common/http/http2/codec_impl.cc | 25 +------------------------ 1 file changed, 1 insertion(+), 24 deletions(-) diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index 6e5950fa85108..475aa1d314e28 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -185,7 +185,6 @@ void ConnectionImpl::StreamImpl::pendingRecvBufferLowWatermark() { void ConnectionImpl::StreamImpl::decodeHeaders() { maybeTransformUpgradeFromH2ToH1(); - ASSERT(decoder_ != nullptr); decoder_->decodeHeaders(std::move(headers_), remote_end_stream_); } @@ -335,7 +334,6 @@ MetadataDecoder& ConnectionImpl::StreamImpl::getMetadataDecoder() { } void ConnectionImpl::StreamImpl::onMetadataDecoded(MetadataMapPtr&& metadata_map_ptr) { - ASSERT(decoder_ != nullptr); decoder_->decodeMetadata(std::move(metadata_map_ptr)); } @@ -432,7 +430,6 @@ int ConnectionImpl::onFrameReceived(const nghttp2_frame* frame) { if (stream->headers_->Status()->value() == "100") { ASSERT(!stream->remote_end_stream_); - ASSERT(stream->decoder_ != nullptr); stream->decoder_->decode100ContinueHeaders(std::move(stream->headers_)); } else { stream->decodeHeaders(); @@ -460,7 +457,6 @@ int ConnectionImpl::onFrameReceived(const nghttp2_frame* frame) { stats_.too_many_header_frames_.inc(); throw CodecProtocolException("Unexpected 'trailers' with no end stream."); } else { - ASSERT(stream->decoder_ != nullptr); stream->decoder_->decodeTrailers(std::move(stream->headers_)); } } else { @@ -491,7 +487,6 @@ int ConnectionImpl::onFrameReceived(const nghttp2_frame* frame) { // It's possible that we are waiting to send a deferred reset, so only raise data if local // is not complete. if (!stream->deferred_reset_) { - ASSERT(stream->decoder_ != nullptr); stream->decoder_->decodeData(stream->pending_recv_data_, stream->remote_end_stream_); } @@ -544,11 +539,11 @@ int ConnectionImpl::onInvalidFrame(int32_t stream_id, int error_code) { ENVOY_CONN_LOG(debug, "invalid frame: {} on stream {}", connection_, nghttp2_strerror(error_code), stream_id); - StreamImpl* stream = getStream(stream_id); // The stream is about to be closed due to an invalid header or messaging. Don't kill the // entire connection if one stream has bad headers or messaging. if (error_code == NGHTTP2_ERR_HTTP_HEADER || error_code == NGHTTP2_ERR_HTTP_MESSAGING) { stats_.rx_messaging_error_.inc(); + StreamImpl* stream = getStream(stream_id); if (stream != nullptr) { // See comment below in onStreamClose() for why we do this. stream->reset_due_to_messaging_error_ = true; @@ -556,12 +551,6 @@ int ConnectionImpl::onInvalidFrame(int32_t stream_id, int error_code) { return 0; } - if (stream != nullptr) { - // nghttp2 returns error, and ConnectionManager will call resetAllStreams(). Should not refer to - // decoder_ from now on. Null out stream->decoder_. - stream->decoder_ = nullptr; - } - // Cause dispatch to return with an error code. return NGHTTP2_ERR_CALLBACK_FAILURE; } @@ -617,13 +606,6 @@ int ConnectionImpl::onMetadataReceived(int32_t stream_id, const uint8_t* data, s } bool success = stream->getMetadataDecoder().receiveMetadata(data, len); - - if (!success) { - // nghttp2 returns error, and ConnectionManager will call resetAllStreams(). Should not refer to - // decoder_ from now on. Null out stream->decoder_. - stream->decoder_ = nullptr; - } - return success ? 0 : NGHTTP2_ERR_CALLBACK_FAILURE; } @@ -635,11 +617,6 @@ int ConnectionImpl::onMetadataFrameComplete(int32_t stream_id, bool end_metadata ASSERT(stream != nullptr); bool result = stream->getMetadataDecoder().onMetadataFrameComplete(end_metadata); - if (!result) { - // nghttp2 returns error, and ConnectionManager will call resetAllStreams(). Should not refer to - // decoder_ from now on. Null out stream->decoder_. - stream->decoder_ = nullptr; - } return result ? 0 : NGHTTP2_ERR_CALLBACK_FAILURE; }