header_map: copy constructor for HeaderMapImpl.

source/common/http/header_map_impl.cc

@@ -207,6 +207,17 @@ HeaderMapImpl::HeaderEntryImpl::HeaderEntryImpl(const LowerCaseString& key, Head
 HeaderMapImpl::HeaderEntryImpl::HeaderEntryImpl(HeaderString&& key, HeaderString&& value)
     : key_(std::move(key)), value_(std::move(value)) {}
 
+HeaderMapImpl& HeaderMapImpl::operator=(const HeaderMapImpl& rhs) {
+  if (&rhs == this) {
+    return *this;
+  }
+
+  removePrefix(LowerCaseString(""));
+  copyFrom(rhs);
+
+  return *this;
+}
+
 void HeaderMapImpl::HeaderEntryImpl::value(const char* value, uint32_t size) {
   value_.setCopy(value, size);
 }
@@ -276,21 +287,10 @@ 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(
-      [](const HeaderEntry& header, void* context) -> HeaderMap::Iterate {
-        // TODO(mattklein123) PERF: Avoid copying here is not necessary.
-        HeaderString key_string;
-        key_string.setCopy(header.key().c_str(), header.key().size());
-        HeaderString value_string;
-        value_string.setCopy(header.value().c_str(), header.value().size());
+HeaderMapImpl::HeaderMapImpl(const HeaderMap& rhs) : HeaderMapImpl() { copyFrom(rhs); }
 
-        static_cast<HeaderMapImpl*>(context)->addViaMove(std::move(key_string),
-                                                         std::move(value_string));
-        return HeaderMap::Iterate::Continue;
-      },
-      this);
-}
+HeaderMapImpl::HeaderMapImpl(const HeaderMapImpl& rhs)
+    : HeaderMapImpl(static_cast<const HeaderMap&>(rhs)) {}
 
 HeaderMapImpl::HeaderMapImpl(
     const std::initializer_list<std::pair<LowerCaseString, std::string>>& values)
@@ -304,6 +304,22 @@ HeaderMapImpl::HeaderMapImpl(
   }
 }
 
+void HeaderMapImpl::copyFrom(const HeaderMap& header_map) {
+  header_map.iterate(
+      [](const HeaderEntry& header, void* context) -> HeaderMap::Iterate {
+        // 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;
+        value_string.setCopy(header.value().c_str(), header.value().size());
+
+        static_cast<HeaderMapImpl*>(context)->addViaMove(std::move(key_string),
+                                                         std::move(value_string));
+        return HeaderMap::Iterate::Continue;
+      },
+      this);
+}
+
 bool HeaderMapImpl::operator==(const HeaderMapImpl& rhs) const {
   if (size() != rhs.size()) {
     return false;

source/common/http/header_map_impl.h

@@ -46,8 +46,14 @@ 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);
+  explicit HeaderMapImpl(
+      const std::initializer_list<std::pair<LowerCaseString, std::string>>& values);
   HeaderMapImpl(const HeaderMap& rhs);
+  // The above constructor for HeaderMap is not an actual copy constructor. This also prevent the
+  // implicit move constructor, moving HeaderMapImpl is unsafe (see HeaderList comments).
+  HeaderMapImpl(const HeaderMapImpl& rhs);
+  // Safe copy assignment; this is highly inefficient, don't use this except for tests.
+  HeaderMapImpl& operator=(const HeaderMapImpl& rhs);
 
   /**
    * Add a header via full move. This is the expected high performance paths for codecs populating
@@ -80,6 +86,8 @@ class HeaderMapImpl : public HeaderMap {
   size_t size() const override { return headers_.size(); }
 
 protected:
+  void copyFrom(const HeaderMap& rhs);
+
   struct HeaderEntryImpl : public HeaderEntry, NonCopyable {
     HeaderEntryImpl(const LowerCaseString& key);
     HeaderEntryImpl(const LowerCaseString& key, HeaderString&& value);
@@ -130,8 +138,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/http/header_map_impl_test.cc

@@ -853,5 +853,20 @@ TEST(HeaderMapImplTest, PseudoHeaderOrder) {
   }
 }
 
+// Validate that HeaderMapImpl 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, HeaderMapImplyCopy) {
+  HeaderMapImpl foo;
+  foo.addCopy(LowerCaseString("foo"), "bar");
+  auto headers = std::make_unique<HeaderMapImpl>(foo);
+  EXPECT_STREQ("bar", headers->get(LowerCaseString("foo"))->value().c_str());
+  HeaderMapImpl baz{{LowerCaseString("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