Skip to content

Commit

Permalink
[http] bugfix where error details are overridden (envoyproxy#12353)
Browse files Browse the repository at this point in the history 
nghttp2 will continue to do some frame processing on the same stream even if an error is detected that should trigger a stream close. This causes setDetails to be called twice, and override the error details in release mode (crash in debug mode)

Signed-off-by: Asra Ali <asraa@google.com>
  • Loading branch information
@asraa
asraa committed Jul 31, 2020
1 parent c3814e5 commit 153d225
Show file tree
Hide file tree
Showing 8 changed files with 128 additions and 55 deletions.
16 changes: 11 additions & 5 deletions source/common/http/http2/codec_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -226,11 +226,17 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
// This code assumes that details is a static string, so that we
// can avoid copying it.
void setDetails(absl::string_view details) {
// It is probably a mistake to call setDetails() twice, so
// assert that details_ is empty.
ASSERT(details_.empty());

details_ = details;
// TODO(asraa): In some cases nghttp2's error handling may cause processing of multiple
// invalid frames for a single stream. If a temporal stream error is returned from a callback,
// remaining frames in the buffer will still be partially processed. For example, remaining
// frames will still parse through nghttp2's push promise error handling and in
// onBeforeFrame(Send/Received) callbacks, which may return invalid frame errors and attempt
// to set details again. In these cases, we simply do not overwrite details. When internal
// error latching is implemented in the codec for exception removal, we should prevent calling
// setDetails in an error state.
if (details_.empty()) {
details_ = details;
}
}

void setWriteBufferWatermarks(uint32_t low_watermark, uint32_t high_watermark) {
Expand Down
16 changes: 11 additions & 5 deletions source/common/http/http2/codec_impl_legacy.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -226,11 +226,17 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable<Log
// This code assumes that details is a static string, so that we
// can avoid copying it.
void setDetails(absl::string_view details) {
// It is probably a mistake to call setDetails() twice, so
// assert that details_ is empty.
ASSERT(details_.empty());

details_ = details;
// TODO(asraa): In some cases nghttp2's error handling may cause processing of multiple
// invalid frames for a single stream. If a temporal stream error is returned from a callback,
// remaining frames in the buffer will still be partially processed. For example, remaining
// frames will still parse through nghttp2's push promise error handling and in
// onBeforeFrame(Send/Received) callbacks, which may return invalid frame errors and attempt
// to set details again. In these cases, we simply do not overwrite details. When internal
// error latching is implemented in the codec for exception removal, we should prevent calling
// setDetails in an error state.
if (details_.empty()) {
details_ = details;
}
}

void setWriteBufferWatermarks(uint32_t low_watermark, uint32_t high_watermark) {
Expand Down
1 change: 1 addition & 0 deletions test/common/http/http2/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ envoy_cc_test_library(
hdrs = ["http2_frame.h"],
deps = [
"//source/common/common:assert_lib",
"//source/common/common:hex_lib",
"//source/common/common:macros",
],
)
Expand Down
8 changes: 8 additions & 0 deletions test/common/http/http2/http2_frame.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

#include "envoy/common/platform.h"

#include "common/common/hex.h"

namespace {

// Make request stream ID in the network byte order
Expand Down Expand Up @@ -267,6 +269,12 @@ Http2Frame Http2Frame::makeGenericFrame(absl::string_view contents) {
return frame;
}

Http2Frame Http2Frame::makeGenericFrameFromHexDump(absl::string_view contents) {
Http2Frame frame;
frame.appendData(Hex::decode(std::string(contents)));
return frame;
}

} // namespace Http2
} // namespace Http
} // namespace Envoy
4 changes: 4 additions & 0 deletions test/common/http/http2/http2_frame.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ class Http2Frame {
* @return an Http2Frame that is comprised of the given contents.
*/
static Http2Frame makeGenericFrame(absl::string_view contents);
static Http2Frame makeGenericFrameFromHexDump(absl::string_view contents);

Type type() const { return static_cast<Type>(data_[3]); }
ResponseStatus responseStatus() const;
Expand Down Expand Up @@ -155,6 +156,9 @@ class Http2Frame {
// header.
void appendHpackInt(uint64_t value, unsigned char prefix_mask);
void appendData(absl::string_view data) { data_.insert(data_.end(), data.begin(), data.end()); }
void appendData(std::vector<uint8_t> data) {
data_.insert(data_.end(), data.begin(), data.end());
}

// Headers are directly encoded
void appendStaticHeader(StaticHeaderIndex index);
Expand Down
Binary file added test/common/http/http2/response_header_corpus/set_details_twice
View file
Binary file not shown.
112 changes: 76 additions & 36 deletions test/integration/http2_integration_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1494,36 +1494,32 @@ TEST_P(Http2RingHashIntegrationTest, CookieRoutingWithCookieWithTtlSet) {
EXPECT_EQ(served_by.size(), 1);
}

namespace {
const int64_t TransmitThreshold = 100 * 1024 * 1024;
} // namespace
void Http2FrameIntegrationTest::startHttp2Session() {
ASSERT_TRUE(tcp_client_->write(Http2Frame::Preamble, false, false));

void Http2FloodMitigationTest::setNetworkConnectionBufferSize() {
// nghttp2 library has its own internal mitigation for outbound control frames (see
// NGHTTP2_DEFAULT_MAX_OBQ_FLOOD_ITEM). The default nghttp2 mitigation threshold of 1K is modified
// to 10K in the ConnectionImpl::Http2Options::Http2Options. The mitigation is triggered when
// there are more than 10000 PING or SETTINGS frames with ACK flag in the nghttp2 internal
// outbound queue. It is possible to trigger this mitigation in nghttp2 before triggering Envoy's
// own flood mitigation. This can happen when a buffer large enough to contain over 10K PING or
// SETTINGS frames is dispatched to the nghttp2 library. To prevent this from happening the
// network connection receive buffer needs to be smaller than 90Kb (which is 10K SETTINGS frames).
// Set it to the arbitrarily chosen value of 32K. Note that this buffer has 16K lower bound.
config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) -> void {
RELEASE_ASSERT(bootstrap.mutable_static_resources()->listeners_size() >= 1, "");
auto* listener = bootstrap.mutable_static_resources()->mutable_listeners(0);
// Send empty initial SETTINGS frame.
auto settings = Http2Frame::makeEmptySettingsFrame();
ASSERT_TRUE(tcp_client_->write(std::string(settings), false, false));

listener->mutable_per_connection_buffer_limit_bytes()->set_value(32 * 1024);
});
// Read initial SETTINGS frame from the server.
readFrame();

// Send an SETTINGS ACK.
settings = Http2Frame::makeEmptySettingsFrame(Http2Frame::SettingsFlags::Ack);
ASSERT_TRUE(tcp_client_->write(std::string(settings), false, false));

// read pending SETTINGS and WINDOW_UPDATE frames
readFrame();
readFrame();
}

void Http2FloodMitigationTest::beginSession() {
void Http2FrameIntegrationTest::beginSession() {
setDownstreamProtocol(Http::CodecClient::Type::HTTP2);
setUpstreamProtocol(FakeHttpConnection::Type::HTTP2);
// set lower outbound frame limits to make tests run faster
config_helper_.setOutboundFramesLimits(1000, 100);
initialize();
// Set up a raw connection to easily send requests without reading responses. Also, set a small
// TCP receive buffer to speed up connection backup.
// Set up a raw connection to easily send requests without reading responses.
auto options = std::make_shared<Network::Socket::Options>();
options->emplace_back(std::make_shared<Network::SocketOptionImpl>(
envoy::config::core::v3::SocketOption::STATE_PREBIND,
Expand All @@ -1532,7 +1528,7 @@ void Http2FloodMitigationTest::beginSession() {
startHttp2Session();
}

Http2Frame Http2FloodMitigationTest::readFrame() {
Http2Frame Http2FrameIntegrationTest::readFrame() {
Http2Frame frame;
EXPECT_TRUE(tcp_client_->waitForData(frame.HeaderSize));
frame.setHeader(tcp_client_->data());
Expand All @@ -1546,28 +1542,72 @@ Http2Frame Http2FloodMitigationTest::readFrame() {
return frame;
}

void Http2FloodMitigationTest::sendFrame(const Http2Frame& frame) {
void Http2FrameIntegrationTest::sendFrame(const Http2Frame& frame) {
ASSERT_TRUE(tcp_client_->connected());
ASSERT_TRUE(tcp_client_->write(std::string(frame), false, false));
}

void Http2FloodMitigationTest::startHttp2Session() {
ASSERT_TRUE(tcp_client_->write(Http2Frame::Preamble, false, false));
// Regression test.
TEST_P(Http2FrameIntegrationTest, SetDetailsTwice) {
autonomous_upstream_ = true;
useAccessLog("%RESPONSE_FLAGS% %RESPONSE_CODE_DETAILS%");
beginSession();
fake_upstreams_[0]->set_allow_unexpected_disconnects(true);

// Send empty initial SETTINGS frame.
auto settings = Http2Frame::makeEmptySettingsFrame();
ASSERT_TRUE(tcp_client_->write(std::string(settings), false, false));
// Send two concatenated frames, the first with too many headers, and the second an invalid frame
// (push_promise)
std::string bad_frame =
"00006d0104000000014083a8749783ee3a3fbebebebebebebebebebebebebebebebebebebebebebebebebebebebe"
"bebebebebebebebebebebebebebebebebebebebebebebebebebebebebebebebebebebebebebebebebebebebebebe"
"bebebebebebebebebebebebebebebebebebebebebebebebebebe0001010500000000018800a065";
Http2Frame request = Http2Frame::makeGenericFrameFromHexDump(bad_frame);
sendFrame(request);
tcp_client_->close();

// Read initial SETTINGS frame from the server.
readFrame();
// Expect that the details for the first frame are kept.
EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("too_many_headers"));
}

// Send an SETTINGS ACK.
settings = Http2Frame::makeEmptySettingsFrame(Http2Frame::SettingsFlags::Ack);
ASSERT_TRUE(tcp_client_->write(std::string(settings), false, false));
INSTANTIATE_TEST_SUITE_P(IpVersions, Http2FrameIntegrationTest,
testing::ValuesIn(TestEnvironment::getIpVersionsForTest()),
TestUtility::ipTestParamsToString);

// read pending SETTINGS and WINDOW_UPDATE frames
readFrame();
readFrame();
namespace {
const int64_t TransmitThreshold = 100 * 1024 * 1024;
} // namespace

void Http2FloodMitigationTest::setNetworkConnectionBufferSize() {
// nghttp2 library has its own internal mitigation for outbound control frames (see
// NGHTTP2_DEFAULT_MAX_OBQ_FLOOD_ITEM). The default nghttp2 mitigation threshold of 1K is modified
// to 10K in the ConnectionImpl::Http2Options::Http2Options. The mitigation is triggered when
// there are more than 10000 PING or SETTINGS frames with ACK flag in the nghttp2 internal
// outbound queue. It is possible to trigger this mitigation in nghttp2 before triggering Envoy's
// own flood mitigation. This can happen when a buffer large enough to contain over 10K PING or
// SETTINGS frames is dispatched to the nghttp2 library. To prevent this from happening the
// network connection receive buffer needs to be smaller than 90Kb (which is 10K SETTINGS frames).
// Set it to the arbitrarily chosen value of 32K. Note that this buffer has 16K lower bound.
config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) -> void {
RELEASE_ASSERT(bootstrap.mutable_static_resources()->listeners_size() >= 1, "");
auto* listener = bootstrap.mutable_static_resources()->mutable_listeners(0);

listener->mutable_per_connection_buffer_limit_bytes()->set_value(32 * 1024);
});
}

void Http2FloodMitigationTest::beginSession() {
setDownstreamProtocol(Http::CodecClient::Type::HTTP2);
setUpstreamProtocol(FakeHttpConnection::Type::HTTP2);
// set lower outbound frame limits to make tests run faster
config_helper_.setOutboundFramesLimits(1000, 100);
initialize();
// Set up a raw connection to easily send requests without reading responses. Also, set a small
// TCP receive buffer to speed up connection backup.
auto options = std::make_shared<Network::Socket::Options>();
options->emplace_back(std::make_shared<Network::SocketOptionImpl>(
envoy::config::core::v3::SocketOption::STATE_PREBIND,
ENVOY_MAKE_SOCKET_OPTION_NAME(SOL_SOCKET, SO_RCVBUF), 1024));
tcp_client_ = makeTcpConnection(lookupPort("http"), options);
startHttp2Session();
}

// Verify that the server detects the flood of the given frame.
Expand Down
26 changes: 17 additions & 9 deletions test/integration/http2_integration_test.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,25 +67,33 @@ class Http2MetadataIntegrationTest : public Http2IntegrationTest {
void runHeaderOnlyTest(bool send_request_body, size_t body_size);
};

class Http2FloodMitigationTest : public testing::TestWithParam<Network::Address::IpVersion>,
public HttpIntegrationTest {
class Http2FrameIntegrationTest : public testing::TestWithParam<Network::Address::IpVersion>,
public HttpIntegrationTest {
public:
Http2FloodMitigationTest() : HttpIntegrationTest(Http::CodecClient::Type::HTTP2, GetParam()) {
Http2FrameIntegrationTest() : HttpIntegrationTest(Http::CodecClient::Type::HTTP2, GetParam()) {}

protected:
void startHttp2Session();
Http2Frame readFrame();
void sendFrame(const Http2Frame& frame);
virtual void beginSession();

IntegrationTcpClientPtr tcp_client_;
};

class Http2FloodMitigationTest : public Http2FrameIntegrationTest {
public:
Http2FloodMitigationTest() {
config_helper_.addConfigModifier(
[](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager&
hcm) { hcm.mutable_delayed_close_timeout()->set_seconds(1); });
}

protected:
void startHttp2Session();
void floodServer(const Http2Frame& frame, const std::string& flood_stat);
void floodServer(absl::string_view host, absl::string_view path,
Http2Frame::ResponseStatus expected_http_status, const std::string& flood_stat);
Http2Frame readFrame();
void sendFrame(const Http2Frame& frame);
void setNetworkConnectionBufferSize();
void beginSession();

IntegrationTcpClientPtr tcp_client_;
void beginSession() override;
};
} // namespace Envoy

0 comments on commit 153d225

Please sign in to comment.