server: reorder SecretManagerImpl field to avoid heap-after-use in te…#4940
Merged
htuch merged 1 commit intoenvoyproxy:masterfrom Nov 1, 2018
Merged
server: reorder SecretManagerImpl field to avoid heap-after-use in te…#4940htuch merged 1 commit intoenvoyproxy:masterfrom
htuch merged 1 commit intoenvoyproxy:masterfrom
Conversation
…ardown.
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
envoyproxy#1 0xc1e826 in bucket_count /usr/local/include/c++/v1/__hash_table:1197
envoyproxy#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
envoyproxy#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
envoyproxy#4 0xc1def6 in erase /usr/local/include/c++/v1/unordered_map:1156:59
envoyproxy#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
envoyproxy#6 0x724aa9 in Envoy::Cleanup::~Cleanup() /bazel-out/k8-fastbuild/bin/source/common/common/_virtual_includes/cleanup_lib/common/common/cleanup.h:11:16
envoyproxy#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
envoyproxy#8 0xc188a0 in __release_shared /usr/local/include/c++/v1/memory:3530:9
envoyproxy#9 0xc188a0 in __release_shared /usr/local/include/c++/v1/memory:3572
envoyproxy#10 0xc188a0 in std::__1::shared_ptr<Envoy::Secret::SecretProvider<Envoy::Ssl::TlsCertificateConfig> >::~shared_ptr() /usr/local/include/c++/v1/memory:4508
envoyproxy#11 0x149c922 in Envoy::Ssl::ContextConfigImpl::~ContextConfigImpl() /source/common/ssl/context_config_impl.cc:117:1
envoyproxy#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
envoyproxy#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
envoyproxy#14 0x149815b in operator() /usr/local/include/c++/v1/memory:2325:5
envoyproxy#15 0x149815b in reset /usr/local/include/c++/v1/memory:2638
envoyproxy#16 0x149815b in ~unique_ptr /usr/local/include/c++/v1/memory:2592
envoyproxy#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
envoyproxy#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
envoyproxy#19 0x1362caf in operator() /usr/local/include/c++/v1/memory:2325:5
envoyproxy#20 0x1362caf in reset /usr/local/include/c++/v1/memory:2638
envoyproxy#21 0x1362caf in ~unique_ptr /usr/local/include/c++/v1/memory:2592
envoyproxy#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
envoyproxy#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
envoyproxy#24 0x66e560 in __release_shared /usr/local/include/c++/v1/memory:3530:9
envoyproxy#25 0x66e560 in __release_shared /usr/local/include/c++/v1/memory:3572
envoyproxy#26 0x66e560 in std::__1::shared_ptr<Envoy::Upstream::ClusterInfo const>::~shared_ptr() /usr/local/include/c++/v1/memory:4508
envoyproxy#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
envoyproxy#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
envoyproxy#29 0x66e650 in __release_shared /usr/local/include/c++/v1/memory:3530:9
envoyproxy#30 0x66e650 in __release_shared /usr/local/include/c++/v1/memory:3572
envoyproxy#31 0x66e650 in std::__1::shared_ptr<Envoy::Upstream::HostDescription const>::~shared_ptr() /usr/local/include/c++/v1/memory:4508
envoyproxy#32 0x13b20c3 in Envoy::Http::CodecClient::~CodecClient() /source/common/http/codec_client.cc:38:30
envoyproxy#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
envoyproxy#34 0x751de6 in operator() /usr/local/include/c++/v1/memory:2325:5
envoyproxy#35 0x751de6 in reset /usr/local/include/c++/v1/memory:2638
envoyproxy#36 0x751de6 in ~unique_ptr /usr/local/include/c++/v1/memory:2592
envoyproxy#37 0x751de6 in destroy /usr/local/include/c++/v1/memory:1867
envoyproxy#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
envoyproxy#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
envoyproxy#40 0x751de6 in __destruct_at_end /usr/local/include/c++/v1/vector:422
envoyproxy#41 0x751de6 in clear /usr/local/include/c++/v1/vector:365
envoyproxy#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
envoyproxy#43 0x74d1aa in ~vector /usr/local/include/c++/v1/vector:551:5
envoyproxy#44 0x74d1aa in Envoy::Event::DispatcherImpl::~DispatcherImpl() /source/common/event/dispatcher_impl.cc:41
envoyproxy#45 0x74d658 in Envoy::Event::DispatcherImpl::~DispatcherImpl() /source/common/event/dispatcher_impl.cc:41:35
envoyproxy#46 0x697b76 in operator() /usr/local/include/c++/v1/memory:2325:5
envoyproxy#47 0x697b76 in reset /usr/local/include/c++/v1/memory:2638
envoyproxy#48 0x697b76 in ~unique_ptr /usr/local/include/c++/v1/memory:2592
envoyproxy#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
envoyproxy#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
envoyproxy#51 0x586026 in Envoy::Server::TestOneProtoInput(envoy::config::bootstrap::v2::Bootstrap const&) /test/server/server_fuzz_test.cc:78
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>
mattklein123
approved these changes
Nov 1, 2018
htuch
added a commit
to htuch/envoy
that referenced
this pull request
Feb 5, 2019
Same issue as in envoyproxy#4940, but on the config_validation side. Risk level: Low Testing: corpus entry added. Signed-off-by: Harvey Tuch <htuch@google.com>
htuch
added a commit
that referenced
this pull request
Feb 8, 2019
Same issue as in #4940, but on the config_validation side. Risk level: Low Testing: corpus entry added. Signed-off-by: Harvey Tuch <htuch@google.com>
htuch
added a commit
to htuch/envoy
that referenced
this pull request
Feb 21, 2019
Another heap-user-after-free, this time we were missing a fix that had been applied to server.h but not config_validation/server.h (envoyproxy#4940). While working on this, attempted to make fully consistent and as simple/clear as possible the constraints on member ordering. This PR is in the tradition (!) of envoyproxy#5847, envoyproxy#4940, envoyproxy#4937. I think long-term we might want to think of more dynamic and explicit declaration of ordering constraints, it's evidently pretty fragile. Also, the lack of single-source-of-truth and duplication across prod and config server code bites again. Fixes oss-fuzz issue https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=13228. Risk level: Low Testing: Corpus entry added. Signed-off-by: Harvey Tuch <htuch@google.com>
htuch
added a commit
that referenced
this pull request
Feb 21, 2019
Another heap-user-after-free, this time we were missing a fix that had been applied to server.h but not config_validation/server.h (#4940). While working on this, attempted to make fully consistent and as simple/clear as possible the constraints on member ordering. This PR is in the tradition (!) of #5847, #4940, #4937. I think long-term we might want to think of more dynamic and explicit declaration of ordering constraints, it's evidently pretty fragile. Also, the lack of single-source-of-truth and duplication across prod and config server code bites again. Fixes oss-fuzz issue https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=13228. Risk level: Low Testing: Corpus entry added. Signed-off-by: Harvey Tuch <htuch@google.com>
fredlas
pushed a commit
to fredlas/envoy
that referenced
this pull request
Mar 5, 2019
Same issue as in envoyproxy#4940, but on the config_validation side. Risk level: Low Testing: corpus entry added. Signed-off-by: Harvey Tuch <htuch@google.com> Signed-off-by: Fred Douglas <fredlas@google.com>
fredlas
pushed a commit
to fredlas/envoy
that referenced
this pull request
Mar 5, 2019
…#6023) Another heap-user-after-free, this time we were missing a fix that had been applied to server.h but not config_validation/server.h (envoyproxy#4940). While working on this, attempted to make fully consistent and as simple/clear as possible the constraints on member ordering. This PR is in the tradition (!) of envoyproxy#5847, envoyproxy#4940, envoyproxy#4937. I think long-term we might want to think of more dynamic and explicit declaration of ordering constraints, it's evidently pretty fragile. Also, the lack of single-source-of-truth and duplication across prod and config server code bites again. Fixes oss-fuzz issue https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=13228. Risk level: Low Testing: Corpus entry added. Signed-off-by: Harvey Tuch <htuch@google.com> Signed-off-by: Fred Douglas <fredlas@google.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…ardown.
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.
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