From cd129b49c20e6eb741e291e5c6f0db9b43125a64 Mon Sep 17 00:00:00 2001
From: "Mark D. Roth" <roth@google.com>
Date: Tue, 1 Oct 2024 17:20:13 -0700
Subject: [PATCH] [GCP auth filter] hold ref to service config (#37831)

This fixes the following crash:

https://btx.cloud.google.com/invocations/3a7065d4-db7f-4f01-a239-5376b7f5ee8b/targets/%2F%2Ftest%2Fcpp%2Fend2end%2Fxds:xds_gcp_authn_end2end_test@experiment%3Dwork_serializer_dispatch;config=1a7dc092b28796b045d00aec96c95b85c1d4dc656912e0021a1fc84b3ecb2ac9/log

The problem is caused by a race whereby the channel swaps out the service config due to a resolver update while the old dynamic filter stack is still processing calls in another thread.  The GCP auth filter was dereferencing the old service config but not holding a ref to it.  I've fixed this by having it hold a ref.

In the long run, I suspect that we may run into other cases like this, in which case we may want the dynamic filter stack itself to hold a ref to the service config, so that individual filters don't have to.

Closes #37831

COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/37831 from markdroth:gcp_auth_race_fix f2a0d1dd7921adfbb47332d50109ae612fc43240
PiperOrigin-RevId: 681221795
---
 .../gcp_authentication/gcp_authentication_filter.cc   | 11 +++++++----
 .../gcp_authentication/gcp_authentication_filter.h    |  4 ++++
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/src/core/ext/filters/gcp_authentication/gcp_authentication_filter.cc b/src/core/ext/filters/gcp_authentication/gcp_authentication_filter.cc
index 7e51b20dca399..e301ed62154e6 100644
--- a/src/core/ext/filters/gcp_authentication/gcp_authentication_filter.cc
+++ b/src/core/ext/filters/gcp_authentication/gcp_authentication_filter.cc
@@ -153,7 +153,7 @@ absl::StatusOr<std::unique_ptr<GcpAuthenticationFilter>>
 GcpAuthenticationFilter::Create(const ChannelArgs& args,
                                 ChannelFilter::Args filter_args) {
   // Get filter config.
-  auto* service_config = args.GetObject<ServiceConfig>();
+  auto service_config = args.GetObjectRef<ServiceConfig>();
   if (service_config == nullptr) {
     return absl::InvalidArgumentError(
         "gcp_auth: no service config in channel args");
@@ -184,15 +184,18 @@ GcpAuthenticationFilter::Create(const ChannelArgs& args,
   // cache but it has the wrong size.
   cache->SetMaxSize(filter_config->cache_size);
   // Instantiate filter.
-  return std::unique_ptr<GcpAuthenticationFilter>(new GcpAuthenticationFilter(
-      filter_config, std::move(xds_config), std::move(cache)));
+  return std::unique_ptr<GcpAuthenticationFilter>(
+      new GcpAuthenticationFilter(std::move(service_config), filter_config,
+                                  std::move(xds_config), std::move(cache)));
 }
 
 GcpAuthenticationFilter::GcpAuthenticationFilter(
+    RefCountedPtr<ServiceConfig> service_config,
     const GcpAuthenticationParsedConfig::Config* filter_config,
     RefCountedPtr<const XdsConfig> xds_config,
     RefCountedPtr<CallCredentialsCache> cache)
-    : filter_config_(filter_config),
+    : service_config_(std::move(service_config)),
+      filter_config_(filter_config),
       xds_config_(std::move(xds_config)),
       cache_(std::move(cache)) {}
 
diff --git a/src/core/ext/filters/gcp_authentication/gcp_authentication_filter.h b/src/core/ext/filters/gcp_authentication/gcp_authentication_filter.h
index f8fc704c9e83e..a3136ebd460db 100644
--- a/src/core/ext/filters/gcp_authentication/gcp_authentication_filter.h
+++ b/src/core/ext/filters/gcp_authentication/gcp_authentication_filter.h
@@ -80,10 +80,14 @@ class GcpAuthenticationFilter
   };
 
   GcpAuthenticationFilter(
+      RefCountedPtr<ServiceConfig> service_config,
       const GcpAuthenticationParsedConfig::Config* filter_config,
       RefCountedPtr<const XdsConfig> xds_config,
       RefCountedPtr<CallCredentialsCache> cache);
 
+  // TODO(roth): Consider having the channel stack hold this ref so that
+  // individual filters don't need to.
+  const RefCountedPtr<ServiceConfig> service_config_;
   const GcpAuthenticationParsedConfig::Config* filter_config_;
   const RefCountedPtr<const XdsConfig> xds_config_;
   const RefCountedPtr<CallCredentialsCache> cache_;