header_map: copy constructor for HeaderMapImpl. #4129

htuch · 2018-08-13T18:43:56Z

Previously, since we had an implicitly deleted copy constructor, we picked up the implicitly created move constructor. This then performed a move operation on HeaderList, which is not safe to move because it contains iterator elements, which can point to end() and so are not safe when when moved, see the Notes in https://en.cppreference.com/w/cpp/container/list/list.

In this PR, we explicitly mark HeaderList as non-copyable (also suppresses the implicit move constructor), then provide an explicit copy constructor and assignment operator for TestHeaderImpl.

This appears to resolve the leak in #4118.

Risk level: Low
Testing: Unit test added.

Signed-off-by: Harvey Tuch htuch@google.com

Previously, we were picking up the default copy constructor, despite having effectively a copy constructor for the base class HeaderMap. This appears to resolve the leak in envoyproxy#4118. Risk level: Low Testing: Unit test added. Signed-off-by: Harvey Tuch <htuch@google.com>

mattklein123 · 2018-08-13T18:55:10Z

source/common/http/header_map_impl.cc

@@ -292,6 +292,9 @@ HeaderMapImpl::HeaderMapImpl(const HeaderMap& rhs) : HeaderMapImpl() {
      this);
 }

+HeaderMapImpl::HeaderMapImpl(const HeaderMapImpl& rhs)
+    : HeaderMapImpl(static_cast<const HeaderMap&>(rhs)) {}


Sorry I'm confused. How is this different from what the compiler did before?

It was synthesizing the default copy constructor, since we only supplied a constructor for the class we were deriving from AFAICT.

There's something more going on here though, witness the build failures. Looking into this now.

…estHeaderMapImpl. Signed-off-by: Harvey Tuch <htuch@google.com>

htuch · 2018-08-13T21:20:21Z

I think this is the proper fix. By marking HeaderList explicitly non-copyable, this forced the implicit copy assignment to stop happening. I then added an explicit copy assignment operator for TestHeaderMapImpl.

Still puzzled why the copy assignment ever worked (CC @jsedgwick).

mattklein123 · 2018-08-13T21:22:28Z

source/common/http/header_map_impl.cc

-      },
-      this);
-}
+HeaderMapImpl::HeaderMapImpl(const HeaderMapImpl& rhs)


Is this change still needed? It's unclear to me why the compiler won't just pick the other one.

I think so; the implicit copy constructor is deleted, the above constructor for HeaderMap isn't actually a copy constructor.

Signed-off-by: Harvey Tuch <htuch@google.com>

mattklein123

Thanks for digging into this!

htuch · 2018-08-13T22:10:29Z

OK, just chatted with @jsedgwick and have some ideas for how to try and either rationalize this or simplify, will update later tonight.

Signed-off-by: Harvey Tuch <htuch@google.com>

htuch · 2018-08-14T02:05:59Z

@mattklein123 I just figured this out, I think, with some great help from @jsedgwick. He pointed out that what was going on is not some magic implicit copy constructor generation, which should have been impossible because of the non-copyable HeaderEntryImpl, but instead the compiler was falling back on the implicit move constructor that was generated in lieu.

Moving HeaderList is unsafe; it contains iterator fields that can point to end() and so are not safe when when moved, see the Notes in https://en.cppreference.com/w/cpp/container/list/list. I think this is why I was seeing a leak in #4118.

The solution is basically what I had this arvo; by making HeaderList non-copyable explicitly, this actually disable the implicit move constructor. This then leaves us a clear compiler error message telling us to write a copy constructor/assignment operator in various places, which is what we provide in this PR.

Ultimately, maybe we would want to make HeaderList movable safely, I've left a TODO there. Given we largely are working with HeaderMapPtr when doing std::move(headers), I think we are safe to leave it non-movable, but thought I'd check with you.

Let nobody ever say fuzzing doesn't churn up fun bugs ;)

Signed-off-by: Harvey Tuch <htuch@google.com>

mattklein123

LGTM, one optional comment.

mattklein123 · 2018-08-14T02:57:12Z

source/common/http/header_map_impl.h

   */
-  class HeaderList {
+  class HeaderList : NonCopyable {


I think it's fine to block move, but why not just block the implicit move constructor inside NonCopyable? I realize it's called NonCopyable but I'm pretty sure in all cases where we use that we also don't want move?

I don't think this change is needed anymore, but fine to keep.

jmarantz

I'm really glad you are doing this. Not having copyable HeaderMap was a bit frustrating.

jmarantz · 2018-08-14T02:49:55Z

test/test_common/utility.cc

+    return *this;
+  }
+
+  removePrefix(LowerCaseString(""));


WDYT of factoring out a separate clear() method in the public API while you are in here?

I would rather not make any API public on HeaderMapImpl that we don't need to. This is a performance sensitive object and we should avoid having folks do slow/inefficient things with it that they don't need to.

If someone does need to clear(), I think it'd be better for it to be abstracted at the class interface.
Right now removePrefix(LowerCaseString("")) seems like it might be specific to the implementation?

But a compromise might be to define clear() but make it private or protected, with a note about exposing it in the future if there's a real need.

Arguably, nobody should be doing this, removePrefix is a horrible way to clear, it's only suitable for test purposes. I'm going to follow Matt's suggestion and move this to TestHeaderMapImpl only.

jmarantz · 2018-08-14T02:50:41Z

source/common/http/header_map_impl.cc

+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.


s/is/if/ ?

Do we keep a bit indicating whether a header-map contains owned strings or refs?

Will fix typo, performance optimization is orthogonal.

jmarantz · 2018-08-14T03:02:48Z

source/common/http/header_map_impl.cc

@@ -304,6 +293,22 @@ HeaderMapImpl::HeaderMapImpl(
  }
 }

+void HeaderMapImpl::copyFrom(const HeaderMap& header_map) {
+  header_map.iterate(


If you are defining a copy-ctor I think you should also define an assignment operator (and test it).

https://en.cppreference.com/w/cpp/language/rule_of_three

I see you are defining one in a test subclass, but it doesn't look like you've got one in HeaderMapImpl.

Yes, that's fair. I will mvoe copy assignment to HeaderMapImpl.

jmarantz · 2018-08-14T03:04:11Z

source/common/http/header_map_impl.cc

+        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());


Best practice is to use .data() rather than .c_str() here (x2), although I admit in practice I don't see how they can differ :)

There is no .data() on header.value().

Signed-off-by: Harvey Tuch <htuch@google.com>

htuch · 2018-08-14T15:34:18Z

@mattklein123 @jmarantz review feedback in last commit. I don't think we should mark HeaderMapImpl as NonCopyable, since we now do have copy assignment/construction, and these force the implicit move constructor/assignment operators to disappear, which makes things safe.

mattklein123 · 2018-08-14T16:42:03Z

source/common/http/header_map_impl.h

+  // 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);


Sorry to have more comments on this, but my concern about these changes is that we can accidentally start copying without knowing about it. Is it not possible to make HeaderMapImpl NonCopyable but then add the copy/assignment constructors in TestHeaderMapImpl?

That's a fair point. Having copy-ctor and assign-operator illegal is fine, but it would be nice to have copyFrom() as an explicit call.

Yes, we can do this then. I'm honestly surprised what a mess this all is. If anyone wants ammunition in the language wars that points to C++ being a time suck and unreasonably complicated for even experienced programmers to work with, this is a prime example.

Signed-off-by: Harvey Tuch <htuch@google.com>

jmarantz · 2018-08-14T17:42:49Z

source/common/http/header_map_impl.h

@@ -80,6 +81,9 @@ class HeaderMapImpl : public HeaderMap {
  size_t size() const override { return headers_.size(); }

 protected:
+  void copyFrom(const HeaderMap& rhs);


comment that these are protected because they are suspected to be slow, and should only be used in tests?

mattklein123 · 2018-08-14T17:43:08Z

source/common/http/header_map_impl.h

   */
-  class HeaderList {
+  class HeaderList : NonCopyable {


I don't think this change is needed anymore, but fine to keep.

Signed-off-by: Harvey Tuch <htuch@google.com>

htuch · 2018-08-14T17:47:09Z

I rather mark things everywhere as NonCopyable to make it as explicit as possible why/where we get into this situation. Long term, we should rethink the use of iterators to end(), but let's just land this fix for now.

* origin/master: fix flaky RBAC integration test. (envoyproxy#4147) header_map: copy constructor for HeaderMapImpl. (envoyproxy#4129) test: moving websocket tests to using HTTP codec. (envoyproxy#4143) upstream: init host hc value based on hc value from other priorities (envoyproxy#3959) Signed-off-by: Snow Pettersen <snowp@squareup.com>

mattklein123 reviewed Aug 13, 2018

View reviewed changes

Mark HeaderList non-copyable, explicit copy assignment operator for T…

fffdbbc

…estHeaderMapImpl. Signed-off-by: Harvey Tuch <htuch@google.com>

mattklein123 reviewed Aug 13, 2018

View reviewed changes

Kick CI

b0aa98a

Signed-off-by: Harvey Tuch <htuch@google.com>

htuch force-pushed the header-map-copy branch from e2197ac to b0aa98a Compare August 13, 2018 21:26

htuch added 3 commits August 13, 2018 17:26

Kick CI

e5ce010

Signed-off-by: Harvey Tuch <htuch@google.com>

Add explicit in various places.

76dfa6c

Signed-off-by: Harvey Tuch <htuch@google.com>

fix_format

080a830

Signed-off-by: Harvey Tuch <htuch@google.com>

mattklein123 previously approved these changes Aug 13, 2018

View reviewed changes

Finally it clicks, we need to prevent implicit move constructor.

f88da73

Signed-off-by: Harvey Tuch <htuch@google.com>

htuch dismissed mattklein123’s stale review via f88da73 August 14, 2018 02:05

Better explanation as per @jsedgwick.

86cc2fa

Signed-off-by: Harvey Tuch <htuch@google.com>

mattklein123 previously approved these changes Aug 14, 2018

View reviewed changes

jmarantz reviewed Aug 14, 2018

View reviewed changes

mattklein123 self-assigned this Aug 14, 2018

wip

d7d65ad

Signed-off-by: Harvey Tuch <htuch@google.com>

htuch dismissed mattklein123’s stale review via d7d65ad August 14, 2018 15:33

mattklein123 reviewed Aug 14, 2018

View reviewed changes

More review feedback.

3c525eb

Signed-off-by: Harvey Tuch <htuch@google.com>

jmarantz previously approved these changes Aug 14, 2018

View reviewed changes

mattklein123 previously approved these changes Aug 14, 2018

View reviewed changes

Comment.

aa008ff

Signed-off-by: Harvey Tuch <htuch@google.com>

htuch dismissed stale reviews from mattklein123 and jmarantz via aa008ff August 14, 2018 17:46

jmarantz approved these changes Aug 14, 2018

View reviewed changes

mattklein123 approved these changes Aug 14, 2018

View reviewed changes

htuch merged commit b150d61 into envoyproxy:master Aug 14, 2018

htuch deleted the header-map-copy branch August 14, 2018 19:56

zuercher mentioned this pull request Aug 14, 2018

rbac/fuzz: fix build #4150

Merged

header_map: copy constructor for HeaderMapImpl. #4129

header_map: copy constructor for HeaderMapImpl. #4129

Uh oh!

Conversation

htuch commented Aug 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch commented Aug 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch Aug 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

htuch commented Aug 13, 2018

Uh oh!

htuch commented Aug 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch commented Aug 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch commented Aug 14, 2018

Uh oh!

Uh oh!

htuch commented Aug 13, 2018 •

edited

Loading

htuch Aug 13, 2018 •

edited

Loading

htuch commented Aug 14, 2018 •

edited

Loading