config_validation/server: fix initialization order redux. (envoyproxy…

…#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>

source/server/config_validation/server.h

@@ -139,6 +139,16 @@ class ValidationInstance : Logger::Loggable<Logger::Id::main>,
   void initialize(const Options& options, Network::Address::InstanceConstSharedPtr local_address,
                   ComponentFactory& component_factory);
 
+  // init_manager_ must come before any member that participates in initialization, and destructed
+  // only after referencing members are gone, since initialization continuation can potentially
+  // occur at any point during member lifetime.
+  InitManagerImpl init_manager_;
+  // secret_manager_ must come before listener_manager_, config_ and dispatcher_, and destructed
+  // only after these members can no longer reference it, since:
+  // - There may be active filter chains referencing it in listener_manager_.
+  // - There may be active clusters referencing it in config_.cluster_manager_.
+  // - There may be active connections referencing it.
+  std::unique_ptr<Secret::SecretManager> secret_manager_;
   const Options& options_;
   Stats::IsolatedStoreImpl& stats_store_;
   ThreadLocal::InstanceImpl thread_local_;
@@ -153,10 +163,6 @@ class ValidationInstance : Logger::Loggable<Logger::Id::main>,
   LocalInfo::LocalInfoPtr local_info_;
   AccessLog::AccessLogManagerImpl access_log_manager_;
   std::unique_ptr<Upstream::ValidationClusterManagerFactory> cluster_manager_factory_;
-  InitManagerImpl init_manager_;
-  // secret_manager_ must come before listener_manager_, since there may be active filter chains
-  // referencing it, so need to destruct these first.
-  std::unique_ptr<Secret::SecretManager> secret_manager_;
   std::unique_ptr<ListenerManagerImpl> listener_manager_;
   std::unique_ptr<OverloadManager> overload_manager_;
   MutexTracer* mutex_tracer_;

source/server/server.cc

@@ -52,11 +52,11 @@ InstanceImpl::InstanceImpl(const Options& options, Event::TimeSystem& time_syste
                            ComponentFactory& component_factory,
                            Runtime::RandomGeneratorPtr&& random_generator,
                            ThreadLocal::Instance& tls, Thread::ThreadFactory& thread_factory)
-    : shutdown_(false), options_(options), time_source_(time_system), restarter_(restarter),
+    : secret_manager_(std::make_unique<Secret::SecretManagerImpl>()), shutdown_(false),
+      options_(options), time_source_(time_system), restarter_(restarter),
       start_time_(time(nullptr)), original_start_time_(start_time_), stats_store_(store),
       thread_local_(tls),
       api_(new Api::Impl(options.fileFlushIntervalMsec(), thread_factory, store, time_system)),
-      secret_manager_(std::make_unique<Secret::SecretManagerImpl>()),
       dispatcher_(api_->allocateDispatcher()),
       singleton_manager_(new Singleton::ManagerImpl(api_->threadFactory().currentThreadId())),
       handler_(new ConnectionHandlerImpl(ENVOY_LOGGER(), *dispatcher_)),

source/server/server.h

@@ -200,6 +200,16 @@ class InstanceImpl : Logger::Loggable<Logger::Id::main>, public Instance {
   void startWorkers();
   void terminate();
 
+  // init_manager_ must come before any member that participates in initialization, and destructed
+  // only after referencing members are gone, since initialization continuation can potentially
+  // occur at any point during member lifetime.
+  InitManagerImpl init_manager_;
+  // secret_manager_ must come before listener_manager_, config_ and dispatcher_, and destructed
+  // only after these members can no longer reference it, since:
+  // - There may be active filter chains referencing it in listener_manager_.
+  // - There may be active clusters referencing it in config_.cluster_manager_.
+  // - There may be active connections referencing it.
+  std::unique_ptr<Secret::SecretManager> secret_manager_;
   bool shutdown_;
   const Options& options_;
   TimeSource& time_source_;
@@ -211,9 +221,6 @@ class InstanceImpl : Logger::Loggable<Logger::Id::main>, public Instance {
   Assert::ActionRegistrationPtr assert_action_registration_;
   ThreadLocal::Instance& thread_local_;
   Api::ApiPtr api_;
-  // secret_manager_ must come before dispatcher_, since there may be active connections
-  // referencing it, so need to destruct these first.
-  std::unique_ptr<Secret::SecretManager> secret_manager_;
   Event::DispatcherPtr dispatcher_;
   std::unique_ptr<AdminImpl> admin_;
   Singleton::ManagerPtr singleton_manager_;
@@ -231,7 +238,6 @@ class InstanceImpl : Logger::Loggable<Logger::Id::main>, public Instance {
   DrainManagerPtr drain_manager_;
   AccessLog::AccessLogManagerImpl access_log_manager_;
   std::unique_ptr<Upstream::ClusterManagerFactory> cluster_manager_factory_;
-  InitManagerImpl init_manager_;
   std::unique_ptr<Server::GuardDog> guard_dog_;
   bool terminated_;
   std::unique_ptr<Logger::FileSinkDelegate> file_logger_;