Skip to content

Commit

Permalink
router: fix an invalid ASSERT when encoding metadata frames in the ro…
Browse files Browse the repository at this point in the history
…uter. (#13511)

Commit Message: Fix an invalid ASSERT when encoding metadata frames in the router.
Additional Description:
METADATA frames can't end stream, so there must be data, trailers, or a reset stream frame to end the stream. The ASSERT was meant to verify that there is data following METADATA frames to end the stream (for example, in case a client sends headers only request and metadata is added, FM adds empty data to end the stream). However, trailers could also end the stream, or the client could send body later/never. The ASSERT is invalid.

The PR removes the ASSERT.

Open to suggestions to have an ASSERT, but I couldn't think of one that was worth it / could be detected here. "if we had a header only request before, ASSERT there is empty data", or "if there is no possibility of stream reset occurring at this stage, ASSERT there is data or trailers."

Risk Level: Low. This only affects the ASSERT, has no impact on traffic.
Testing: Integration test added as a regression. Fuzz testcase added.
Fixes:
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=26238

Signed-off-by: Asra Ali <asraa@google.com>
  • Loading branch information
asraa authored Oct 13, 2020
1 parent 549acee commit eb9af96
Show file tree
Hide file tree
Showing 3 changed files with 80 additions and 2 deletions.
2 changes: 0 additions & 2 deletions source/common/router/upstream_request.cc
Original file line number Diff line number Diff line change
Expand Up @@ -440,8 +440,6 @@ void UpstreamRequest::encodeBodyAndTrailers() {
downstream_metadata_map_vector_);
upstream_->encodeMetadata(downstream_metadata_map_vector_);
downstream_metadata_map_vector_.clear();

ASSERT(buffered_request_body_);
}

if (buffered_request_body_) {
Expand Down
61 changes: 61 additions & 0 deletions test/integration/h2_corpus/buffered_body

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

19 changes: 19 additions & 0 deletions test/integration/http2_integration_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -568,6 +568,25 @@ TEST_P(Http2MetadataIntegrationTest, RequestMetadataReachSizeLimit) {
ASSERT_FALSE(response->complete());
}

TEST_P(Http2MetadataIntegrationTest, RequestMetadataThenTrailers) {
initialize();
codec_client_ = makeHttpConnection(lookupPort("http"));

auto encoder_decoder = codec_client_->startRequest(default_request_headers_);
request_encoder_ = &encoder_decoder.first;
auto response = std::move(encoder_decoder.second);
Http::MetadataMap metadata_map = {{"key", "value"}};
codec_client_->sendMetadata(*request_encoder_, metadata_map);
Http::TestRequestTrailerMapImpl request_trailers{{"trailer", "trailer"}};
codec_client_->sendTrailers(*request_encoder_, request_trailers);

waitForNextUpstreamRequest();

upstream_request_->encodeHeaders(default_response_headers_, true);
response->waitForEndStream();
ASSERT_TRUE(response->complete());
}

static std::string request_metadata_filter = R"EOF(
name: request-metadata-filter
typed_config:
Expand Down

0 comments on commit eb9af96

Please sign in to comment.