From 125129d5c4f2b81dbef65bff10177aa7cd60a735 Mon Sep 17 00:00:00 2001 From: Kevin Baichoo Date: Mon, 25 Jan 2021 02:49:00 +0000 Subject: [PATCH 1/8] Dump H1 debug information if crashing during parsing. Signed-off-by: Kevin Baichoo --- docs/root/version_history/current.rst | 1 + source/common/common/dump_state_utils.h | 14 ++- source/common/http/http1/BUILD | 2 + source/common/http/http1/codec_impl.cc | 57 ++++++++- source/common/http/http1/codec_impl.h | 26 +++- test/common/http/http1/codec_impl_test.cc | 145 +++++++++++++++++++++- 6 files changed, 237 insertions(+), 8 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 2bf22571c14e..a7ea250ea54d 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -45,6 +45,7 @@ New Features * access log: added the :ref:`formatters ` extension point for custom formatters (command operators). * access log: support command operator: %REQUEST_HEADERS_BYTES%, %RESPONSE_HEADERS_BYTES% and %RESPONSE_TRAILERS_BYTES%. * dispatcher: supports a stack of `Envoy::ScopeTrackedObject` instead of a single tracked object. This will allow Envoy to dump more debug information on crash. +* http: added support for `Envoy::ScopeTrackedObject` for HTTP 1 dispatching. Crashes while inside the dispatching loop should dump debug information. * http: added support for :ref:`:ref:`preconnecting `. Preconnecting is off by default, but recommended for clusters serving latency-sensitive traffic, especially if using HTTP/1.1. * http: change frame flood and abuse checks to the upstream HTTP/2 codec to ON by default. It can be disabled by setting the `envoy.reloadable_features.upstream_http2_flood_checks` runtime key to false. * server: added :ref:`fips_mode ` statistic. diff --git a/source/common/common/dump_state_utils.h b/source/common/common/dump_state_utils.h index 9fbb8b43a175..12701eac47b6 100644 --- a/source/common/common/dump_state_utils.h +++ b/source/common/common/dump_state_utils.h @@ -9,7 +9,19 @@ namespace Envoy { // i.e. under the Envoy signal handler if encountering a crash due to OOM, where allocating more // memory would likely lead to the crash handler itself causing a subsequent OOM. -#define DUMP_MEMBER(member) ", " #member ": " << (member) +#define _DUMP_MEMBER(member) ", " #member ": " << (member) +#define _DUMP_MEMBER_VIA_VALUE(member, value) ", " #member ": " << (value) +#define _DUMP_MEMBER_SELECTOR(_1, _2, DUMP_MACRO, ...) DUMP_MACRO + +// This is a workaround for fact that MSVC expands __VA_ARGS__ after passing them into a macro, +// rather than before passing them into a macro. Without this, +// _DUMP_MEMBER_SELECTOR does not work correctly when compiled with MSVC. +#define EXPAND(X) X + +// If DUMP_MEMBER is called with one argument, then _DUMP_MEMBER is called. +// If DUMP_MEMBER is called with two arguments, then _DUMP_MEMBER_VIA_VALUE is called. +#define DUMP_MEMBER(...) \ + EXPAND(_DUMP_MEMBER_SELECTOR(__VA_ARGS__, _DUMP_MEMBER_VIA_VALUE, _DUMP_MEMBER)(__VA_ARGS__)) #define DUMP_OPTIONAL_MEMBER(member) \ ", " #member ": " << ((member).has_value() ? absl::StrCat((member).value()) : "null") diff --git a/source/common/http/http1/BUILD b/source/common/http/http1/BUILD index 8c864eaca715..699ab767a9d0 100644 --- a/source/common/http/http1/BUILD +++ b/source/common/http/http1/BUILD @@ -33,6 +33,7 @@ envoy_cc_library( ":codec_stats_lib", ":header_formatter_lib", "//include/envoy/buffer:buffer_interface", + "//include/envoy/common:scope_tracker_interface", "//include/envoy/http:codec_interface", "//include/envoy/http:header_map_interface", "//include/envoy/network:connection_interface", @@ -40,6 +41,7 @@ envoy_cc_library( "//source/common/buffer:watermark_buffer_lib", "//source/common/common:assert_lib", "//source/common/common:cleanup_lib", + "//source/common/common:dump_state_utils", "//source/common/common:statusor_lib", "//source/common/common:utility_lib", "//source/common/grpc:common_lib", diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index ea9ad820d43c..a3772d933ffa 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -10,7 +10,9 @@ #include "envoy/network/connection.h" #include "common/common/cleanup.h" +#include "common/common/dump_state_utils.h" #include "common/common/enum_to_int.h" +#include "common/common/scope_tracker.h" #include "common/common/statusor.h" #include "common/common/utility.h" #include "common/grpc/common.h" @@ -67,7 +69,6 @@ HeaderKeyFormatterPtr formatter(const Http::Http1Settings& settings) { return nullptr; } - } // namespace const std::string StreamEncoderImpl::CRLF = "\r\n"; @@ -568,6 +569,8 @@ Http::Status ClientConnectionImpl::dispatch(Buffer::Instance& data) { } Http::Status ConnectionImpl::innerDispatch(Buffer::Instance& data) { + // Add self to the Dispatcher's tracked object stack. + ScopeTrackerScopeState scope(this, connection_.dispatcher()); ENVOY_CONN_LOG(trace, "parsing {} bytes", connection_, data.length()); // Make sure that dispatching_ is set to false after dispatching, even when // http_parser exits early with an error code. @@ -861,6 +864,58 @@ void ConnectionImpl::onResetStreamBase(StreamResetReason reason) { onResetStream(reason); } +void ConnectionImpl::dumpState(std::ostream& os, int indent_level) const { + const char* spaces = spacesForLevel(indent_level); + // Dump all bool to provide context + os << spaces << "Http1::ConnectionImpl " << this << DUMP_MEMBER(dispatching_) + << DUMP_MEMBER(dispatching_slice_already_drained_) << DUMP_MEMBER(reset_stream_called_) + << DUMP_MEMBER(handling_upgrade_) << DUMP_MEMBER(deferred_end_stream_headers_) + << DUMP_MEMBER(strict_1xx_and_204_headers_) << DUMP_MEMBER(processing_trailers_); + + // Dump the first slice of the dispatching buffer and buffered_body if + // applicable. + auto dumpBuffer = [](auto* instance) { + auto slice = instance->frontSlice(); + return absl::string_view(static_cast(slice.mem_), slice.len_); + }; + + os << DUMP_NULLABLE_MEMBER(current_dispatching_buffer_, dumpBuffer(current_dispatching_buffer_)); + os << DUMP_MEMBER(buffered_body_, dumpBuffer(&buffered_body_)); + + // Dump header parsing state, and any progress on other headers. + os << DUMP_MEMBER(header_parsing_state_); + if (header_parsing_state_ == HeaderParsingState::Field) { + os << DUMP_MEMBER(current_header_field_, current_header_field_.getStringView()); + } else if (header_parsing_state_ == HeaderParsingState::Value) { + os << DUMP_MEMBER(current_header_field_, current_header_field_.getStringView()); + os << DUMP_MEMBER(current_header_value_, current_header_value_.getStringView()); + } + + // Dump Child + os << "\n"; + dumpAdditionalState(os, indent_level); +} + +void ServerConnectionImpl::dumpAdditionalState(std::ostream& os, int indent_level) const { + const char* spaces = spacesForLevel(indent_level); + // Dump header map, it may be null if it was moved to the request. + if (absl::holds_alternative(headers_or_trailers_)) { + DUMP_DETAILS(absl::get(headers_or_trailers_)); + } else { + DUMP_DETAILS(absl::get(headers_or_trailers_)); + } +} + +void ClientConnectionImpl::dumpAdditionalState(std::ostream& os, int indent_level) const { + const char* spaces = spacesForLevel(indent_level); + // Dump header map, it may be null if it was moved to the request. + if (absl::holds_alternative(headers_or_trailers_)) { + DUMP_DETAILS(absl::get(headers_or_trailers_)); + } else { + DUMP_DETAILS(absl::get(headers_or_trailers_)); + } +} + ServerConnectionImpl::ServerConnectionImpl( Network::Connection& connection, CodecStats& stats, ServerConnectionCallbacks& callbacks, const Http1Settings& settings, uint32_t max_request_headers_kb, diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index deb08526a3ad..f19a0ad5f88d 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -8,6 +8,7 @@ #include #include +#include "envoy/common/scope_tracker.h" #include "envoy/config/core/v3/protocol.pb.h" #include "envoy/http/codec.h" #include "envoy/network/connection.h" @@ -170,7 +171,9 @@ class RequestEncoderImpl : public StreamEncoderImpl, public RequestEncoder { * Handles the callbacks of http_parser with its own base routine and then * virtual dispatches to its subclasses. */ -class ConnectionImpl : public virtual Connection, protected Logger::Loggable { +class ConnectionImpl : public virtual Connection, + protected Logger::Loggable, + public ScopeTrackedObject { public: /** * @return Network::Connection& the backing network connection. @@ -231,6 +234,9 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable(RequestTrailerMapImpl::create()); } } + void dumpAdditionalState(std::ostream& os, int indent_level) const override; void sendProtocolErrorOld(absl::string_view details); @@ -617,6 +640,7 @@ class ClientConnectionImpl : public ClientConnection, public ConnectionImpl { headers_or_trailers_.emplace(ResponseTrailerMapImpl::create()); } } + void dumpAdditionalState(std::ostream& os, int indent_level) const override; absl::optional pending_response_; // TODO(mattklein123): The following bool tracks whether a pending response is complete before diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index e20a89f2d7b0..41a07131967a 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -1,3 +1,5 @@ +#include +#include #include #include @@ -6,6 +8,7 @@ #include "envoy/http/codec.h" #include "common/buffer/buffer_impl.h" +#include "common/common/utility.h" #include "common/http/exception.h" #include "common/http/header_map_impl.h" #include "common/http/http1/codec_impl.h" @@ -24,6 +27,7 @@ #include "gtest/gtest.h" using testing::_; +using testing::HasSubstr; using testing::InSequence; using testing::Invoke; using testing::NiceMock; @@ -1913,6 +1917,75 @@ TEST_F(Http1ServerConnectionImplTest, TestSmugglingAllowChunkedContentLength100) testServerAllowChunkedContentLength(100, true); } +TEST_F(Http1ServerConnectionImplTest, + ShouldDumpParsedAndPartialHeadersWithoutAllocatingMemoryIfProcessingHeaders) { + initialize(); + + MockRequestDecoder decoder; + EXPECT_CALL(callbacks_, newStream(_, _)) + .WillOnce(Invoke([&](ResponseEncoder&, bool) -> RequestDecoder& { return decoder; })); + + std::array buffer; + OutputBufferStream ostream{buffer.data(), buffer.size()}; + + Buffer::OwnedImpl headers("POST / HTTP/1.1\r\n" + "Host: host\r\n" + "Accept-Language: en\r\n" + "Connection: keep-alive\r\n" + "Unfinished-Header: Not-Finished-Value"); + + auto status = codec_->dispatch(headers); + EXPECT_TRUE(status.ok()); + + // Dumps the header map without allocating memory + Stats::TestUtil::MemoryTest memory_test; + dynamic_cast(codec_.get())->dumpState(ostream, 0); + EXPECT_EQ(memory_test.consumedBytes(), 0); + + // Check dump contents for completed headers and partial headers. + EXPECT_THAT( + ostream.contents(), + testing::HasSubstr("absl::get(headers_or_trailers_): \n ':authority', " + "'host'\n 'accept-language', 'en'\n 'connection', 'keep-alive'")); + EXPECT_THAT(ostream.contents(), + testing::HasSubstr("header_parsing_state_: Value, current_header_field_: " + "Unfinished-Header, current_header_value_: Not-Finished-Value")); +} + +TEST_F(Http1ServerConnectionImplTest, ShouldDumpBuffersWithoutAllocatingMemory) { + initialize(); + + NiceMock decoder; + EXPECT_CALL(callbacks_, newStream(_, _)) + .WillOnce(Invoke([&](ResponseEncoder&, bool) -> RequestDecoder& { return decoder; })); + + std::array buffer; + OutputBufferStream ostream{buffer.data(), buffer.size()}; + + // Dump the body + Buffer::OwnedImpl request("POST / HTTP/1.1\r\n" + "Content-Length: 11\r\n" + "\r\n" + "Hello Envoy"); + EXPECT_CALL(decoder, decodeData(_, _)) + .WillOnce(Invoke([&](Buffer::Instance&, bool) { + // dumpState here before buffers are drained. No memory should be allocated. + Stats::TestUtil::MemoryTest memory_test; + dynamic_cast(codec_.get())->dumpState(ostream, 0); + EXPECT_EQ(memory_test.consumedBytes(), 0); + })) + .WillOnce(Invoke([]() {})); + + auto status = codec_->dispatch(request); + EXPECT_TRUE(status.ok()); + + // Check dump contents + EXPECT_THAT( + ostream.contents(), + HasSubstr("current_dispatching_buffer_: POST / HTTP/1.1\r\nContent-Length: 11\r\n\r\nHello " + "Envoy, buffered_body_: Hello Envoy, header_parsing_state_: Done")); +} + class Http1ClientConnectionImplTest : public Http1CodecTestBase { public: void initialize() { @@ -2412,8 +2485,8 @@ TEST_F(Http1ClientConnectionImplTest, PrematureUpgradeResponse) { initialize(); // make sure upgradeAllowed doesn't cause crashes if run with no pending response. - Buffer::OwnedImpl response( - "HTTP/1.1 200 OK\r\nContent-Length: 5\r\nConnection: upgrade\r\nUpgrade: websocket\r\n\r\n"); + Buffer::OwnedImpl response("HTTP/1.1 200 OK\r\nContent-Length: 5\r\nConnection: " + "upgrade\r\nUpgrade: websocket\r\n\r\n"); auto status = codec_->dispatch(response); EXPECT_TRUE(isPrematureResponseError(status)); } @@ -2434,8 +2507,8 @@ TEST_F(Http1ClientConnectionImplTest, UpgradeResponse) { // Send upgrade headers EXPECT_CALL(response_decoder, decodeHeaders_(_, false)); - Buffer::OwnedImpl response( - "HTTP/1.1 200 OK\r\nContent-Length: 5\r\nConnection: upgrade\r\nUpgrade: websocket\r\n\r\n"); + Buffer::OwnedImpl response("HTTP/1.1 200 OK\r\nContent-Length: 5\r\nConnection: " + "upgrade\r\nUpgrade: websocket\r\n\r\n"); auto status = codec_->dispatch(response); // Send body payload @@ -2635,7 +2708,8 @@ TEST_F(Http1ClientConnectionImplTest, LowWatermarkDuringClose) { EXPECT_CALL(response_decoder, decodeHeaders_(_, true)) .WillOnce(Invoke([&](ResponseHeaderMapPtr&, bool) { - // Fake a call for going below the low watermark. Make sure no stream callbacks get called. + // Fake a call for going below the low watermark. Make sure no stream callbacks get + // called. EXPECT_CALL(stream_callbacks, onBelowWriteBufferLowWatermark()).Times(0); static_cast(codec_.get()) ->onUnderlyingConnectionBelowWriteBufferLowWatermark(); @@ -2960,5 +3034,66 @@ TEST_F(Http1ClientConnectionImplTest, TestResponseSplitAllowChunkedLength100) { testClientAllowChunkedContentLength(100, true); } +TEST_F(Http1ClientConnectionImplTest, + ShouldDumpParsedAndPartialHeadersWithoutAllocatingMemoryIfProcessingHeaders) { + initialize(); + + // Send request and dispatch response without headers completed. + MockResponseDecoder response_decoder; + Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); + TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; + EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok()); + + Buffer::OwnedImpl response("HTTP/1.1 200 OK\r\nserver: foo\r\nContent-Length: 8"); + auto status = codec_->dispatch(response); + EXPECT_EQ(0UL, response.length()); + EXPECT_TRUE(status.ok()); + + std::array buffer; + OutputBufferStream ostream{buffer.data(), buffer.size()}; + dynamic_cast(codec_.get())->dumpState(ostream, 0); + + // Check for header map and partial headers. + EXPECT_THAT(ostream.contents(), + testing::HasSubstr( + "absl::get(headers_or_trailers_): \n 'server', 'foo'\n")); + EXPECT_THAT(ostream.contents(), + testing::HasSubstr("header_parsing_state_: Value, current_header_field_: " + "Content-Length, current_header_value_: 8")); +} + +TEST_F(Http1ClientConnectionImplTest, ShouldDumpBuffersWithoutAllocatingMemory) { + initialize(); + + // Send request + NiceMock response_decoder; + Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); + TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; + EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok()); + + // Send response; dumpState while parsing response. + std::array buffer; + OutputBufferStream ostream{buffer.data(), buffer.size()}; + EXPECT_CALL(response_decoder, decodeData(_, _)) + .WillOnce(Invoke([&](Buffer::Instance&, bool) { + // dumpState here before buffers are drained. No memory should be allocated. + Stats::TestUtil::MemoryTest memory_test; + dynamic_cast(codec_.get())->dumpState(ostream, 0); + EXPECT_EQ(memory_test.consumedBytes(), 0); + })) + .WillOnce(Invoke([]() {})); + + Buffer::OwnedImpl response("HTTP/1.1 200 OK\r\nContent-Length: 11\r\n\r\nHello Envoy"); + auto status = codec_->dispatch(response); + EXPECT_EQ(0UL, response.length()); + EXPECT_TRUE(status.ok()); + + // Check for body data. + EXPECT_THAT(ostream.contents(), + testing::HasSubstr( + "current_dispatching_buffer_: HTTP/1.1 200 OK\r\nContent-Length: 11\r\n\r\nHello " + "Envoy, buffered_body_: Hello Envoy, header_parsing_state_: Done")); +} + } // namespace Http } // namespace Envoy From 5bbe8edd9b05f485dd4afb2557ee86c711a9e40d Mon Sep 17 00:00:00 2001 From: Kevin Baichoo Date: Tue, 26 Jan 2021 20:20:52 +0000 Subject: [PATCH 2/8] Minor changes. Signed-off-by: Kevin Baichoo --- source/common/common/dump_state_utils.h | 2 +- source/common/http/http1/codec_impl.cc | 41 ++++++++++++++++------- test/common/http/http1/codec_impl_test.cc | 19 ++++++----- 3 files changed, 40 insertions(+), 22 deletions(-) diff --git a/source/common/common/dump_state_utils.h b/source/common/common/dump_state_utils.h index 12701eac47b6..c44c1fec7753 100644 --- a/source/common/common/dump_state_utils.h +++ b/source/common/common/dump_state_utils.h @@ -27,7 +27,7 @@ namespace Envoy { ", " #member ": " << ((member).has_value() ? absl::StrCat((member).value()) : "null") #define DUMP_NULLABLE_MEMBER(member, value) \ - ", " #member ": " << ((member) != nullptr ? value : "null") + ", " #member ": " << ((member) != nullptr ? (value) : "null") // Macro assumes local member variables // os (ostream) diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index a3772d933ffa..7af9208bbb42 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -870,17 +870,8 @@ void ConnectionImpl::dumpState(std::ostream& os, int indent_level) const { os << spaces << "Http1::ConnectionImpl " << this << DUMP_MEMBER(dispatching_) << DUMP_MEMBER(dispatching_slice_already_drained_) << DUMP_MEMBER(reset_stream_called_) << DUMP_MEMBER(handling_upgrade_) << DUMP_MEMBER(deferred_end_stream_headers_) - << DUMP_MEMBER(strict_1xx_and_204_headers_) << DUMP_MEMBER(processing_trailers_); - - // Dump the first slice of the dispatching buffer and buffered_body if - // applicable. - auto dumpBuffer = [](auto* instance) { - auto slice = instance->frontSlice(); - return absl::string_view(static_cast(slice.mem_), slice.len_); - }; - - os << DUMP_NULLABLE_MEMBER(current_dispatching_buffer_, dumpBuffer(current_dispatching_buffer_)); - os << DUMP_MEMBER(buffered_body_, dumpBuffer(&buffered_body_)); + << DUMP_MEMBER(strict_1xx_and_204_headers_) << DUMP_MEMBER(processing_trailers_) + << DUMP_MEMBER(buffered_body_.length(), buffered_body_.length()); // Dump header parsing state, and any progress on other headers. os << DUMP_MEMBER(header_parsing_state_); @@ -894,11 +885,37 @@ void ConnectionImpl::dumpState(std::ostream& os, int indent_level) const { // Dump Child os << "\n"; dumpAdditionalState(os, indent_level); + + // Dump the first slice of the dispatching buffer if not drained. + if (current_dispatching_buffer_ == nullptr || dispatching_slice_already_drained_) { + // Buffer is either null or already drained (in the body). + // Use the macro for consistent formatting. + os << DUMP_NULLABLE_MEMBER(current_dispatching_buffer_, "null"); + return; + } else { + auto front_slice = [](Buffer::Instance* instance) { + auto slice = instance->frontSlice(); + return absl::string_view(static_cast(slice.mem_), slice.len_); + }(current_dispatching_buffer_); + + os << spaces << DUMP_MEMBER(front_slice.length(), front_slice.length()) << ", front_slice: \n"; + { + const char* spaces = spacesForLevel(indent_level + 1); + // Dump buffer data. TODO(kbaichoo): escape? + os << spaces << front_slice; + } + } } void ServerConnectionImpl::dumpAdditionalState(std::ostream& os, int indent_level) const { const char* spaces = spacesForLevel(indent_level); - // Dump header map, it may be null if it was moved to the request. + os << DUMP_MEMBER(active_request_.request_url_, + active_request_.has_value() && + !active_request_.value().request_url_.getStringView().empty() + ? active_request_.value().request_url_.getStringView() + : "null\n"); + // Dump header map, it may be null if it was moved to the request, and + // request_url. if (absl::holds_alternative(headers_or_trailers_)) { DUMP_DETAILS(absl::get(headers_or_trailers_)); } else { diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 41a07131967a..8db5a6fd9b91 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -1952,7 +1952,7 @@ TEST_F(Http1ServerConnectionImplTest, "Unfinished-Header, current_header_value_: Not-Finished-Value")); } -TEST_F(Http1ServerConnectionImplTest, ShouldDumpBuffersWithoutAllocatingMemory) { +TEST_F(Http1ServerConnectionImplTest, ShouldDumpDispatchBufferWithoutAllocatingMemory) { initialize(); NiceMock decoder; @@ -1980,10 +1980,10 @@ TEST_F(Http1ServerConnectionImplTest, ShouldDumpBuffersWithoutAllocatingMemory) EXPECT_TRUE(status.ok()); // Check dump contents - EXPECT_THAT( - ostream.contents(), - HasSubstr("current_dispatching_buffer_: POST / HTTP/1.1\r\nContent-Length: 11\r\n\r\nHello " - "Envoy, buffered_body_: Hello Envoy, header_parsing_state_: Done")); + EXPECT_THAT(ostream.contents(), HasSubstr("buffered_body_.length(): 11, header_parsing_state_: " + "Done\n, active_request_.request_url_: null")); + EXPECT_THAT(ostream.contents(), HasSubstr("front_slice.length(): 50, front_slice: \n POST / " + "HTTP/1.1\r\nContent-Length: 11\r\n\r\nHello Envoy")); } class Http1ClientConnectionImplTest : public Http1CodecTestBase { @@ -3062,7 +3062,7 @@ TEST_F(Http1ClientConnectionImplTest, "Content-Length, current_header_value_: 8")); } -TEST_F(Http1ClientConnectionImplTest, ShouldDumpBuffersWithoutAllocatingMemory) { +TEST_F(Http1ClientConnectionImplTest, ShouldDumpDispatchBufferWithoutAllocatingMemory) { initialize(); // Send request @@ -3089,10 +3089,11 @@ TEST_F(Http1ClientConnectionImplTest, ShouldDumpBuffersWithoutAllocatingMemory) EXPECT_TRUE(status.ok()); // Check for body data. + EXPECT_THAT(ostream.contents(), HasSubstr("buffered_body_.length(): 11, header_parsing_state_: " + "Done\n")); EXPECT_THAT(ostream.contents(), - testing::HasSubstr( - "current_dispatching_buffer_: HTTP/1.1 200 OK\r\nContent-Length: 11\r\n\r\nHello " - "Envoy, buffered_body_: Hello Envoy, header_parsing_state_: Done")); + testing::HasSubstr("front_slice.length(): 50, front_slice: \n HTTP/1.1 200 " + "OK\r\nContent-Length: 11\r\n\r\nHello Envoy")); } } // namespace Http From 090ce6c8cec6e244bbbace006f0ee2fb107abd00 Mon Sep 17 00:00:00 2001 From: Kevin Baichoo Date: Tue, 26 Jan 2021 22:49:13 +0000 Subject: [PATCH 3/8] Added escaping support for dumping the buffer. Signed-off-by: Kevin Baichoo --- source/common/common/utility.cc | 28 +++++++++++++++++ source/common/common/utility.h | 10 ++++++ source/common/http/http1/codec_impl.cc | 10 ++++-- test/common/common/utility_test.cc | 37 +++++++++++++++++++++++ test/common/http/http1/codec_impl_test.cc | 7 +++-- 5 files changed, 86 insertions(+), 6 deletions(-) diff --git a/source/common/common/utility.cc b/source/common/common/utility.cc index 2ed40659314c..9400039ea11c 100644 --- a/source/common/common/utility.cc +++ b/source/common/common/utility.cc @@ -457,6 +457,34 @@ std::string StringUtil::escape(const std::string& source) { return ret; } +void StringUtil::escapeToOstream(std::ostream& os, absl::string_view view) { + for (const char c : view) { + switch (c) { + case '\r': + os << "\\r"; + break; + case '\n': + os << "\\n"; + break; + case '\t': + os << "\\t"; + break; + case '"': + os << "\\\""; + break; + case '\'': + os << "\\\'"; + break; + case '\\': + os << "\\\\"; + break; + default: + os << c; + break; + } + } +} + const std::string& getDefaultDateFormat() { CONSTRUCT_ON_FIRST_USE(std::string, "%Y-%m-%dT%H:%M:%E3SZ"); } diff --git a/source/common/common/utility.h b/source/common/common/utility.h index c995fed2db38..2d89cb3273d0 100644 --- a/source/common/common/utility.h +++ b/source/common/common/utility.h @@ -382,6 +382,16 @@ class StringUtil { */ static std::string escape(const std::string& source); + /** + * Outputs the string to the provided ostream, while escaping \n, \r, \t, and " + * (double quote), ' (single quote), and \ (backslash) escaped. + * This may be particularly useful if you cannot allocate memory, and the + * ostream being written to is backed by an entity that won't allocate memory. + * @param os the ostream to output to. + * @param view a string view to output + */ + static void escapeToOstream(std::ostream& os, absl::string_view view); + /** * Provide a default value for a string if empty. * @param s string. diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 7af9208bbb42..ebd60702b298 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -883,7 +883,7 @@ void ConnectionImpl::dumpState(std::ostream& os, int indent_level) const { } // Dump Child - os << "\n"; + os << '\n'; dumpAdditionalState(os, indent_level); // Dump the first slice of the dispatching buffer if not drained. @@ -901,8 +901,12 @@ void ConnectionImpl::dumpState(std::ostream& os, int indent_level) const { os << spaces << DUMP_MEMBER(front_slice.length(), front_slice.length()) << ", front_slice: \n"; { const char* spaces = spacesForLevel(indent_level + 1); - // Dump buffer data. TODO(kbaichoo): escape? - os << spaces << front_slice; + // Dump buffer data escaping \r, \n, \t, ", ', and \. + // This is not the most performant implementation, but we're crashing and + // cannot allocate memory. + os << spaces; + StringUtil::escapeToOstream(os, front_slice); + os << '\n'; } } } diff --git a/test/common/common/utility_test.cc b/test/common/common/utility_test.cc index 48b82909bfce..7dd3422f99e5 100644 --- a/test/common/common/utility_test.cc +++ b/test/common/common/utility_test.cc @@ -262,6 +262,43 @@ TEST(StringUtil, escape) { EXPECT_EQ(StringUtil::escape("{\"linux\": \"penguin\"}"), "{\\\"linux\\\": \\\"penguin\\\"}"); } +TEST(StringUtil, escapeToOstream) { + { + std::array buffer; + OutputBufferStream ostream{buffer.data(), buffer.size()}; + StringUtil::escapeToOstream(ostream, "hello world"); + EXPECT_EQ(ostream.contents(), "hello world"); + } + + { + std::array buffer; + OutputBufferStream ostream{buffer.data(), buffer.size()}; + StringUtil::escapeToOstream(ostream, "hello\nworld\n"); + EXPECT_EQ(ostream.contents(), "hello\\nworld\\n"); + } + + { + std::array buffer; + OutputBufferStream ostream{buffer.data(), buffer.size()}; + StringUtil::escapeToOstream(ostream, "\t\nworld\r\n"); + EXPECT_EQ(ostream.contents(), "\\t\\nworld\\r\\n"); + } + + { + std::array buffer; + OutputBufferStream ostream{buffer.data(), buffer.size()}; + StringUtil::escapeToOstream(ostream, "{'linux': \"penguin\"}"); + EXPECT_EQ(ostream.contents(), "{\\'linux\\': \\\"penguin\\\"}"); + } + + { + std::array buffer; + OutputBufferStream ostream{buffer.data(), buffer.size()}; + StringUtil::escapeToOstream(ostream, R"(\\)"); + EXPECT_EQ(ostream.contents(), R"(\\\\)"); + } +} + TEST(StringUtil, toUpper) { EXPECT_EQ(StringUtil::toUpper(""), ""); EXPECT_EQ(StringUtil::toUpper("a"), "A"); diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 8db5a6fd9b91..739d60b18538 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -1982,8 +1982,9 @@ TEST_F(Http1ServerConnectionImplTest, ShouldDumpDispatchBufferWithoutAllocatingM // Check dump contents EXPECT_THAT(ostream.contents(), HasSubstr("buffered_body_.length(): 11, header_parsing_state_: " "Done\n, active_request_.request_url_: null")); - EXPECT_THAT(ostream.contents(), HasSubstr("front_slice.length(): 50, front_slice: \n POST / " - "HTTP/1.1\r\nContent-Length: 11\r\n\r\nHello Envoy")); + EXPECT_THAT(ostream.contents(), + HasSubstr("front_slice.length(): 50, front_slice: \n POST / " + "HTTP/1.1\\r\\nContent-Length: 11\\r\\n\\r\\nHello Envoy")); } class Http1ClientConnectionImplTest : public Http1CodecTestBase { @@ -3093,7 +3094,7 @@ TEST_F(Http1ClientConnectionImplTest, ShouldDumpDispatchBufferWithoutAllocatingM "Done\n")); EXPECT_THAT(ostream.contents(), testing::HasSubstr("front_slice.length(): 50, front_slice: \n HTTP/1.1 200 " - "OK\r\nContent-Length: 11\r\n\r\nHello Envoy")); + "OK\\r\\nContent-Length: 11\\r\\n\\r\\nHello Envoy")); } } // namespace Http From 0df63f0b8a13e40864075f1208c6a5afc01c8c98 Mon Sep 17 00:00:00 2001 From: Kevin Baichoo Date: Wed, 27 Jan 2021 14:56:17 +0000 Subject: [PATCH 4/8] Minor fixes in formatting, improve test to check only front slice is dumped. Signed-off-by: Kevin Baichoo --- source/common/http/http1/codec_impl.cc | 21 ++++++------- test/common/http/http1/codec_impl_test.cc | 36 +++++++++++++++-------- 2 files changed, 32 insertions(+), 25 deletions(-) diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index ebd60702b298..9b7ab90907b7 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -871,7 +871,7 @@ void ConnectionImpl::dumpState(std::ostream& os, int indent_level) const { << DUMP_MEMBER(dispatching_slice_already_drained_) << DUMP_MEMBER(reset_stream_called_) << DUMP_MEMBER(handling_upgrade_) << DUMP_MEMBER(deferred_end_stream_headers_) << DUMP_MEMBER(strict_1xx_and_204_headers_) << DUMP_MEMBER(processing_trailers_) - << DUMP_MEMBER(buffered_body_.length(), buffered_body_.length()); + << DUMP_MEMBER(buffered_body_.length()); // Dump header parsing state, and any progress on other headers. os << DUMP_MEMBER(header_parsing_state_); @@ -890,7 +890,7 @@ void ConnectionImpl::dumpState(std::ostream& os, int indent_level) const { if (current_dispatching_buffer_ == nullptr || dispatching_slice_already_drained_) { // Buffer is either null or already drained (in the body). // Use the macro for consistent formatting. - os << DUMP_NULLABLE_MEMBER(current_dispatching_buffer_, "null"); + os << DUMP_NULLABLE_MEMBER(current_dispatching_buffer_, "drained"); return; } else { auto front_slice = [](Buffer::Instance* instance) { @@ -898,16 +898,13 @@ void ConnectionImpl::dumpState(std::ostream& os, int indent_level) const { return absl::string_view(static_cast(slice.mem_), slice.len_); }(current_dispatching_buffer_); - os << spaces << DUMP_MEMBER(front_slice.length(), front_slice.length()) << ", front_slice: \n"; - { - const char* spaces = spacesForLevel(indent_level + 1); - // Dump buffer data escaping \r, \n, \t, ", ', and \. - // This is not the most performant implementation, but we're crashing and - // cannot allocate memory. - os << spaces; - StringUtil::escapeToOstream(os, front_slice); - os << '\n'; - } + // Dump buffer data escaping \r, \n, \t, ", ', and \. + // This is not the most performant implementation, but we're crashing and + // cannot allocate memory. + os << spaces << "current_dispatching_buffer_ front_slice length: " << front_slice.length() + << " contents: \""; + StringUtil::escapeToOstream(os, front_slice); + os << "\"\n"; } } diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 739d60b18538..73a809bba9aa 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -1963,10 +1963,14 @@ TEST_F(Http1ServerConnectionImplTest, ShouldDumpDispatchBufferWithoutAllocatingM OutputBufferStream ostream{buffer.data(), buffer.size()}; // Dump the body - Buffer::OwnedImpl request("POST / HTTP/1.1\r\n" - "Content-Length: 11\r\n" - "\r\n" - "Hello Envoy"); + // Set content length to enable us to dumpState before + // buffers are drained. Only the first slice should be dumped. + Buffer::OwnedImpl request; + request.appendSliceForTest("POST / HTTP/1.1\r\n" + "Content-Length: 5\r\n" + "\r\n" + "Hello"); + request.appendSliceForTest("GarbageDataShouldNotBeDumped"); EXPECT_CALL(decoder, decodeData(_, _)) .WillOnce(Invoke([&](Buffer::Instance&, bool) { // dumpState here before buffers are drained. No memory should be allocated. @@ -1980,11 +1984,11 @@ TEST_F(Http1ServerConnectionImplTest, ShouldDumpDispatchBufferWithoutAllocatingM EXPECT_TRUE(status.ok()); // Check dump contents - EXPECT_THAT(ostream.contents(), HasSubstr("buffered_body_.length(): 11, header_parsing_state_: " + EXPECT_THAT(ostream.contents(), HasSubstr("buffered_body_.length(): 5, header_parsing_state_: " "Done\n, active_request_.request_url_: null")); EXPECT_THAT(ostream.contents(), - HasSubstr("front_slice.length(): 50, front_slice: \n POST / " - "HTTP/1.1\\r\\nContent-Length: 11\\r\\n\\r\\nHello Envoy")); + HasSubstr("current_dispatching_buffer_ front_slice length: 43 contents: \"POST / " + "HTTP/1.1\\r\\nContent-Length: 5\\r\\n\\r\\nHello\"\n")); } class Http1ClientConnectionImplTest : public Http1CodecTestBase { @@ -3084,17 +3088,23 @@ TEST_F(Http1ClientConnectionImplTest, ShouldDumpDispatchBufferWithoutAllocatingM })) .WillOnce(Invoke([]() {})); - Buffer::OwnedImpl response("HTTP/1.1 200 OK\r\nContent-Length: 11\r\n\r\nHello Envoy"); + // Dump the body + // Set content length to enable us to dumpState before + // buffers are drained. Only the first slice should be dumped. + Buffer::OwnedImpl response; + response.appendSliceForTest("HTTP/1.1 200 OK\r\nContent-Length: 5\r\n\r\nHello"); + response.appendSliceForTest("GarbageDataShouldNotBeDumped"); auto status = codec_->dispatch(response); - EXPECT_EQ(0UL, response.length()); - EXPECT_TRUE(status.ok()); + // Client codec complains about extraneous data. + EXPECT_EQ(response.length(), 28UL); + EXPECT_FALSE(status.ok()); // Check for body data. - EXPECT_THAT(ostream.contents(), HasSubstr("buffered_body_.length(): 11, header_parsing_state_: " + EXPECT_THAT(ostream.contents(), HasSubstr("buffered_body_.length(): 5, header_parsing_state_: " "Done\n")); EXPECT_THAT(ostream.contents(), - testing::HasSubstr("front_slice.length(): 50, front_slice: \n HTTP/1.1 200 " - "OK\\r\\nContent-Length: 11\\r\\n\\r\\nHello Envoy")); + testing::HasSubstr("current_dispatching_buffer_ front_slice length: 43 contents: " + "\"HTTP/1.1 200 OK\\r\\nContent-Length: 5\\r\\n\\r\\nHello\"\n")); } } // namespace Http From 813be1b62786a1dffe8a79ab76abaa215be872ec Mon Sep 17 00:00:00 2001 From: Kevin Baichoo Date: Mon, 1 Feb 2021 14:56:34 +0000 Subject: [PATCH 5/8] Version history. Signed-off-by: Kevin Baichoo --- docs/root/version_history/current.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 27bd22141030..a2a1d3c07247 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -55,9 +55,9 @@ New Features * access log: added the :ref:`formatters ` extension point for custom formatters (command operators). * access log: support command operator: %REQUEST_HEADERS_BYTES%, %RESPONSE_HEADERS_BYTES% and %RESPONSE_TRAILERS_BYTES%. * dispatcher: supports a stack of `Envoy::ScopeTrackedObject` instead of a single tracked object. This will allow Envoy to dump more debug information on crash. -* http: added support for `Envoy::ScopeTrackedObject` for HTTP 1 dispatching. Crashes while inside the dispatching loop should dump debug information. * grpc_json_transcoder: added option :ref:`strict_http_request_validation ` to reject invalid requests early. * grpc_json_transcoder: filter can now be configured on per-route/per-vhost level as well. Leaving empty list of services in the filter configuration disables transcoding on the specific route. +* http: added support for `Envoy::ScopeTrackedObject` for HTTP 1 dispatching. Crashes while inside the dispatching loop should dump debug information. * http: added support for :ref:`:ref:`preconnecting `. Preconnecting is off by default, but recommended for clusters serving latency-sensitive traffic, especially if using HTTP/1.1. * http: change frame flood and abuse checks to the upstream HTTP/2 codec to ON by default. It can be disabled by setting the `envoy.reloadable_features.upstream_http2_flood_checks` runtime key to false. * overload: add support for scaling :ref:`transport connection timeouts`. This can be used to reduce the TLS handshake timeout in response to overload. From 81c9ee9959023984c336f7a5725725f5d362e82b Mon Sep 17 00:00:00 2001 From: Kevin Baichoo Date: Mon, 1 Feb 2021 18:41:54 +0000 Subject: [PATCH 6/8] Version history merge. Signed-off-by: Kevin Baichoo --- docs/root/version_history/current.rst | 1 - 1 file changed, 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index b8011b97832e..36c86988869f 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -58,7 +58,6 @@ New Features * grpc_json_transcoder: added option :ref:`strict_http_request_validation ` to reject invalid requests early. * grpc_json_transcoder: filter can now be configured on per-route/per-vhost level as well. Leaving empty list of services in the filter configuration disables transcoding on the specific route. * http: added support for `Envoy::ScopeTrackedObject` for HTTP 1 dispatching. Crashes while inside the dispatching loop should dump debug information. -* http: added support for :ref:`:ref:`preconnecting `. Preconnecting is off by default, but recommended for clusters serving latency-sensitive traffic, especially if using HTTP/1.1. * http: added support for :ref:`preconnecting `. Preconnecting is off by default, but recommended for clusters serving latency-sensitive traffic, especially if using HTTP/1.1. * http: change frame flood and abuse checks to the upstream HTTP/2 codec to ON by default. It can be disabled by setting the `envoy.reloadable_features.upstream_http2_flood_checks` runtime key to false. * overload: add support for scaling :ref:`transport connection timeouts`. This can be used to reduce the TLS handshake timeout in response to overload. From 020ed25a961e417887b17bae8322c1b2054aa797 Mon Sep 17 00:00:00 2001 From: Kevin Baichoo Date: Mon, 1 Feb 2021 21:53:23 +0000 Subject: [PATCH 7/8] Minor cleanup: naming, comments, etc. Signed-off-by: Kevin Baichoo --- docs/root/version_history/current.rst | 2 +- source/common/common/dump_state_utils.h | 15 ++---------- source/common/common/utility.cc | 1 + source/common/http/http1/codec_impl.cc | 29 ++++++++++------------- test/common/http/http1/codec_impl_test.cc | 16 ++++++------- 5 files changed, 24 insertions(+), 39 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 36c86988869f..fae0ec2551fd 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -57,7 +57,7 @@ New Features * dispatcher: supports a stack of `Envoy::ScopeTrackedObject` instead of a single tracked object. This will allow Envoy to dump more debug information on crash. * grpc_json_transcoder: added option :ref:`strict_http_request_validation ` to reject invalid requests early. * grpc_json_transcoder: filter can now be configured on per-route/per-vhost level as well. Leaving empty list of services in the filter configuration disables transcoding on the specific route. -* http: added support for `Envoy::ScopeTrackedObject` for HTTP 1 dispatching. Crashes while inside the dispatching loop should dump debug information. +* http: added support for `Envoy::ScopeTrackedObject` for HTTP/1 dispatching. Crashes while inside the dispatching loop should dump debug information. * http: added support for :ref:`preconnecting `. Preconnecting is off by default, but recommended for clusters serving latency-sensitive traffic, especially if using HTTP/1.1. * http: change frame flood and abuse checks to the upstream HTTP/2 codec to ON by default. It can be disabled by setting the `envoy.reloadable_features.upstream_http2_flood_checks` runtime key to false. * overload: add support for scaling :ref:`transport connection timeouts`. This can be used to reduce the TLS handshake timeout in response to overload. diff --git a/source/common/common/dump_state_utils.h b/source/common/common/dump_state_utils.h index c44c1fec7753..f2821195fd9d 100644 --- a/source/common/common/dump_state_utils.h +++ b/source/common/common/dump_state_utils.h @@ -9,19 +9,8 @@ namespace Envoy { // i.e. under the Envoy signal handler if encountering a crash due to OOM, where allocating more // memory would likely lead to the crash handler itself causing a subsequent OOM. -#define _DUMP_MEMBER(member) ", " #member ": " << (member) -#define _DUMP_MEMBER_VIA_VALUE(member, value) ", " #member ": " << (value) -#define _DUMP_MEMBER_SELECTOR(_1, _2, DUMP_MACRO, ...) DUMP_MACRO - -// This is a workaround for fact that MSVC expands __VA_ARGS__ after passing them into a macro, -// rather than before passing them into a macro. Without this, -// _DUMP_MEMBER_SELECTOR does not work correctly when compiled with MSVC. -#define EXPAND(X) X - -// If DUMP_MEMBER is called with one argument, then _DUMP_MEMBER is called. -// If DUMP_MEMBER is called with two arguments, then _DUMP_MEMBER_VIA_VALUE is called. -#define DUMP_MEMBER(...) \ - EXPAND(_DUMP_MEMBER_SELECTOR(__VA_ARGS__, _DUMP_MEMBER_VIA_VALUE, _DUMP_MEMBER)(__VA_ARGS__)) +#define DUMP_MEMBER(member) ", " #member ": " << (member) +#define DUMP_MEMBER_AS(member, value) ", " #member ": " << (value) #define DUMP_OPTIONAL_MEMBER(member) \ ", " #member ": " << ((member).has_value() ? absl::StrCat((member).value()) : "null") diff --git a/source/common/common/utility.cc b/source/common/common/utility.cc index 9400039ea11c..815bd28db820 100644 --- a/source/common/common/utility.cc +++ b/source/common/common/utility.cc @@ -457,6 +457,7 @@ std::string StringUtil::escape(const std::string& source) { return ret; } +// TODO(kbaichoo): If needed, add support for escaping < and chars >= 127. void StringUtil::escapeToOstream(std::ostream& os, absl::string_view view) { for (const char c : view) { switch (c) { diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 097a3d6aa26f..56da4e908ad5 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -868,37 +868,32 @@ void ConnectionImpl::onResetStreamBase(StreamResetReason reason) { void ConnectionImpl::dumpState(std::ostream& os, int indent_level) const { const char* spaces = spacesForLevel(indent_level); - // Dump all bool to provide context os << spaces << "Http1::ConnectionImpl " << this << DUMP_MEMBER(dispatching_) << DUMP_MEMBER(dispatching_slice_already_drained_) << DUMP_MEMBER(reset_stream_called_) << DUMP_MEMBER(handling_upgrade_) << DUMP_MEMBER(deferred_end_stream_headers_) << DUMP_MEMBER(strict_1xx_and_204_headers_) << DUMP_MEMBER(processing_trailers_) << DUMP_MEMBER(buffered_body_.length()); - // Dump header parsing state, and any progress on other headers. + // Dump header parsing state, and any progress on headers. os << DUMP_MEMBER(header_parsing_state_); - if (header_parsing_state_ == HeaderParsingState::Field) { - os << DUMP_MEMBER(current_header_field_, current_header_field_.getStringView()); - } else if (header_parsing_state_ == HeaderParsingState::Value) { - os << DUMP_MEMBER(current_header_field_, current_header_field_.getStringView()); - os << DUMP_MEMBER(current_header_value_, current_header_value_.getStringView()); - } + os << DUMP_MEMBER_AS(current_header_field_, current_header_field_.getStringView()); + os << DUMP_MEMBER_AS(current_header_value_, current_header_value_.getStringView()); // Dump Child os << '\n'; dumpAdditionalState(os, indent_level); - // Dump the first slice of the dispatching buffer if not drained. + // Dump the first slice of the dispatching buffer if not drained escaping + // certain characters. We do this last as the slice could be rather large. if (current_dispatching_buffer_ == nullptr || dispatching_slice_already_drained_) { // Buffer is either null or already drained (in the body). // Use the macro for consistent formatting. os << DUMP_NULLABLE_MEMBER(current_dispatching_buffer_, "drained"); return; } else { - auto front_slice = [](Buffer::Instance* instance) { - auto slice = instance->frontSlice(); + absl::string_view front_slice = [](Buffer::RawSlice slice) { return absl::string_view(static_cast(slice.mem_), slice.len_); - }(current_dispatching_buffer_); + }(current_dispatching_buffer_->frontSlice()); // Dump buffer data escaping \r, \n, \t, ", ', and \. // This is not the most performant implementation, but we're crashing and @@ -912,11 +907,11 @@ void ConnectionImpl::dumpState(std::ostream& os, int indent_level) const { void ServerConnectionImpl::dumpAdditionalState(std::ostream& os, int indent_level) const { const char* spaces = spacesForLevel(indent_level); - os << DUMP_MEMBER(active_request_.request_url_, - active_request_.has_value() && - !active_request_.value().request_url_.getStringView().empty() - ? active_request_.value().request_url_.getStringView() - : "null\n"); + os << DUMP_MEMBER_AS(active_request_.request_url_, + active_request_.has_value() && + !active_request_.value().request_url_.getStringView().empty() + ? active_request_.value().request_url_.getStringView() + : "null\n"); // Dump header map, it may be null if it was moved to the request, and // request_url. if (absl::holds_alternative(headers_or_trailers_)) { diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 73a809bba9aa..1466b4c68c97 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -1985,7 +1985,8 @@ TEST_F(Http1ServerConnectionImplTest, ShouldDumpDispatchBufferWithoutAllocatingM // Check dump contents EXPECT_THAT(ostream.contents(), HasSubstr("buffered_body_.length(): 5, header_parsing_state_: " - "Done\n, active_request_.request_url_: null")); + "Done, current_header_field_: , current_header_value_: " + "\n, active_request_.request_url_: null")); EXPECT_THAT(ostream.contents(), HasSubstr("current_dispatching_buffer_ front_slice length: 43 contents: \"POST / " "HTTP/1.1\\r\\nContent-Length: 5\\r\\n\\r\\nHello\"\n")); @@ -2490,8 +2491,8 @@ TEST_F(Http1ClientConnectionImplTest, PrematureUpgradeResponse) { initialize(); // make sure upgradeAllowed doesn't cause crashes if run with no pending response. - Buffer::OwnedImpl response("HTTP/1.1 200 OK\r\nContent-Length: 5\r\nConnection: " - "upgrade\r\nUpgrade: websocket\r\n\r\n"); + Buffer::OwnedImpl response( + "HTTP/1.1 200 OK\r\nContent-Length: 5\r\nConnection: upgrade\r\nUpgrade: websocket\r\n\r\n"); auto status = codec_->dispatch(response); EXPECT_TRUE(isPrematureResponseError(status)); } @@ -2512,8 +2513,8 @@ TEST_F(Http1ClientConnectionImplTest, UpgradeResponse) { // Send upgrade headers EXPECT_CALL(response_decoder, decodeHeaders_(_, false)); - Buffer::OwnedImpl response("HTTP/1.1 200 OK\r\nContent-Length: 5\r\nConnection: " - "upgrade\r\nUpgrade: websocket\r\n\r\n"); + Buffer::OwnedImpl response( + "HTTP/1.1 200 OK\r\nContent-Length: 5\r\nConnection: upgrade\r\nUpgrade: websocket\r\n\r\n"); auto status = codec_->dispatch(response); // Send body payload @@ -2713,8 +2714,7 @@ TEST_F(Http1ClientConnectionImplTest, LowWatermarkDuringClose) { EXPECT_CALL(response_decoder, decodeHeaders_(_, true)) .WillOnce(Invoke([&](ResponseHeaderMapPtr&, bool) { - // Fake a call for going below the low watermark. Make sure no stream callbacks get - // called. + // Fake a call for going below the low watermark. Make sure no stream callbacks get called. EXPECT_CALL(stream_callbacks, onBelowWriteBufferLowWatermark()).Times(0); static_cast(codec_.get()) ->onUnderlyingConnectionBelowWriteBufferLowWatermark(); @@ -3101,7 +3101,7 @@ TEST_F(Http1ClientConnectionImplTest, ShouldDumpDispatchBufferWithoutAllocatingM // Check for body data. EXPECT_THAT(ostream.contents(), HasSubstr("buffered_body_.length(): 5, header_parsing_state_: " - "Done\n")); + "Done")); EXPECT_THAT(ostream.contents(), testing::HasSubstr("current_dispatching_buffer_ front_slice length: 43 contents: " "\"HTTP/1.1 200 OK\\r\\nContent-Length: 5\\r\\n\\r\\nHello\"\n")); From 5d3838b903a4661540fafd12551aabbaa41a73f2 Mon Sep 17 00:00:00 2001 From: Kevin Baichoo Date: Tue, 2 Feb 2021 02:32:07 +0000 Subject: [PATCH 8/8] Minor nits. Signed-off-by: Kevin Baichoo --- source/common/common/utility.cc | 2 +- source/common/http/http1/codec_impl.cc | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/source/common/common/utility.cc b/source/common/common/utility.cc index 815bd28db820..a325bcc32110 100644 --- a/source/common/common/utility.cc +++ b/source/common/common/utility.cc @@ -457,7 +457,7 @@ std::string StringUtil::escape(const std::string& source) { return ret; } -// TODO(kbaichoo): If needed, add support for escaping < and chars >= 127. +// TODO(kbaichoo): If needed, add support for escaping chars < 32 and >= 127. void StringUtil::escapeToOstream(std::ostream& os, absl::string_view view) { for (const char c : view) { switch (c) { diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 56da4e908ad5..5afb8b5699bb 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -911,7 +911,9 @@ void ServerConnectionImpl::dumpAdditionalState(std::ostream& os, int indent_leve active_request_.has_value() && !active_request_.value().request_url_.getStringView().empty() ? active_request_.value().request_url_.getStringView() - : "null\n"); + : "null"); + os << '\n'; + // Dump header map, it may be null if it was moved to the request, and // request_url. if (absl::holds_alternative(headers_or_trailers_)) {