diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index de4e17fd5424d..3bb74d6b8a109 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -67,8 +67,7 @@ ConnectionManagerImpl::ConnectionManagerImpl(ConnectionManagerConfig& config, const HeaderMapImpl& ConnectionManagerImpl::continueHeader() { CONSTRUCT_ON_FIRST_USE(HeaderMapImpl, - Http::HeaderMapImpl{{Http::Headers::get().Status, - std::to_string(enumToInt(Code::Continue))}}); + {Http::Headers::get().Status, std::to_string(enumToInt(Code::Continue))}); } void ConnectionManagerImpl::initializeReadFilterCallbacks(Network::ReadFilterCallbacks& callbacks) { diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index bb260620f57b8..28728515ef12e 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -276,10 +276,22 @@ void HeaderMapImpl::appendToHeader(HeaderString& header, absl::string_view data) HeaderMapImpl::HeaderMapImpl() { memset(&inline_headers_, 0, sizeof(inline_headers_)); } -HeaderMapImpl::HeaderMapImpl(const HeaderMap& rhs) : HeaderMapImpl() { - rhs.iterate( +HeaderMapImpl::HeaderMapImpl( + const std::initializer_list>& values) + : HeaderMapImpl() { + for (auto& value : values) { + HeaderString key_string; + key_string.setCopy(value.first.get().c_str(), value.first.get().size()); + HeaderString value_string; + value_string.setCopy(value.second.c_str(), value.second.size()); + addViaMove(std::move(key_string), std::move(value_string)); + } +} + +void HeaderMapImpl::copyFrom(const HeaderMap& header_map) { + header_map.iterate( [](const HeaderEntry& header, void* context) -> HeaderMap::Iterate { - // TODO(mattklein123) PERF: Avoid copying here is not necessary. + // TODO(mattklein123) PERF: Avoid copying here if not necessary. HeaderString key_string; key_string.setCopy(header.key().c_str(), header.key().size()); HeaderString value_string; @@ -292,18 +304,6 @@ HeaderMapImpl::HeaderMapImpl(const HeaderMap& rhs) : HeaderMapImpl() { this); } -HeaderMapImpl::HeaderMapImpl( - const std::initializer_list>& values) - : HeaderMapImpl() { - for (auto& value : values) { - HeaderString key_string; - key_string.setCopy(value.first.get().c_str(), value.first.get().size()); - HeaderString value_string; - value_string.setCopy(value.second.c_str(), value.second.size()); - addViaMove(std::move(key_string), std::move(value_string)); - } -} - bool HeaderMapImpl::operator==(const HeaderMapImpl& rhs) const { if (size() != rhs.size()) { return false; diff --git a/source/common/http/header_map_impl.h b/source/common/http/header_map_impl.h index 8a4664090e28e..ae507866d2afc 100644 --- a/source/common/http/header_map_impl.h +++ b/source/common/http/header_map_impl.h @@ -35,7 +35,7 @@ public: * paths use O(1) direct access. In general, we try to copy as little as possible and allocate as * little as possible in any of the paths. */ -class HeaderMapImpl : public HeaderMap { +class HeaderMapImpl : public HeaderMap, NonCopyable { public: /** * Appends data to header. If header already has a value, the string ',' is added between the @@ -46,8 +46,9 @@ class HeaderMapImpl : public HeaderMap { static void appendToHeader(HeaderString& header, absl::string_view data); HeaderMapImpl(); - HeaderMapImpl(const std::initializer_list>& values); - HeaderMapImpl(const HeaderMap& rhs); + explicit HeaderMapImpl( + const std::initializer_list>& values); + explicit HeaderMapImpl(const HeaderMap& rhs) : HeaderMapImpl() { copyFrom(rhs); } /** * Add a header via full move. This is the expected high performance paths for codecs populating @@ -80,6 +81,10 @@ class HeaderMapImpl : public HeaderMap { size_t size() const override { return headers_.size(); } protected: + // For tests only, unoptimized, they aren't intended for regular HeaderMapImpl users. + void copyFrom(const HeaderMap& rhs); + void clear() { removePrefix(LowerCaseString("")); } + struct HeaderEntryImpl : public HeaderEntry, NonCopyable { HeaderEntryImpl(const LowerCaseString& key); HeaderEntryImpl(const LowerCaseString& key, HeaderString&& value); @@ -130,8 +135,15 @@ class HeaderMapImpl : public HeaderMap { /** * List of HeaderEntryImpl that keeps the pseudo headers (key starting with ':') in the front * of the list (as required by nghttp2) and otherwise maintains insertion order. + * + * Note: the internal iterators held in fields make this unsafe to copy and move, since the + * reference to end() is not preserved across a move (see Notes in + * https://en.cppreference.com/w/cpp/container/list/list). The NonCopyable will supress both copy + * and move constructors/assignment. + * TODO(htuch): Maybe we want this to movable one day; for now, our header map moves happen on + * HeaderMapPtr, so the performance impact should not be evident. */ - class HeaderList { + class HeaderList : NonCopyable { public: HeaderList() : pseudo_headers_end_(headers_.end()) {} diff --git a/test/common/grpc/grpc_client_integration_test_harness.h b/test/common/grpc/grpc_client_integration_test_harness.h index 9211c5411421d..2c3dfcf89026b 100644 --- a/test/common/grpc/grpc_client_integration_test_harness.h +++ b/test/common/grpc/grpc_client_integration_test_harness.h @@ -120,7 +120,7 @@ class HelloworldStream : public MockAsyncStreamCallbacks reply_headers->addReference(value.first, value.second); } expectInitialMetadata(metadata); - fake_stream_->encodeHeaders(*reply_headers, false); + fake_stream_->encodeHeaders(Http::HeaderMapImpl(*reply_headers), false); } void sendReply() { diff --git a/test/common/http/header_map_impl_test.cc b/test/common/http/header_map_impl_test.cc index 5b0ce1a7092ad..7d8bb9413ece3 100644 --- a/test/common/http/header_map_impl_test.cc +++ b/test/common/http/header_map_impl_test.cc @@ -853,5 +853,20 @@ TEST(HeaderMapImplTest, PseudoHeaderOrder) { } } +// Validate that TestHeaderMapImpl copy construction and assignment works. This is a +// regression for where we were missing a valid copy constructor and had the +// default (dangerous) move semantics takeover. +TEST(HeaderMapImplTest, TestHeaderMapImplyCopy) { + TestHeaderMapImpl foo; + foo.addCopy(LowerCaseString("foo"), "bar"); + auto headers = std::make_unique(foo); + EXPECT_STREQ("bar", headers->get(LowerCaseString("foo"))->value().c_str()); + TestHeaderMapImpl baz{{"foo", "baz"}}; + baz = *headers; + EXPECT_STREQ("bar", baz.get(LowerCaseString("foo"))->value().c_str()); + baz = baz; + EXPECT_STREQ("bar", baz.get(LowerCaseString("foo"))->value().c_str()); +} + } // namespace Http } // namespace Envoy diff --git a/test/test_common/utility.cc b/test/test_common/utility.cc index 49339c14a1efe..c2f738de31327 100644 --- a/test/test_common/utility.cc +++ b/test/test_common/utility.cc @@ -207,6 +207,20 @@ TestHeaderMapImpl::TestHeaderMapImpl( TestHeaderMapImpl::TestHeaderMapImpl(const HeaderMap& rhs) : HeaderMapImpl(rhs) {} +TestHeaderMapImpl::TestHeaderMapImpl(const TestHeaderMapImpl& rhs) + : TestHeaderMapImpl(static_cast(rhs)) {} + +TestHeaderMapImpl& TestHeaderMapImpl::operator=(const TestHeaderMapImpl& rhs) { + if (&rhs == this) { + return *this; + } + + clear(); + copyFrom(rhs); + + return *this; +} + void TestHeaderMapImpl::addCopy(const std::string& key, const std::string& value) { addCopy(LowerCaseString(key), value); } diff --git a/test/test_common/utility.h b/test/test_common/utility.h index 1904edfb39c12..c27d4be9319ff 100644 --- a/test/test_common/utility.h +++ b/test/test_common/utility.h @@ -323,6 +323,12 @@ class TestHeaderMapImpl : public HeaderMapImpl { TestHeaderMapImpl(const std::initializer_list>& values); TestHeaderMapImpl(const HeaderMap& rhs); + // The above constructor for TestHeaderMap is not an actual copy constructor. + TestHeaderMapImpl(const TestHeaderMapImpl& rhs); + TestHeaderMapImpl& operator=(const TestHeaderMapImpl& rhs); + + bool operator==(const TestHeaderMapImpl& rhs) const { return HeaderMapImpl::operator==(rhs); } + friend std::ostream& operator<<(std::ostream& os, const TestHeaderMapImpl& p) { p.iterate( [](const HeaderEntry& header, void* context) -> HeaderMap::Iterate {