header_map: copy constructor for HeaderMapImpl.

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) {

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<std::pair<LowerCaseString, std::string>>& 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<std::pair<LowerCaseString, std::string>>& 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;

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<std::pair<LowerCaseString, std::string>>& values);
-  HeaderMapImpl(const HeaderMap& rhs);
+  explicit HeaderMapImpl(
+      const std::initializer_list<std::pair<LowerCaseString, std::string>>& 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()) {}
 

test/common/grpc/grpc_client_integration_test_harness.h

@@ -120,7 +120,7 @@ class HelloworldStream : public MockAsyncStreamCallbacks<helloworld::HelloReply>
       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() {

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<TestHeaderMapImpl>(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

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<const HeaderMap&>(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);
 }

test/test_common/utility.h

@@ -323,6 +323,12 @@ class TestHeaderMapImpl : public HeaderMapImpl {
   TestHeaderMapImpl(const std::initializer_list<std::pair<std::string, std::string>>& 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 {