[DRAFT] Refcount stats across scopes by ambuc · Pull Request #1 · ambuc/envoy

ambuc · 2018-04-17T16:56:42Z

Signed-off-by: James Buckland <jbuckland@google.com>

…cyTests Signed-off-by: James Buckland <jbuckland@google.com>

HeapRawStatDataAllocator still does heap allocation without namespace resolution Signed-off-by: James Buckland <jbuckland@google.com>

Signed-off-by: James Buckland <jbuckland@google.com>

mrice32 · 2018-04-17T17:00:29Z

source/common/stats/BUILD

        "//source/common/common:assert_lib",
        "//source/common/common:hash_lib",
        "//source/common/common:perf_annotation_lib",
+        "//source/common/common:shared_memory_hash_set_lib",


test comment

mrice32

Looks awesome - here are some comments to start!

mrice32 · 2018-04-17T17:02:59Z

source/common/stats/stats_impl.cc

 }

+RawStatData* BlockRawStatDataAllocator::alloc(const std::string& name) {
+  absl::string_view key = name;


Why not just use name directly throughout?

name comes in as a std::string& and we want to be able to call things like .remove_suffix() on it, which are absl::string_view features.

ahhh, makes sense. I didn't realize you were substringing it.

mrice32 · 2018-04-17T17:08:36Z

source/common/stats/stats_impl.h

+
+  RawStatData* alloc(const std::string& name);
+  void free(RawStatData& data);
+  std::unique_ptr<RawStatDataSet> stats_set_;


Is there a reason we can't make this variable private?

I think we can! I thought it needed to be used in versionHelper, but I was able to rework that to need only the std::string version number instead of the whole RawStatDataSet itself.

mrice32 · 2018-04-17T17:09:45Z

source/server/hot_restart_impl.cc

  // Stats::RawStatData must be naturally aligned for atomics to work properly.
-  RELEASE_ASSERT((reinterpret_cast<uintptr_t>(shmem->stats_set_data_) % alignof(RawStatDataSet)) ==
-                 0);
+  RELEASE_ASSERT(


What changed here?

RawStatDataSet -> Stats::RawStatDataSet, which pushed it over some character limit and reflowed the formatting.

Ah, feel free to disregard.

mrice32 · 2018-04-17T17:19:32Z

source/server/hot_restart_impl.cc

-      shmem_(SharedMemory::initialize(RawStatDataSet::numBytes(stats_set_options_), options)),
+      shmem_(
+          SharedMemory::initialize(Stats::RawStatDataSet::numBytes(stats_set_options_), options)),
+      allocator_(stats_set_options_, options.restartEpoch() == 0, shmem_.stats_set_data_),


You will probably need to initialize this below inside the scope where lock is created, which is where the stats set used to be initialized. allocator_ will probably need to be a unique_ptr as a result. This is because the lock protects the initialization. If you move it above to the initializer list, the lock won't be held while it's being constructed. Side note: are you familiar with the idea of a scope lock (std::unique_lock<Thread::BasicLockable>)?

This is a good catch. I reworked allocator_ to be a unique pointer and constructed it not-inline, and changed all references to allocator_.foo to allocator_->foo.

mrice32 · 2018-04-17T17:26:20Z

source/server/hot_restart_nop_impl.h

+        calloc(SharedMemoryHashSet<Stats::RawStatData>::numBytes(options), num_stats));
+    stats_allocator_ = std::make_unique<Stats::BlockRawStatDataAllocator>(options, true, ptr_);
+  }
+  ~HotRestartNopImpl() { ::free(ptr_); }


Be careful, here. I'd be sure that that stats_allocator_ doesn't attempt to write to this memory as it's destructing. This is an artifact of the C++ order of destruction where the destructor runs first, and then the member variables are destroyed. If this is a problem, we can modify the destructor to look like this:

~HotRestartNopImpl() { stats_allocator_ = nullptr; ::free(ptr_); }

I'll do this -- is this the kind of bug I can write a test for? Or is this just intuition that this could fail?

mrice32 · 2018-04-17T17:27:15Z

test/common/stats/stats_impl_test.cc

 }

+TEST(SomeTest, HotAllocConsistencyTest) {
+  int max_stats_ = 16384; // ENVOY_DEFAULT_MAX_STATS


If you want this to match that number, you should probably use the macro directly.

I'll include server/options_impl.h and get access to it.

mrice32 · 2018-04-17T17:28:37Z

test/common/stats/stats_impl_test.cc

+  SharedMemoryHashSetOptions options_ = Server::sharedMemHashOptions(max_stats_);
+  uint8_t* ptr = static_cast<uint8_t*>(
+      calloc(SharedMemoryHashSet<Stats::RawStatData>::numBytes(options_), max_stats_));
+  std::unique_ptr<RawStatDataSet> stats_set_ =


Since you have your fancy new block allocator, why not use it here? :) . Otherwise the test is really testing a reimplementation of your allocator rather than the allocator itself.

Good catch. Rewrote to look like

TEST(SomeTest, HotAllocConsistencyTest) { int max_stats_ = 16384; // ENVOY_DEFAULT_MAX_STATS SharedMemoryHashSetOptions options_ = Server::sharedMemHashOptions(max_stats_); uint8_t* ptr = static_cast<uint8_t*>( calloc(SharedMemoryHashSet<Stats::RawStatData>::numBytes(options_), max_stats_)); Stats::BlockRawStatDataAllocator allocator = Stats::BlockRawStatDataAllocator(options_, true, ptr); auto data_1 = allocator.alloc("foo"); auto data_2 = allocator.alloc("foo"); EXPECT_EQ(data_1, data_2); ::free(ptr); }

Signed-off-by: James Buckland <jbuckland@google.com>

and rework versionHelper to expect a string of the RawStatDataSet version instead of the whole set itself Signed-off-by: James Buckland <jbuckland@google.com>

Signed-off-by: James Buckland <jbuckland@google.com>

…nter, so that it can be initialized after the locks are held Signed-off-by: James Buckland <jbuckland@google.com>

ambuc · 2018-04-17T18:42:44Z

test/common/stats/stats_impl_test.cc

      "No regex specified for tag specifier and no default regex for name: 'test_extractor'");
 }

+TEST(SomeTest, HotAllocConsistencyTest) {


@mrice32 I think I don't have a great way to test this twice, once in the context of injecting shared memory, and once in the context of injecting non-shared memory. Is there a test we can write to prove this works in the original worry case in envoyproxy#2453 ?

Signed-off-by: James Buckland <jbuckland@google.com>

since it is no longer used in shared memory only Signed-off-by: James Buckland <jbuckland@google.com>

Signed-off-by: James Buckland <jbuckland@google.com>

able to be passed in by reference or left null for allocators which don't need it Signed-off-by: James Buckland <jbuckland@google.com>

Signed-off-by: James Buckland <jbuckland@google.com>

…matting fixes Signed-off-by: James Buckland <jbuckland@google.com>

ambuc · 2018-04-18T19:44:53Z

source/server/hot_restart_nop_impl.h

+    ptr_ = static_cast<uint8_t*>(
+        calloc(MemoryHashSet<Stats::RawStatData>::numBytes(options), num_stats));
+    stats_allocator_ =
+        std::make_unique<Stats::BlockRawStatDataAllocator>(options, true, ptr_, &stat_lock_);


@mrice32 when you see new statLock / stat_lock_ objects in HotRestart, HotRestartImpl, HotRestartNopImpl, and HotRestart mock objects, it's because of this line. I wanted to create and use a lock mechanism even when the restart isn't hot.

But why does it need to have its own accessor method?

ambuc · 2018-04-18T19:45:18Z

source/server/options_impl.cc

 #include "tclap/CmdLine.h"

-// Can be overridden at compile time
-#ifndef ENVOY_DEFAULT_MAX_STATS


@mrice32 I moved this to options_impl.h for visibility.

ambuc · 2018-04-18T19:46:44Z

source/common/common/BUILD

 )

 envoy_cc_library(
-    name = "shared_memory_hash_set_lib",


@mrice32 This is one of a few commits which rename SharedMemoryHashSet to MemoryHashSet, or shared_memory_hash_set to memory_hash_set, or sharedMemOptions to memOptions. I wanted to indicate that the MemoryHashSet doesn't need to be run on shared memory.

ambuc · 2018-04-18T19:47:51Z

source/common/stats/stats_impl.cc

  return false;
 }

+RawStatData* BlockRawStatDataAllocator::alloc(const std::string& name) {


I moved a lot of these tests from hot_restart_impl_test.cc since they are really unit tests on the allocator itself. A few stayed in hot_restart_impl_test.cc as they tested behavior across hot restart.

What tests are you referring to? This is an implementation file...?

Wow, I don't know how this ended up here. I think I meant to say I moved these ::alloc and ::free functions from hot_restart_impl.cc.

ambuc · 2018-04-18T19:48:20Z

test/server/hot_restart_impl_test.cc

  EXPECT_EQ(stat5, stat5_prime);
 }

-TEST_F(HotRestartImplTest, truncateKey) {


I moved a lot of these to stats_impl.cc. See the other comment here: https://github.com/ambuc/envoy/pull/1/files#r182548087

ambuc · 2018-04-18T19:50:12Z

source/server/hot_restart_impl.cc


 Stats::RawStatData* HotRestartImpl::alloc(const std::string& name) {
  // Try to find the existing slot in shared memory, otherwise allocate a new one.
-  std::unique_lock<Thread::BasicLockable> lock(stat_lock_);


The whole functionality of HotRestartImpl::alloc, including the lock, got moved up into BlockRawStatDataAllocator::alloc. Same for ::free().

ambuc · 2018-04-18T19:51:45Z

source/server/hot_restart_impl.cc

 void HotRestartImpl::shutdown() { socket_event_.reset(); }

 std::string HotRestartImpl::version() {
-  return versionHelper(shmem_.maxStats(), Stats::RawStatData::maxNameLength(), *stats_set_);


@mrice32 I wanted BlockRawStatDataAllocator's stats_set_ member variable to be private, so I ended up writing a function here [1] to just fetch the version as a string.

[1] https://github.com/ambuc/envoy/pull/1/files#diff-fb83459430ccfd2f0d934dcfacdbd9d1R399

Seems reasonable to me.

ambuc · 2018-04-18T19:52:11Z

source/server/hot_restart_impl.cc

+  Stats::RawStatDataSet stats_set(options, true /* init */, mem_buffer_for_dry_run_.get());
+  auto stats_set_version = stats_set.version();

-  return versionHelper(max_num_stats, max_stat_name_len, stats_set);


See the comment here https://github.com/ambuc/envoy/pull/1/files#r182549180, we can now pass the string version instead of the whole set by reference.

ambuc · 2018-04-18T19:52:28Z

source/server/hot_restart_impl.h

  RpcBase* receiveRpc(bool block);
  void sendMessage(sockaddr_un& address, RpcBase& rpc);
  static std::string versionHelper(uint64_t max_num_stats, uint64_t max_stat_name_len,
-                                   RawStatDataSet& stats_set);


See https://github.com/ambuc/envoy/pull/1/files#r182549180

ambuc · 2018-04-18T19:53:24Z

source/server/hot_restart_nop_impl.h

 public:
-  HotRestartNopImpl() {}
+  HotRestartNopImpl() {
+    int num_stats = 100;


@mrice32 TODO(jbuckland) find a less arbitrary number for this. I seem to recall setting this to ENVOY_DEFAULT_MAX_STATS caused testing to hang. What should we do here?

This is not a test, this is the real application, so this must be pulled from the supplied command line options/default just like the hot restart implementation does. I would suggest supplying it via the constructor just like HotRestartImpl:

HotRestartImpl(Options& options);

ambuc · 2018-04-18T19:54:43Z

source/server/options_impl.cc

 #include "tclap/CmdLine.h"

-// Can be overridden at compile time
-#ifndef ENVOY_DEFAULT_MAX_STATS


Moved to options_impl.h for visibility.

Makes sense.

mrice32

Looks good so far - left a few comments.

mrice32 · 2018-04-18T22:33:51Z

source/common/stats/stats_impl.h

+   * @return BlockRawStatDataAllocator newly constructed BlockRawStatDataAllocator.
+   */
+  BlockRawStatDataAllocator(const MemoryHashSetOptions& options, bool init, uint8_t* memory,
+                            Thread::BasicLockable* stat_lock)


Just a note: if we decide that we want this in all cases, we might consider making this a reference. Or if we want this class to own it, we could pass in a unique_ptr and check that it's not nullptr.

mrice32 · 2018-04-18T22:36:17Z

source/server/hot_restart_impl.cc

-    std::unique_lock<Thread::BasicLockable> lock(stat_lock_);
-    stats_set_.reset(new RawStatDataSet(stats_set_options_, options.restartEpoch() == 0,
-                                        shmem_.stats_set_data_));
+    allocator_ = std::make_unique<Stats::BlockRawStatDataAllocator>(


Why did you get rid of the locking here?

I moved the locking functionality inside the BlockRawStatDataAllocator itself, which is why it gets initialized with a reference to a lock.

mrice32 · 2018-04-18T22:36:45Z

source/server/hot_restart_impl.cc

-  ASSERT(key_removed);
-  memset(&data, 0, Stats::RawStatData::size());
-}
+void HotRestartImpl::free(Stats::RawStatData& data) { allocator_->free(data); }


Why are we still forwarding these calls?

mrice32 · 2018-04-18T22:40:37Z

source/server/hot_restart_nop_impl.h

 public:
-  HotRestartNopImpl() {}
+  HotRestartNopImpl() {
+    int num_stats = 100;


This is not a test, this is the real application, so this must be pulled from the supplied command line options/default just like the hot restart implementation does. I would suggest supplying it via the constructor just like HotRestartImpl:

HotRestartImpl(Options& options);

mrice32 · 2018-04-18T22:42:33Z

source/server/hot_restart_nop_impl.h

+    ptr_ = static_cast<uint8_t*>(
+        calloc(MemoryHashSet<Stats::RawStatData>::numBytes(options), num_stats));
+    stats_allocator_ =
+        std::make_unique<Stats::BlockRawStatDataAllocator>(options, true, ptr_, &stat_lock_);


But why does it need to have its own accessor method?

mrice32 · 2018-04-18T22:46:43Z

test/common/stats/stats_impl_test.cc

+  void setup(int num_stats) {
+    Stats::RawStatData::configureForTestsOnly(options_);
+    MemoryHashSetOptions memory_hash_set_options_ = Server::memHashOptions(num_stats);
+    ptr_ = static_cast<uint8_t*>(


We should probably move to the way Josh allocates the memory in his tests - see here. Something like:

uint32_t num_bytes = MemoryHashSet<Stats::RawStatData>::numBytes(memory_hash_set_options_); ptr_ = new uint8_t[num_bytes];

Side note: why are you multiplying the number of bytes times the number of stats? That may be the source of your issues - you're squaring the capacity.

Great catch! I've updated this.

Just kidding, I updated this for HotRestartNopImpl. I'll update it here too.

mrice32 · 2018-04-18T22:55:27Z

source/server/hot_restart_impl.cc

+  Stats::RawStatDataSet stats_set(options, true /* init */, mem_buffer_for_dry_run_.get());
+  auto stats_set_version = stats_set.version();

-  return versionHelper(max_num_stats, max_stat_name_len, stats_set);


…tics Signed-off-by: James Buckland <jbuckland@google.com>

…ly them with memory, have unit tests, and are not child classes of RawStatDataAllocator instead, they are called (in a few places) as restart_obj_.statsAllocator().alloc() or restart_obj.statsAllocator().free() Signed-off-by: James Buckland <jbuckland@google.com>

Signed-off-by: James Buckland <jbuckland@google.com>

mrice32 · 2018-04-20T17:07:20Z

source/server/hot_restart_nop_impl.h

 #include "common/common/thread.h"
+#include "common/stats/stats_impl.h"
+
+#include "server/hot_restart_impl.h"


Why does this include hot_restart_impl.h?

it needs blockMemHashOptions from there.

mrice32 · 2018-04-20T17:08:53Z

source/server/options_impl.cc

 #include "tclap/CmdLine.h"

-// Can be overridden at compile time
-#ifndef ENVOY_DEFAULT_MAX_STATS


Makes sense.

mrice32 · 2018-04-20T17:43:56Z

test/server/hot_restart_nop_impl_test.cc

+  stat2 = nullptr;
+  stat4 = nullptr;
+
+  EXPECT_CALL(options_, restartEpoch()).WillRepeatedly(Return(1));


Why is this here and why would the nopimpl call this method?

You're right, this is leftover from the sameAlloc function when it was being called on a more tightly coupled instance of HotRestart.

mrice32 · 2018-04-20T17:45:45Z

source/common/stats/stats_impl.cc

  return false;
 }

+RawStatData* BlockRawStatDataAllocator::alloc(const std::string& name) {


What tests are you referring to? This is an implementation file...?

mrice32 · 2018-04-20T17:46:45Z

source/server/hot_restart_impl.cc

 void HotRestartImpl::shutdown() { socket_event_.reset(); }

 std::string HotRestartImpl::version() {
-  return versionHelper(shmem_.maxStats(), Stats::RawStatData::maxNameLength(), *stats_set_);


Seems reasonable to me.

Signed-off-by: James Buckland <jbuckland@google.com>

…instead of taking pointer to it Signed-off-by: James Buckland <jbuckland@google.com>

Signed-off-by: James Buckland <jbuckland@google.com>

… for proper scope testing Signed-off-by: James Buckland <jbuckland@google.com>

Signed-off-by: James Buckland <jbuckland@google.com>

ambuc · 2018-04-20T21:25:33Z

test/common/stats/thread_local_store_test.cc

- * This is a heap test allocator that works similar to how the shared memory allocator works in
- * terms of reference counting, etc.
- */
-class TestAllocator : public RawStatDataAllocator {


@mrice32 I was able to completely swap in this TestAllocator for a real, functional BlockRawStatDataAllocator, which ends up running the below suite of namespace resolution tests.

ambuc · 2018-04-23T12:51:21Z

Closed in favor of envoyproxy#3163.

…ot restart (#3249) Simpler solution to issue #2453 than pull #3163, continuing draft work in ambuc#1 and ambuc#2. Summary of changes: adds an unordered_map named stats_set_ as a member variable of HeapRawStatDataAllocator, and implements reference counting / dedup on allocated stats. Risk Level: Low. Testing: Add a test to stats_impl_test. Passes bazel test test/.... Docs Changes: N/A Release Notes: This is user-facing in that non-hot restart stat allocation now resolves namespace properly, but no effect on user configs. Fixes: #2453 Signed-off-by: James Buckland <jbuckland@google.com>

…ardown. (envoyproxy#4940) server_fuzz_test indicated the below crash, where the DispatcherImpl teardown releases some upstream client SSL related objects that then needs SecretManagerImpl to unregister. Previously, this was already destructed by time we were in ~DispatcherImpl(), this PR reorders. #0 0xc1e826 in size /usr/local/include/c++/v1/__hash_table:809:55 #1 0xc1e826 in bucket_count /usr/local/include/c++/v1/__hash_table:1197 #2 0xc1e826 in std::__1::__hash_iterator<std::__1::__hash_node<std::__1::__hash_value_type<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::weak_ptr<Envoy::Secret::TlsCertificateSdsApi> >, void*>*> std::__1::__hash_table<std::__1::__hash_value_type<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::weak_ptr<Envoy::Secret::TlsCertificateSdsApi> >, std::__1::__unordered_map_hasher<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::__hash_value_type<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::weak_ptr<Envoy::Secret::TlsCertificateSdsApi> >, std::__1::hash<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >, true>, std::__1::__unordered_map_equal<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::__hash_value_type<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::weak_ptr<Envoy::Secret::TlsCertificateSdsApi> >, std::__1::equal_to<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >, true>, std::__1::allocator<std::__1::__hash_value_type<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::weak_ptr<Envoy::Secret::TlsCertificateSdsApi> > > >::find<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) /usr/local/include/c++/v1/__hash_table:2334 #3 0xc1e278 in unsigned long std::__1::__hash_table<std::__1::__hash_value_type<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::weak_ptr<Envoy::Secret::TlsCertificateSdsApi> >, std::__1::__unordered_map_hasher<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::__hash_value_type<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::weak_ptr<Envoy::Secret::TlsCertificateSdsApi> >, std::__1::hash<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >, true>, std::__1::__unordered_map_equal<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::__hash_value_type<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::weak_ptr<Envoy::Secret::TlsCertificateSdsApi> >, std::__1::equal_to<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >, true>, std::__1::allocator<std::__1::__hash_value_type<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::weak_ptr<Envoy::Secret::TlsCertificateSdsApi> > > >::__erase_unique<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) /usr/local/include/c++/v1/__hash_table:2510:20 #4 0xc1def6 in erase /usr/local/include/c++/v1/unordered_map:1156:59 #5 0xc1def6 in Envoy::Secret::SecretManagerImpl::DynamicSecretProviders<Envoy::Secret::TlsCertificateSdsApi>::removeDynamicSecretProvider(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) /bazel-out/k8-fastbuild/bin/source/common/secret/_virtual_includes/secret_manager_impl_lib/common/secret/secret_manager_impl.h:75 #6 0x724aa9 in Envoy::Cleanup::~Cleanup() /bazel-out/k8-fastbuild/bin/source/common/common/_virtual_includes/cleanup_lib/common/common/cleanup.h:11:16 #7 0xc1ff33 in Envoy::Secret::SdsApi::~SdsApi() /bazel-out/k8-fastbuild/bin/source/common/secret/_virtual_includes/sds_api_lib/common/secret/sds_api.h:29:7 #8 0xc188a0 in __release_shared /usr/local/include/c++/v1/memory:3530:9 #9 0xc188a0 in __release_shared /usr/local/include/c++/v1/memory:3572 #10 0xc188a0 in std::__1::shared_ptr<Envoy::Secret::SecretProvider<Envoy::Ssl::TlsCertificateConfig> >::~shared_ptr() /usr/local/include/c++/v1/memory:4508 #11 0x149c922 in Envoy::Ssl::ContextConfigImpl::~ContextConfigImpl() /source/common/ssl/context_config_impl.cc:117:1 #12 0x14a0d8f in Envoy::Ssl::ClientContextConfigImpl::~ClientContextConfigImpl() /bazel-out/k8-fastbuild/bin/source/common/ssl/_virtual_includes/context_config_lib/common/ssl/context_config_impl.h:91:7 #13 0x14a0dc8 in Envoy::Ssl::ClientContextConfigImpl::~ClientContextConfigImpl() /bazel-out/k8-fastbuild/bin/source/common/ssl/_virtual_includes/context_config_lib/common/ssl/context_config_impl.h:91:7 #14 0x149815b in operator() /usr/local/include/c++/v1/memory:2325:5 #15 0x149815b in reset /usr/local/include/c++/v1/memory:2638 #16 0x149815b in ~unique_ptr /usr/local/include/c++/v1/memory:2592 #17 0x149815b in Envoy::Ssl::ClientSslSocketFactory::~ClientSslSocketFactory() /bazel-out/k8-fastbuild/bin/source/common/ssl/_virtual_includes/ssl_socket_lib/common/ssl/ssl_socket.h:83 #18 0x14981c8 in Envoy::Ssl::ClientSslSocketFactory::~ClientSslSocketFactory() /bazel-out/k8-fastbuild/bin/source/common/ssl/_virtual_includes/ssl_socket_lib/common/ssl/ssl_socket.h:83:7 #19 0x1362caf in operator() /usr/local/include/c++/v1/memory:2325:5 #20 0x1362caf in reset /usr/local/include/c++/v1/memory:2638 #21 0x1362caf in ~unique_ptr /usr/local/include/c++/v1/memory:2592 #22 0x1362caf in Envoy::Upstream::ClusterInfoImpl::~ClusterInfoImpl() /bazel-out/k8-fastbuild/bin/source/common/upstream/_virtual_includes/upstream_includes/common/upstream/upstream_impl.h:362 #23 0x1362d28 in Envoy::Upstream::ClusterInfoImpl::~ClusterInfoImpl() /bazel-out/k8-fastbuild/bin/source/common/upstream/_virtual_includes/upstream_includes/common/upstream/upstream_impl.h:362:7 #24 0x66e560 in __release_shared /usr/local/include/c++/v1/memory:3530:9 #25 0x66e560 in __release_shared /usr/local/include/c++/v1/memory:3572 #26 0x66e560 in std::__1::shared_ptr<Envoy::Upstream::ClusterInfo const>::~shared_ptr() /usr/local/include/c++/v1/memory:4508 #27 0x13621bf in Envoy::Upstream::HostImpl::~HostImpl() /bazel-out/k8-fastbuild/bin/source/common/upstream/_virtual_includes/upstream_includes/common/upstream/upstream_impl.h:156:7 #28 0x13621f8 in Envoy::Upstream::HostImpl::~HostImpl() /bazel-out/k8-fastbuild/bin/source/common/upstream/_virtual_includes/upstream_includes/common/upstream/upstream_impl.h:156:7 #29 0x66e650 in __release_shared /usr/local/include/c++/v1/memory:3530:9 #30 0x66e650 in __release_shared /usr/local/include/c++/v1/memory:3572 #31 0x66e650 in std::__1::shared_ptr<Envoy::Upstream::HostDescription const>::~shared_ptr() /usr/local/include/c++/v1/memory:4508 #32 0x13b20c3 in Envoy::Http::CodecClient::~CodecClient() /source/common/http/codec_client.cc:38:30 #33 0x13b2258 in Envoy::Http::CodecClientProd::~CodecClientProd() /bazel-out/k8-fastbuild/bin/source/common/http/_virtual_includes/codec_client_lib/common/http/codec_client.h:229:7 #34 0x751de6 in operator() /usr/local/include/c++/v1/memory:2325:5 #35 0x751de6 in reset /usr/local/include/c++/v1/memory:2638 #36 0x751de6 in ~unique_ptr /usr/local/include/c++/v1/memory:2592 #37 0x751de6 in destroy /usr/local/include/c++/v1/memory:1867 #38 0x751de6 in __destroy<std::__1::unique_ptr<Envoy::Event::DeferredDeletable, std::__1::default_delete<Envoy::Event::DeferredDeletable> > > /usr/local/include/c++/v1/memory:1729 #39 0x751de6 in destroy<std::__1::unique_ptr<Envoy::Event::DeferredDeletable, std::__1::default_delete<Envoy::Event::DeferredDeletable> > > /usr/local/include/c++/v1/memory:1597 #40 0x751de6 in __destruct_at_end /usr/local/include/c++/v1/vector:422 #41 0x751de6 in clear /usr/local/include/c++/v1/vector:365 #42 0x751de6 in std::__1::__vector_base<std::__1::unique_ptr<Envoy::Event::DeferredDeletable, std::__1::default_delete<Envoy::Event::DeferredDeletable> >, std::__1::allocator<std::__1::unique_ptr<Envoy::Event::DeferredDeletable, std::__1::default_delete<Envoy::Event::DeferredDeletable> > > >::~__vector_base() /usr/local/include/c++/v1/vector:459 #43 0x74d1aa in ~vector /usr/local/include/c++/v1/vector:551:5 #44 0x74d1aa in Envoy::Event::DispatcherImpl::~DispatcherImpl() /source/common/event/dispatcher_impl.cc:41 #45 0x74d658 in Envoy::Event::DispatcherImpl::~DispatcherImpl() /source/common/event/dispatcher_impl.cc:41:35 #46 0x697b76 in operator() /usr/local/include/c++/v1/memory:2325:5 #47 0x697b76 in reset /usr/local/include/c++/v1/memory:2638 #48 0x697b76 in ~unique_ptr /usr/local/include/c++/v1/memory:2592 #49 0x697b76 in Envoy::Server::InstanceImpl::InstanceImpl(Envoy::Server::Options&, Envoy::Event::TimeSystem&, std::__1::shared_ptr<Envoy::Network::Address::Instance const>, Envoy::TestHooks&, Envoy::Server::HotRestart&, Envoy::Stats::StoreRoot&, Envoy::Thread::BasicLockable&, Envoy::Server::ComponentFactory&, std::__1::unique_ptr<Envoy::Runtime::RandomGenerator, std::__1::default_delete<Envoy::Runtime::RandomGenerator> >&&, Envoy::ThreadLocal::Instance&) /source/server/server.cc:92 #50 0x586026 in make_unique<Envoy::Server::InstanceImpl, testing::NiceMock<Envoy::Server::MockOptions> &, Envoy::Event::TestTimeSystem &, std::__1::shared_ptr<Envoy::Network::Address::Ipv4Instance>, Envoy::DefaultTestHooks &, testing::NiceMock<Envoy::Server::MockHotRestart> &, Envoy::Stats::TestIsolatedStoreImpl &, Envoy::Thread::MutexBasicLockable &, Envoy::Server::TestComponentFactory &, std::__1::unique_ptr<Envoy::Runtime::RandomGeneratorImpl, std::__1::default_delete<Envoy::Runtime::RandomGeneratorImpl> >, Envoy::ThreadLocal::InstanceImpl &> /usr/local/include/c++/v1/memory:3118:32 #51 0x586026 in Envoy::Server::TestOneProtoInput(envoy::config::bootstrap::v2::Bootstrap const&) /test/server/server_fuzz_test.cc:78 Fixes oss-fuzz issue https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=11231 Risk Level: Low Testing: Corpus entry added. It's pretty hard to build regressions for this kind of destruction ordering, so relying on server_fuzz_test + corpus. Signed-off-by: Harvey Tuch <htuch@google.com>

Provide the HTTP path normalization per RFC 3986 (sans case normalization). This addresses CVE-2019-9901. The config HttpConnectionManager.normalize_path needs to be set for each HCM configuration to enable (default is off). There is also a runtime optione http_connection_manager.normalize_path to change this default when not set in HCM. Risk level: Low Testing: New unit and integration tests added. Signed-off-by: Yuchen Dai <silentdai@gmail.com> Signed-off-by: Harvey Tuch <htuch@google.com>

ambuc added 6 commits April 11, 2018 18:44

Initial hot/cold allocation proof of concept

e3f59d5

Signed-off-by: James Buckland <jbuckland@google.com>

Drafting out new BlockStatDataAllocator colocated for now

bfd2084

Signed-off-by: James Buckland <jbuckland@google.com>

Working draft of BlockRawStatDataAllocator passing all AllocConsisten…

c306c01

…cyTests Signed-off-by: James Buckland <jbuckland@google.com>

HotRestartImpl now relies upon BlockRawStatDataAllocator

a263690

HeapRawStatDataAllocator still does heap allocation without namespace resolution Signed-off-by: James Buckland <jbuckland@google.com>

Formatting updates

9298e3c

Signed-off-by: James Buckland <jbuckland@google.com>

Assorted nits

f1e94fa

Signed-off-by: James Buckland <jbuckland@google.com>

mrice32 reviewed Apr 17, 2018

View reviewed changes

ambuc added 4 commits April 17, 2018 13:54

Actually use BlockRawStatDataAllocator in BlockRawStatDataAllocator test

e49a15a

Signed-off-by: James Buckland <jbuckland@google.com>

Make BlockRawStatDataAllocator member variable stats_set_ private

d7dc10e

and rework versionHelper to expect a string of the RawStatDataSet version instead of the whole set itself Signed-off-by: James Buckland <jbuckland@google.com>

Formatting updates

de2879d

Signed-off-by: James Buckland <jbuckland@google.com>

Rework HotRestartImpl's member variable allocator_ to be a unique poi…

e9c65fa

…nter, so that it can be initialized after the locks are held Signed-off-by: James Buckland <jbuckland@google.com>

ambuc commented Apr 17, 2018

View reviewed changes

ambuc added 7 commits April 18, 2018 09:59

Remove not-ready tests

5dde224

Signed-off-by: James Buckland <jbuckland@google.com>

Rename SharedMemoryHashSet to MemoryHashSet

46d4b9e

since it is no longer used in shared memory only Signed-off-by: James Buckland <jbuckland@google.com>

Move allocator unit tests from hot_restart_impl_test to stats_impl_test

5466e13

Signed-off-by: James Buckland <jbuckland@google.com>

Add uniqueNaming test

04d28f2

Signed-off-by: James Buckland <jbuckland@google.com>

Add stat_lock_ mutex to BlockRawStatDataAllocator

bc25b35

able to be passed in by reference or left null for allocators which don't need it Signed-off-by: James Buckland <jbuckland@google.com>

Add stat_lock for mocking purposes inside HotRestart abstract class

adf9a6e

Signed-off-by: James Buckland <jbuckland@google.com>

Add some documentation around BlockRawStatDataAllocator, assorted for…

3dde624

…matting fixes Signed-off-by: James Buckland <jbuckland@google.com>

ambuc commented Apr 18, 2018

View reviewed changes

mrice32 reviewed Apr 18, 2018

View reviewed changes

ambuc added 3 commits April 19, 2018 13:19

Undo statLock stuff, replace calloc with more c-like allocation seman…

3c773a8

…tics Signed-off-by: James Buckland <jbuckland@google.com>

Assorted formatting fixes

2b9f176

Signed-off-by: James Buckland <jbuckland@google.com>

mrice32 reviewed Apr 20, 2018

View reviewed changes

ambuc added 6 commits April 20, 2018 14:18

Rename SharedMemoryHashSet to BlockMemoryHashSet

1916c2c

Signed-off-by: James Buckland <jbuckland@google.com>

Actually pass in stat_lock by reference to BlockRawStatDataAllocator …

765df8e

…instead of taking pointer to it Signed-off-by: James Buckland <jbuckland@google.com>

Formatting updates

afeb8f2

Signed-off-by: James Buckland <jbuckland@google.com>

Minor nits

5aff3d1

Signed-off-by: James Buckland <jbuckland@google.com>

Swap out TestAllocator for real instance of BlockRawStatDataAllocator…

cc22d9e

… for proper scope testing Signed-off-by: James Buckland <jbuckland@google.com>

Formatting updates

fafdf72

Signed-off-by: James Buckland <jbuckland@google.com>

ambuc commented Apr 20, 2018

View reviewed changes

ambuc closed this Apr 23, 2018

Conversation

ambuc commented Apr 17, 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!

mrice32 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!

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!

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!

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!

mrice32 left a comment

ambuc commented Apr 17, 2018 •

edited

Loading