Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[DRAFT] refcount stats across scopes #2

Closed
wants to merge 7 commits into from
Closed

[DRAFT] refcount stats across scopes #2

wants to merge 7 commits into from

Conversation

ambuc
Copy link
Owner

@ambuc ambuc commented Apr 25, 2018

draft solution to envoyproxy#2453

mrice32
mrice32 reviewed Apr 25, 2018
View reviewed changes
source/common/stats/stats_impl.cc Outdated
key.remove_suffix(key.size() - Stats::RawStatData::maxNameLength());
}

RawStatData* data = stats_set_[std::string(key)];
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're copying this pointer, not grabbing a reference to it (yes, a reference to a pointer haha), so anything you do to it after this is only affecting the local variable data and not the pointer you copied from in the map.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the syntax for a ref to a pointer: RawStatData*& data.

source/common/stats/stats_impl.cc Outdated

RawStatData* data = stats_set_[std::string(key)];

if (stats_set_.count(std::string(key)) == 1) { // TODO do better
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this if statement is backwards.

mrice32
mrice32 reviewed Apr 25, 2018
View reviewed changes
source/common/stats/stats_impl.cc Outdated
key.remove_suffix(key.size() - Stats::RawStatData::maxNameLength());
}

bool name_not_in_map = (stats_set_.find(std::string(key)) == stats_set_.end());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for parens. It's safe to assume that the = is the last part to be evaluated :)

source/common/stats/stats_impl.cc Outdated

bool name_not_in_map = (stats_set_.find(std::string(key)) == stats_set_.end());

RawStatData*& data = stats_set_[std::string(key)];
Copy link

@mrice32 mrice32 Apr 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can condense this and the above into a single step (see http://en.cppreference.com/w/cpp/container/unordered_map/insert):

auto ret = stats_set_.insert(std::pair<std::string, RawStatData*>(std::string(key), nullptr));
RawStatData*& data = *ret.first;
if (ret.second) {
  // doesn't already exist 
.
.
.

source/common/stats/stats_impl.cc Outdated
if (name_not_in_map) {
// didn't exist before now, actually create
data = static_cast<RawStatData*>(::calloc(RawStatData::size(), 1));
data->initialize(name);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this need to be truncated?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it expects the full name, not the truncated key. You can see initialize() here wants to warn users if the stats are too long.

mrice32
mrice32 reviewed Apr 25, 2018
View reviewed changes
source/common/stats/stats_impl.cc

if (key.size() > Stats::RawStatData::maxNameLength()) {
key.remove_suffix(key.size() - Stats::RawStatData::maxNameLength());
ENVOY_LOG_MISC(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this here? This warning only happens if it gets all the way to the initialize() call in hot restart.

source/common/stats/stats_impl.cc Outdated
auto ret = stats_set_.insert(std::pair<std::string, RawStatData*>(std::string(key), nullptr));
RawStatData*& data = ret.first->second;
if (ret.second) {
// didn't exist before now, actually create
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: make this a full sentence comment or remove.

@ambuc ambuc closed this Apr 25, 2018
This was referenced Apr 25, 2018
Closed
Merged
htuch pushed a commit to envoyproxy/envoy that referenced this pull request May 4, 2018
@ambuc @htuch
Fix issue with Envoy not reference counting across scopes under not-h…
795848b 
…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 <[email protected]>
ambuc pushed a commit that referenced this pull request Nov 2, 2018
@htuch
server: reorder SecretManagerImpl field to avoid heap-after-use in te…
871155f 
…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 <[email protected]>
ambuc pushed a commit that referenced this pull request Jun 3, 2019
@htuch
codec: reject embedded NUL in headers. (#2)
b155af7 
http-parser doesn't sanitize header values as per RFC 7230 today, so add an
additional check (yielding a CodecProtocolException) for this. See CVE-2019-9900 for further details.

Added CR/LF, in addition to NUL, as prohibited in header strings as per
RFC 7230.

Also added codec_impl_tests for H1 and H2 codecs to validate that
NUL/LF/CR mutations in a request don't violate internal invariants.

Fixes CVE-2019-9900 and oss-fuzz issue
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=13613.

Risk level: Low
Testing: Corpus entry and unit test added.

Signed-off-by: Harvey Tuch <[email protected]>
ambuc pushed a commit that referenced this pull request Dec 15, 2021
Xds mux unification: introduced unifed mux implementation (envoyproxy…
fd6e675 
…#17352)

PR #2 out of 3. Introduces unified mux implementation

Risk Level: Low, nor enabled atm
Testing: Updated tests

Signed-off-by: Dmitri Dolguikh <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrice32 mrice32 mrice32 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ambuc @mrice32