Fix memory leak in HTTP request security handshake cancellation (grpc…

…#28971) * Fix memory leak in HTTP request security handshake cancellation
zkkxu · Mar 1, 2022 · 20521fb · 20521fb
1 parent 4633121
commit 20521fb
Show file tree

Hide file tree

Showing 3 changed files with 64 additions and 1 deletion.
diff --git a/src/core/lib/http/httpcli.cc b/src/core/lib/http/httpcli.cc
@@ -53,6 +53,7 @@ namespace {
 
 grpc_httpcli_get_override g_get_override;
 grpc_httpcli_post_override g_post_override;
+void (*g_test_only_on_handshake_done_intercept)(HttpRequest* req);
 
 }  // namespace
 
@@ -112,6 +113,11 @@ void HttpRequest::SetOverride(grpc_httpcli_get_override get,
   g_post_override = post;
 }
 
+void HttpRequest::TestOnlySetOnHandshakeDoneIntercept(
+    void (*intercept)(HttpRequest* req)) {
+  g_test_only_on_handshake_done_intercept = intercept;
+}
+
 HttpRequest::HttpRequest(
     URI uri, const grpc_slice& request_text, grpc_http_response* response,
     Timestamp deadline, const grpc_channel_args* channel_args,
@@ -260,18 +266,29 @@ void HttpRequest::StartWrite() {
 void HttpRequest::OnHandshakeDone(void* arg, grpc_error_handle error) {
   auto* args = static_cast<HandshakerArgs*>(arg);
   RefCountedPtr<HttpRequest> req(static_cast<HttpRequest*>(args->user_data));
+  if (g_test_only_on_handshake_done_intercept != nullptr) {
+    // Run this testing intercept before the lock so that it has a chance to
+    // do things like calling Orphan on the request
+    g_test_only_on_handshake_done_intercept(req.get());
+  }
   MutexLock lock(&req->mu_);
   req->own_endpoint_ = true;
-  if (error != GRPC_ERROR_NONE || req->cancelled_) {
+  if (error != GRPC_ERROR_NONE) {
     gpr_log(GPR_ERROR, "Secure transport setup failed: %s",
             grpc_error_std_string(error).c_str());
     req->NextAddress(GRPC_ERROR_REF(error));
     return;
   }
+  // Handshake completed, so we own fields in args
   grpc_channel_args_destroy(args->args);
   grpc_slice_buffer_destroy_internal(args->read_buffer);
   gpr_free(args->read_buffer);
   req->ep_ = args->endpoint;
+  if (req->cancelled_) {
+    req->NextAddress(GRPC_ERROR_CREATE_FROM_STATIC_STRING(
+        "HTTP request cancelled during security handshake"));
+    return;
+  }
   req->StartWrite();
 }
 

diff --git a/src/core/lib/http/httpcli.h b/src/core/lib/http/httpcli.h
@@ -128,6 +128,9 @@ class HttpRequest : public InternallyRefCounted<HttpRequest> {
   static void SetOverride(grpc_httpcli_get_override get,
                           grpc_httpcli_post_override post);
 
+  static void TestOnlySetOnHandshakeDoneIntercept(
+      void (*intercept)(HttpRequest* req));
+
  private:
   void Finish(grpc_error_handle error) ABSL_EXCLUSIVE_LOCKS_REQUIRED(mu_) {
     grpc_polling_entity_del_from_pollset_set(pollent_, pollset_set_);

diff --git a/test/core/http/httpcli_test.cc b/test/core/http/httpcli_test.cc
@@ -465,6 +465,49 @@ TEST_F(HttpRequestTest, CallerPollentsAreNotReferencedAfterCallbackIsRan) {
   exec_ctx.Flush();
 }
 
+void CancelRequest(grpc_core::HttpRequest* req) {
+  gpr_log(
+      GPR_INFO,
+      "test only HttpRequest::OnHandshakeDone intercept orphaning request: %p",
+      req);
+  req->Orphan();
+}
+
+// This exercises the code paths that happen when we cancel an HTTP request
+// before the security handshake callback runs, but after that callback has
+// already been scheduled with a success result. This case is interesting
+// because the current security handshake API transfers ownership of output
+// arguments to the caller only if the handshake is successful, rendering
+// this code path as something that only occurs with just the right timing.
+TEST_F(HttpRequestTest,
+       CancelDuringSecurityHandshakeButHandshakeStillSucceeds) {
+  RequestState request_state(this);
+  grpc_http_request req;
+  grpc_core::ExecCtx exec_ctx;
+  std::string host = absl::StrFormat("localhost:%d", g_server_port);
+  gpr_log(GPR_INFO, "requesting from %s", host.c_str());
+  memset(&req, 0, sizeof(req));
+  auto uri = grpc_core::URI::Create("http", host, "/get", {} /* query params */,
+                                    "" /* fragment */);
+  GPR_ASSERT(uri.ok());
+  grpc_core::OrphanablePtr<grpc_core::HttpRequest> http_request =
+      grpc_core::HttpRequest::Get(
+          std::move(*uri), nullptr /* channel args */, pops(), &req,
+          NSecondsTime(15),
+          GRPC_CLOSURE_CREATE(OnFinishExpectFailure, &request_state,
+                              grpc_schedule_on_exec_ctx),
+          &request_state.response,
+          grpc_core::RefCountedPtr<grpc_channel_credentials>(
+              grpc_insecure_credentials_create()));
+  grpc_core::HttpRequest::TestOnlySetOnHandshakeDoneIntercept(CancelRequest);
+  http_request->Start();
+  (void)http_request.release();  // request will be orphaned by CancelRequest
+  exec_ctx.Flush();
+  PollUntil([&request_state]() { return request_state.done; },
+            AbslDeadlineSeconds(60));
+  grpc_core::HttpRequest::TestOnlySetOnHandshakeDoneIntercept(nullptr);
+}
+
 }  // namespace
 
 int main(int argc, char** argv) {