[ImproveTLS] fix memory leak issue from users' perspective

include/grpc/grpc_security.h

@@ -793,11 +793,13 @@ GRPCAPI grpc_tls_key_materials_config* grpc_tls_key_materials_config_create(
     void);
 
 /** Set grpc_tls_key_materials_config instance with provided a TLS certificate.
-    config will take the ownership of pem_root_certs and pem_key_cert_pairs.
     It's valid for the caller to provide nullptr pem_root_certs, in which case
     the gRPC-provided root cert will be used. pem_key_cert_pairs should not be
-    NULL. It returns 1 on success and 0 on failure. It is used for
-    experimental purpose for now and subject to change.
+    NULL.
+    The ownerships of |pem_root_certs| and |pem_key_cert_pairs| remain with the
+    caller.
+    It returns 1 on success and 0 on failure. It is used for experimental
+    purpose for now and subject to change.
  */
 GRPCAPI int grpc_tls_key_materials_config_set_key_materials(
     grpc_tls_key_materials_config* config, const char* pem_root_certs,
@@ -836,8 +838,10 @@ typedef void (*grpc_tls_on_credential_reload_done_cb)(
     - cb and cb_user_data represent a gRPC-provided
       callback and an argument passed to it.
     - key_materials_config is an in/output parameter containing currently
-      used/newly reloaded credentials. If credential reload does not result
-      in a new credential, key_materials_config should not be modified.
+      used/newly reloaded credentials. If credential reload does not result in
+      a new credential, key_materials_config should not be modified. The same
+      key_materials_config object can be updated if new key materials is
+      available.
     - status and error_details are used to hold information about
       errors occurred when a credential reload request is scheduled/cancelled.
     - config is a pointer to the unique grpc_tls_credential_reload_config
@@ -865,8 +869,9 @@ struct grpc_tls_credential_reload_arg {
     - schedule is a pointer to an application-provided callback used to invoke
       credential reload API. The implementation of this method has to be
       non-blocking, but can be performed synchronously or asynchronously.
-      1) If processing occurs synchronously, it populates arg->key_materials,
-      arg->status, and arg->error_details and returns zero.
+      1) If processing occurs synchronously, it populates
+      arg->key_materials_config, arg->status, and arg->error_details
+      and returns zero.
       2) If processing occurs asynchronously, it returns a non-zero value.
       The application then invokes arg->cb when processing is completed. Note
       that arg->cb cannot be invoked before schedule API returns.

src/core/lib/security/credentials/tls/grpc_tls_credentials_options.cc

@@ -29,10 +29,28 @@
 
 /** -- gRPC TLS key materials config API implementation. -- **/
 void grpc_tls_key_materials_config::set_key_materials(
-    grpc_core::UniquePtr<char> pem_root_certs,
-    PemKeyCertPairList pem_key_cert_pair_list) {
-  pem_key_cert_pair_list_ = std::move(pem_key_cert_pair_list);
-  pem_root_certs_ = std::move(pem_root_certs);
+    const char* pem_root_certs,
+    const grpc_ssl_pem_key_cert_pair** pem_key_cert_pairs,
+    size_t num_key_cert_pairs) {
+  this->set_pem_root_certs(pem_root_certs);
+  grpc_tls_key_materials_config::PemKeyCertPairList cert_pair_list;
+  for (size_t i = 0; i < num_key_cert_pairs; i++) {
+    auto current_pair = static_cast<grpc_ssl_pem_key_cert_pair*>(
+        gpr_zalloc(sizeof(grpc_ssl_pem_key_cert_pair)));
+    current_pair->cert_chain = gpr_strdup(pem_key_cert_pairs[i]->cert_chain);
+    current_pair->private_key = gpr_strdup(pem_key_cert_pairs[i]->private_key);
+    cert_pair_list.emplace_back(grpc_core::PemKeyCertPair(current_pair));
+  }
+  pem_key_cert_pair_list_ = std::move(cert_pair_list);
+}
+
+void grpc_tls_key_materials_config::set_key_materials(
+    const char* pem_root_certs,
+    const PemKeyCertPairList& pem_key_cert_pair_list) {
+  this->set_pem_root_certs(pem_root_certs);
+  grpc_tls_key_materials_config::PemKeyCertPairList dup_list(
+      pem_key_cert_pair_list);
+  pem_key_cert_pair_list_ = std::move(dup_list);
 }
 
 /** -- gRPC TLS credential reload config API implementation. -- **/
@@ -165,15 +183,7 @@ int grpc_tls_key_materials_config_set_key_materials(
             "grpc_tls_key_materials_config_set_key_materials()");
     return 0;
   }
-  grpc_core::UniquePtr<char> pem_root(const_cast<char*>(root_certs));
-  grpc_tls_key_materials_config::PemKeyCertPairList cert_pair_list;
-  for (size_t i = 0; i < num; i++) {
-    grpc_core::PemKeyCertPair key_cert_pair(
-        const_cast<grpc_ssl_pem_key_cert_pair*>(key_cert_pairs[i]));
-    cert_pair_list.emplace_back(std::move(key_cert_pair));
-  }
-  config->set_key_materials(std::move(pem_root), std::move(cert_pair_list));
-  gpr_free(key_cert_pairs);
+  config->set_key_materials(root_certs, key_cert_pairs, num);
   return 1;
 }
 

src/core/lib/security/credentials/tls/grpc_tls_credentials_options.h

@@ -42,14 +42,28 @@ struct grpc_tls_key_materials_config
   int version() const { return version_; }
 
   /** Setters for member fields. **/
+  // TODO(ZhenLian): Remove this function
   void set_pem_root_certs(grpc_core::UniquePtr<char> pem_root_certs) {
     pem_root_certs_ = std::move(pem_root_certs);
   }
+  // The ownerships of |pem_root_certs| remain with the caller.
+  void set_pem_root_certs(const char* pem_root_certs) {
+    // make a copy of pem_root_certs.
+    grpc_core::UniquePtr<char> pem_root_ptr(gpr_strdup(pem_root_certs));
+    pem_root_certs_ = std::move(pem_root_ptr);
+  }
   void add_pem_key_cert_pair(grpc_core::PemKeyCertPair pem_key_cert_pair) {
     pem_key_cert_pair_list_.push_back(pem_key_cert_pair);
   }
-  void set_key_materials(grpc_core::UniquePtr<char> pem_root_certs,
-                         PemKeyCertPairList pem_key_cert_pair_list);
+  // The ownerships of |pem_root_certs| and |pem_key_cert_pairs| remain with the
+  // caller.
+  void set_key_materials(const char* pem_root_certs,
+                         const grpc_ssl_pem_key_cert_pair** pem_key_cert_pairs,
+                         size_t num_key_cert_pairs);
+  // The ownerships of |pem_root_certs| and |pem_key_cert_pair_list| remain with
+  // the caller.
+  void set_key_materials(const char* pem_root_certs,
+                         const PemKeyCertPairList& pem_key_cert_pair_list);
   void set_version(int version) { version_ = version; }
 
  private:

src/core/lib/security/security_connector/tls/tls_security_connector.cc

@@ -346,14 +346,12 @@ grpc_security_status TlsChannelSecurityConnector::InitializeHandshakerFactory(
       static_cast<const TlsCredentials*>(channel_creds());
   grpc_tls_key_materials_config* key_materials_config =
       creds->options().key_materials_config();
-  /* Copy key materials config from credential options. */
+  // key_materials_config_->set_key_materials will handle the copying of the key
+  // materials users provided
   if (key_materials_config != nullptr) {
-    grpc_tls_key_materials_config::PemKeyCertPairList cert_pair_list =
-        key_materials_config->pem_key_cert_pair_list();
-    auto pem_root_certs = grpc_core::UniquePtr<char>(
-        gpr_strdup(key_materials_config->pem_root_certs()));
-    key_materials_config_->set_key_materials(std::move(pem_root_certs),
-                                             std::move(cert_pair_list));
+    key_materials_config_->set_key_materials(
+        key_materials_config->pem_root_certs(),
+        key_materials_config->pem_key_cert_pair_list());
   }
   grpc_ssl_certificate_config_reload_status reload_status =
       GRPC_SSL_CERTIFICATE_CONFIG_RELOAD_UNCHANGED;
@@ -557,12 +555,9 @@ grpc_security_status TlsServerSecurityConnector::InitializeHandshakerFactory() {
   grpc_tls_key_materials_config* key_materials_config =
       creds->options().key_materials_config();
   if (key_materials_config != nullptr) {
-    grpc_tls_key_materials_config::PemKeyCertPairList cert_pair_list =
-        key_materials_config->pem_key_cert_pair_list();
-    auto pem_root_certs = grpc_core::UniquePtr<char>(
-        gpr_strdup(key_materials_config->pem_root_certs()));
-    key_materials_config_->set_key_materials(std::move(pem_root_certs),
-                                             std::move(cert_pair_list));
+    key_materials_config_->set_key_materials(
+        key_materials_config->pem_root_certs(),
+        key_materials_config->pem_key_cert_pair_list());
   }
   grpc_ssl_certificate_config_reload_status reload_status =
       GRPC_SSL_CERTIFICATE_CONFIG_RELOAD_UNCHANGED;

src/cpp/common/tls_credentials_options.cc

@@ -119,9 +119,7 @@ void TlsCredentialReloadArg::set_key_materials(
         ConvertToCorePemKeyCertPair(key_cert_pair));
   }
   /** Populate the key materials config field of |c_arg_|. **/
-  ::grpc_core::UniquePtr<char> c_pem_root_certs(
-      gpr_strdup(pem_root_certs.c_str()));
-  c_arg_->key_materials_config->set_key_materials(std::move(c_pem_root_certs),
+  c_arg_->key_materials_config->set_key_materials(pem_root_certs.c_str(),
                                                   c_pem_key_cert_pair_list);
 }
 
@@ -150,7 +148,7 @@ void TlsCredentialReloadArg::set_key_materials_config(
     c_arg_->key_materials_config = grpc_tls_key_materials_config_create();
   }
   c_arg_->key_materials_config->set_key_materials(
-      std::move(c_pem_root_certs), std::move(c_pem_key_cert_pair_list));
+      key_materials_config->pem_root_certs().c_str(), c_pem_key_cert_pair_list);
   c_arg_->key_materials_config->set_version(key_materials_config->version());
 }
 

src/cpp/common/tls_credentials_options_util.cc

@@ -47,10 +47,8 @@ grpc_tls_key_materials_config* ConvertToCKeyMaterialsConfig(
         ::grpc_core::PemKeyCertPair(ssl_pair);
     c_pem_key_cert_pair_list.push_back(::std::move(c_pem_key_cert_pair));
   }
-  ::grpc_core::UniquePtr<char> c_pem_root_certs(
-      gpr_strdup(config->pem_root_certs().c_str()));
-  c_config->set_key_materials(std::move(c_pem_root_certs),
-                              std::move(c_pem_key_cert_pair_list));
+  c_config->set_key_materials(config->pem_root_certs().c_str(),
+                              c_pem_key_cert_pair_list);
   c_config->set_version(config->version());
   return c_config;
 }

test/core/end2end/fixtures/h2_tls.cc

@@ -140,17 +140,14 @@ static int client_cred_reload_sync(void* /*config_user_data*/,
     arg->status = GRPC_SSL_CERTIFICATE_CONFIG_RELOAD_UNCHANGED;
     return 0;
   }
-  grpc_ssl_pem_key_cert_pair** key_cert_pair =
-      static_cast<grpc_ssl_pem_key_cert_pair**>(
-          gpr_zalloc(sizeof(grpc_ssl_pem_key_cert_pair*)));
-  key_cert_pair[0] = static_cast<grpc_ssl_pem_key_cert_pair*>(
-      gpr_zalloc(sizeof(grpc_ssl_pem_key_cert_pair)));
-  key_cert_pair[0]->private_key = gpr_strdup(test_server1_key);
-  key_cert_pair[0]->cert_chain = gpr_strdup(test_server1_cert);
+  const grpc_ssl_pem_key_cert_pair pem_key_pair = {
+      test_server1_key,
+      test_server1_cert,
+  };
   if (arg->key_materials_config->pem_key_cert_pair_list().empty()) {
+    const auto* pem_key_pair_ptr = &pem_key_pair;
     grpc_tls_key_materials_config_set_key_materials(
-        arg->key_materials_config, gpr_strdup(test_root_cert),
-        (const grpc_ssl_pem_key_cert_pair**)key_cert_pair, 1);
+        arg->key_materials_config, test_root_cert, &pem_key_pair_ptr, 1);
   }
   // new credential has been reloaded.
   arg->status = GRPC_SSL_CERTIFICATE_CONFIG_RELOAD_NEW;
@@ -166,21 +163,18 @@ static int server_cred_reload_sync(void* /*config_user_data*/,
     arg->status = GRPC_SSL_CERTIFICATE_CONFIG_RELOAD_UNCHANGED;
     return 0;
   }
-  grpc_ssl_pem_key_cert_pair** key_cert_pair =
-      static_cast<grpc_ssl_pem_key_cert_pair**>(
-          gpr_zalloc(sizeof(grpc_ssl_pem_key_cert_pair*)));
-  key_cert_pair[0] = static_cast<grpc_ssl_pem_key_cert_pair*>(
-      gpr_zalloc(sizeof(grpc_ssl_pem_key_cert_pair)));
-  key_cert_pair[0]->private_key = gpr_strdup(test_server1_key);
-  key_cert_pair[0]->cert_chain = gpr_strdup(test_server1_cert);
+  const grpc_ssl_pem_key_cert_pair pem_key_pair = {
+      test_server1_key,
+      test_server1_cert,
+  };
   GPR_ASSERT(arg != nullptr);
   GPR_ASSERT(arg->key_materials_config != nullptr);
   GPR_ASSERT(arg->key_materials_config->pem_key_cert_pair_list().data() !=
              nullptr);
   if (arg->key_materials_config->pem_key_cert_pair_list().empty()) {
+    const auto* pem_key_pair_ptr = &pem_key_pair;
     grpc_tls_key_materials_config_set_key_materials(
-        arg->key_materials_config, gpr_strdup(test_root_cert),
-        (const grpc_ssl_pem_key_cert_pair**)key_cert_pair, 1);
+        arg->key_materials_config, test_root_cert, &pem_key_pair_ptr, 1);
   }
   // new credential has been reloaded.
   arg->status = GRPC_SSL_CERTIFICATE_CONFIG_RELOAD_NEW;

test/core/security/grpc_tls_credentials_options_test.cc

@@ -28,16 +28,13 @@
 namespace testing {
 
 static void SetKeyMaterials(grpc_tls_key_materials_config* config) {
-  grpc_ssl_pem_key_cert_pair** key_cert_pair =
-      static_cast<grpc_ssl_pem_key_cert_pair**>(
-          gpr_zalloc(sizeof(grpc_ssl_pem_key_cert_pair*)));
-  key_cert_pair[0] = static_cast<grpc_ssl_pem_key_cert_pair*>(
-      gpr_zalloc(sizeof(grpc_ssl_pem_key_cert_pair)));
-  key_cert_pair[0]->private_key = gpr_strdup(test_server1_key);
-  key_cert_pair[0]->cert_chain = gpr_strdup(test_server1_cert);
-  grpc_tls_key_materials_config_set_key_materials(
-      config, gpr_strdup(test_root_cert),
-      (const grpc_ssl_pem_key_cert_pair**)key_cert_pair, 1);
+  const grpc_ssl_pem_key_cert_pair pem_key_pair = {
+      test_server1_key,
+      test_server1_cert,
+  };
+  const auto* pem_key_pair_ptr = &pem_key_pair;
+  grpc_tls_key_materials_config_set_key_materials(config, test_root_cert,
+                                                  &pem_key_pair_ptr, 1);
 }
 
 TEST(GrpcTlsCredentialsOptionsTest, SetKeyMaterials) {

test/core/security/tls_security_connector_test.cc

@@ -35,16 +35,13 @@ namespace {
 enum CredReloadResult { FAIL, SUCCESS, UNCHANGED, ASYNC };
 
 void SetKeyMaterials(grpc_tls_key_materials_config* config) {
-  grpc_ssl_pem_key_cert_pair** key_cert_pair =
-      static_cast<grpc_ssl_pem_key_cert_pair**>(
-          gpr_zalloc(sizeof(grpc_ssl_pem_key_cert_pair*)));
-  key_cert_pair[0] = static_cast<grpc_ssl_pem_key_cert_pair*>(
-      gpr_zalloc(sizeof(grpc_ssl_pem_key_cert_pair)));
-  key_cert_pair[0]->private_key = gpr_strdup(test_server1_key);
-  key_cert_pair[0]->cert_chain = gpr_strdup(test_server1_cert);
-  grpc_tls_key_materials_config_set_key_materials(
-      config, gpr_strdup(test_root_cert),
-      (const grpc_ssl_pem_key_cert_pair**)key_cert_pair, 1);
+  const grpc_ssl_pem_key_cert_pair pem_key_pair = {
+      test_server1_key,
+      test_server1_cert,
+  };
+  const auto* pem_key_pair_ptr = &pem_key_pair;
+  grpc_tls_key_materials_config_set_key_materials(config, test_root_cert,
+                                                  &pem_key_pair_ptr, 1);
 }
 
 int CredReloadSuccess(void* /*config_user_data*/,

test/cpp/client/credentials_test.cc

@@ -402,9 +402,8 @@ TEST_F(CredentialsTest, TlsCredentialReloadConfigCppToC) {
       pem_key_cert_pair_list;
   pem_key_cert_pair_list.push_back(pem_key_cert_pair);
   grpc::string test_pem_root_certs = "pem_root_certs";
-  c_key_materials.set_key_materials(
-      ::grpc_core::UniquePtr<char>(gpr_strdup(test_pem_root_certs.c_str())),
-      pem_key_cert_pair_list);
+  c_key_materials.set_key_materials(test_pem_root_certs.c_str(),
+                                    pem_key_cert_pair_list);
   c_arg.key_materials_config = &c_key_materials;
   c_arg.status = GRPC_SSL_CERTIFICATE_CONFIG_RELOAD_UNCHANGED;
   grpc::string test_error_details = "error_details";