From 5ac68916dfb6c8ab1a773a911bd64e958f71b913 Mon Sep 17 00:00:00 2001
From: "Mark D. Roth" <roth@google.com>
Date: Tue, 26 Jul 2022 07:42:08 -0700
Subject: [PATCH] subchannel list: fix ubsan error (#30393)

* don't expose vector type

* don't down-cast from inside base class ctor
---
 .../lb_policy/pick_first/pick_first.cc        |  1 +
 .../lb_policy/ring_hash/ring_hash.cc          |  1 +
 .../lb_policy/round_robin/round_robin.cc      |  1 +
 .../lb_policy/subchannel_list.h               | 22 ++++++++++++-------
 4 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc b/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc
index 923d8ee0f49..43c90ec12f1 100644
--- a/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc
+++ b/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc
@@ -229,6 +229,7 @@ void PickFirst::AttemptToConnectUsingLatestUpdateArgsLocked() {
   }
   latest_pending_subchannel_list_ = MakeOrphanable<PickFirstSubchannelList>(
       this, std::move(addresses), latest_update_args_.args);
+  latest_pending_subchannel_list_->StartWatchingLocked();
   // Empty update or no valid subchannels.  Put the channel in
   // TRANSIENT_FAILURE.
   if (latest_pending_subchannel_list_->num_subchannels() == 0) {
diff --git a/src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.cc b/src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.cc
index 7976956db98..d6c73eedfe2 100644
--- a/src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.cc
+++ b/src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.cc
@@ -838,6 +838,7 @@ void RingHash::UpdateLocked(UpdateArgs args) {
   }
   latest_pending_subchannel_list_ = MakeOrphanable<RingHashSubchannelList>(
       this, std::move(addresses), args.args);
+  latest_pending_subchannel_list_->StartWatchingLocked();
   // If we have no existing list or the new list is empty, immediately
   // promote the new list.
   // Otherwise, do nothing; the new list will be promoted when the
diff --git a/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc b/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc
index da960fb6b95..3473ce9e848 100644
--- a/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc
+++ b/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc
@@ -290,6 +290,7 @@ void RoundRobin::UpdateLocked(UpdateArgs args) {
   }
   latest_pending_subchannel_list_ = MakeOrphanable<RoundRobinSubchannelList>(
       this, std::move(addresses), args.args);
+  latest_pending_subchannel_list_->StartWatchingLocked();
   // If the new list is empty, immediately promote it to
   // subchannel_list_ and report TRANSIENT_FAILURE.
   if (latest_pending_subchannel_list_->num_subchannels() == 0) {
diff --git a/src/core/ext/filters/client_channel/lb_policy/subchannel_list.h b/src/core/ext/filters/client_channel/lb_policy/subchannel_list.h
index a7453b999a6..2a5fe8f752b 100644
--- a/src/core/ext/filters/client_channel/lb_policy/subchannel_list.h
+++ b/src/core/ext/filters/client_channel/lb_policy/subchannel_list.h
@@ -178,9 +178,9 @@ class SubchannelData {
 template <typename SubchannelListType, typename SubchannelDataType>
 class SubchannelList : public InternallyRefCounted<SubchannelListType> {
  public:
-  // We use ManualConstructor here to support SubchannelDataType classes
-  // that are not copyable.
-  using SubchannelVector = std::vector<ManualConstructor<SubchannelDataType>>;
+  // Starts watching the connectivity state of all subchannels.
+  // Must be called immediately after instantiation.
+  void StartWatchingLocked();
 
   // The number of subchannels in the list.
   size_t num_subchannels() const { return subchannels_.size(); }
@@ -225,7 +225,9 @@ class SubchannelList : public InternallyRefCounted<SubchannelListType> {
   const char* tracer_;
 
   // The list of subchannels.
-  SubchannelVector subchannels_;
+  // We use ManualConstructor here to support SubchannelDataType classes
+  // that are not copyable.
+  std::vector<ManualConstructor<SubchannelDataType>> subchannels_;
 
   // Is this list shutting down? This may be true due to the shutdown of the
   // policy itself or because a newer update has arrived while this one hadn't
@@ -395,10 +397,6 @@ SubchannelList<SubchannelListType, SubchannelDataType>::SubchannelList(
     subchannels_.emplace_back();
     subchannels_.back().Init(this, std::move(address), std::move(subchannel));
   }
-  // Start watching subchannel connectivity state.
-  for (auto& sd : subchannels_) {
-    sd->StartConnectivityWatchLocked();
-  }
 }
 
 template <typename SubchannelListType, typename SubchannelDataType>
@@ -412,6 +410,14 @@ SubchannelList<SubchannelListType, SubchannelDataType>::~SubchannelList() {
   }
 }
 
+template <typename SubchannelListType, typename SubchannelDataType>
+void SubchannelList<SubchannelListType,
+                    SubchannelDataType>::StartWatchingLocked() {
+  for (auto& sd : subchannels_) {
+    sd->StartConnectivityWatchLocked();
+  }
+}
+
 template <typename SubchannelListType, typename SubchannelDataType>
 void SubchannelList<SubchannelListType, SubchannelDataType>::ShutdownLocked() {
   if (GPR_UNLIKELY(tracer_ != nullptr)) {