From 66cf5ea6e070be3a2b81ddb519ef9ecd0022c876 Mon Sep 17 00:00:00 2001
From: Craig Tiller <ctiller@google.com>
Date: Thu, 10 Feb 2022 11:53:24 -0800
Subject: [PATCH] Fix use after free (#28840)

Previously we'd use an explicit arena->Destroy() call to free memory.
This change makes the arena a scoped pointer, and in doing so lets the
grpc_metadata_batch destructors run prior to the arena being destroyed,
preventing a use-after-free we've seen in production code.
---
 .../client_channel/health/health_check_client.cc      | 11 +++++------
 .../client_channel/health/health_check_client.h       |  2 +-
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/src/core/ext/filters/client_channel/health/health_check_client.cc b/src/core/ext/filters/client_channel/health/health_check_client.cc
index 118088fceb0..47abbcd8a99 100644
--- a/src/core/ext/filters/client_channel/health/health_check_client.cc
+++ b/src/core/ext/filters/client_channel/health/health_check_client.cc
@@ -262,10 +262,10 @@ HealthCheckClient::CallState::CallState(
                                ->GetInitialCallSizeEstimate(),
                            &health_check_client_->call_allocator_)),
       payload_(context_),
-      send_initial_metadata_(arena_),
-      send_trailing_metadata_(arena_),
-      recv_initial_metadata_(arena_),
-      recv_trailing_metadata_(arena_) {}
+      send_initial_metadata_(arena_.get()),
+      send_trailing_metadata_(arena_.get()),
+      recv_initial_metadata_(arena_.get()),
+      recv_trailing_metadata_(arena_.get()) {}
 
 HealthCheckClient::CallState::~CallState() {
   if (GRPC_TRACE_FLAG_ENABLED(grpc_health_check_client_trace)) {
@@ -282,7 +282,6 @@ HealthCheckClient::CallState::~CallState() {
   // any, so that it can release any internal references it may be
   // holding to the call stack.
   call_combiner_.SetNotifyOnCancel(nullptr);
-  arena_->Destroy();
 }
 
 void HealthCheckClient::CallState::Orphan() {
@@ -297,7 +296,7 @@ void HealthCheckClient::CallState::StartCall() {
       Slice::FromStaticString("/grpc.health.v1.Health/Watch"),
       gpr_get_cycle_counter(),  // start_time
       GRPC_MILLIS_INF_FUTURE,   // deadline
-      arena_,
+      arena_.get(),
       context_,
       &call_combiner_,
   };
diff --git a/src/core/ext/filters/client_channel/health/health_check_client.h b/src/core/ext/filters/client_channel/health/health_check_client.h
index bbf82a0e289..3b04a90f698 100644
--- a/src/core/ext/filters/client_channel/health/health_check_client.h
+++ b/src/core/ext/filters/client_channel/health/health_check_client.h
@@ -93,7 +93,7 @@ class HealthCheckClient : public InternallyRefCounted<HealthCheckClient> {
     RefCountedPtr<HealthCheckClient> health_check_client_;
     grpc_polling_entity pollent_;
 
-    Arena* arena_;
+    ScopedArenaPtr arena_;
     CallCombiner call_combiner_;
     grpc_call_context_element context_[GRPC_CONTEXT_COUNT] = {};