[tls] Fix ownership bugs in TlsCredentialsOptions and grpc_tls_credentials_options. (#34758)

Currently it is very easy to use the `TlsCredentialsOptions` in such a
way that it produces a memory leak. For example, the code block
```
{
  TlsCredentialsOptions options;
}
```
produces a memory leak. This PR fixes up the ownership bugs in this
class and its `grpc_tls_credentials_options`, the C-core analogue.

grpc.def

@@ -157,6 +157,8 @@ EXPORTS
     grpc_tls_certificate_provider_file_watcher_create
     grpc_tls_certificate_provider_release
     grpc_tls_credentials_options_create
+    grpc_tls_credentials_options_copy
+    grpc_tls_credentials_options_destroy
     grpc_tls_credentials_options_set_certificate_provider
     grpc_tls_credentials_options_watch_root_certs
     grpc_tls_credentials_options_set_root_cert_name

include/grpc/grpc_security.h

@@ -815,6 +815,22 @@ GRPCAPI void grpc_tls_certificate_provider_release(
  */
 GRPCAPI grpc_tls_credentials_options* grpc_tls_credentials_options_create(void);
 
+/**
+ * EXPERIMENTAL API - Subject to change
+ *
+ * Copies a grpc_tls_credentials_options.
+ */
+GRPCAPI grpc_tls_credentials_options* grpc_tls_credentials_options_copy(
+    grpc_tls_credentials_options* options);
+
+/**
+ * EXPERIMENTAL API - Subject to change
+ *
+ * Destroys a grpc_tls_credentials_options.
+ */
+GRPCAPI void grpc_tls_credentials_options_destroy(
+    grpc_tls_credentials_options* options);
+
 /**
  * EXPERIMENTAL API - Subject to change
  *

include/grpcpp/security/tls_credentials_options.h

@@ -44,6 +44,7 @@ class TlsCredentialsOptions {
   // @param certificate_provider the provider which fetches TLS credentials that
   // will be used in the TLS handshake
   TlsCredentialsOptions();
+  ~TlsCredentialsOptions();
   // ---- Setters for member fields ----
   // Sets the certificate provider used to store root certs and identity certs.
   void set_certificate_provider(
@@ -108,8 +109,14 @@ class TlsCredentialsOptions {
   void set_crl_provider(std::shared_ptr<CrlProvider> crl_provider);
 
   // ----- Getters for member fields ----
-  // Get the internal c options. This function shall be used only internally.
+  // Returns a deep copy of the internal c options. The caller takes ownership
-  grpc_tls_credentials_options* c_credentials_options() const {
+  // of the returned pointer. This function shall be used only internally.
+  grpc_tls_credentials_options* c_credentials_options() const;
+
+ protected:
+  // Returns the internal c options. The caller does not take ownership of the
+  // returned pointer.
+  grpc_tls_credentials_options* mutable_c_credentials_options() {
     return c_credentials_options_;
   }
 

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

@@ -38,6 +38,17 @@ grpc_tls_credentials_options* grpc_tls_credentials_options_create() {
   return new grpc_tls_credentials_options();
 }
 
+grpc_tls_credentials_options* grpc_tls_credentials_options_copy(
+    grpc_tls_credentials_options* options) {
+  GPR_ASSERT(options != nullptr);
+  return new grpc_tls_credentials_options(*options);
+}
+
+void grpc_tls_credentials_options_destroy(
+    grpc_tls_credentials_options* options) {
+  delete options;
+}
+
 void grpc_tls_credentials_options_set_cert_request_type(
     grpc_tls_credentials_options* options,
     grpc_ssl_client_certificate_request_type type) {
@@ -136,5 +147,5 @@ void grpc_tls_credentials_options_set_crl_provider(
     grpc_tls_credentials_options* options,
     std::shared_ptr<grpc_core::experimental::CrlProvider> provider) {
   GPR_ASSERT(options != nullptr);
-  options->set_crl_provider(std::move(provider));
+  options->set_crl_provider(provider);
 }

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

@@ -39,6 +39,7 @@
 struct grpc_tls_credentials_options
     : public grpc_core::RefCounted<grpc_tls_credentials_options> {
  public:
+  grpc_tls_credentials_options() = default;
   ~grpc_tls_credentials_options() override = default;
 
   // Getters for member fields.
@@ -105,6 +106,23 @@ struct grpc_tls_credentials_options
       send_client_ca_list_ == other.send_client_ca_list_;
   }
 
+  grpc_tls_credentials_options(grpc_tls_credentials_options& other) :
+      cert_request_type_(other.cert_request_type_),
+      verify_server_cert_(other.verify_server_cert_),
+      min_tls_version_(other.min_tls_version_),
+      max_tls_version_(other.max_tls_version_),
+      certificate_verifier_(other.certificate_verifier_),
+      check_call_host_(other.check_call_host_),
+      certificate_provider_(other.certificate_provider_),
+      watch_root_cert_(other.watch_root_cert_),
+      root_cert_name_(other.root_cert_name_),
+      watch_identity_pair_(other.watch_identity_pair_),
+      identity_cert_name_(other.identity_cert_name_),
+      tls_session_key_log_file_path_(other.tls_session_key_log_file_path_),
+      crl_directory_(other.crl_directory_),
+      crl_provider_(other.crl_provider_),
+      send_client_ca_list_(other.send_client_ca_list_)  {}
+
  private:
   grpc_ssl_client_certificate_request_type cert_request_type_ = GRPC_SSL_DONT_REQUEST_CLIENT_CERTIFICATE;
   bool verify_server_cert_ = true;

src/cpp/common/tls_credentials_options.cc

@@ -18,7 +18,6 @@
 
 #include <memory>
 #include <string>
-#include <utility>
 
 #include <grpc/grpc_crl_provider.h>
 #include <grpc/grpc_security.h>
@@ -36,9 +35,13 @@ TlsCredentialsOptions::TlsCredentialsOptions() {
   c_credentials_options_ = grpc_tls_credentials_options_create();
 }
 
+TlsCredentialsOptions::~TlsCredentialsOptions() {
+  grpc_tls_credentials_options_destroy(c_credentials_options_);
+}
+
 void TlsCredentialsOptions::set_certificate_provider(
     std::shared_ptr<CertificateProviderInterface> certificate_provider) {
-  certificate_provider_ = std::move(certificate_provider);
+  certificate_provider_ = certificate_provider;
   if (certificate_provider_ != nullptr) {
     grpc_tls_credentials_options_set_certificate_provider(
         c_credentials_options_, certificate_provider_->c_provider());
@@ -48,7 +51,7 @@ void TlsCredentialsOptions::set_certificate_provider(
 void TlsCredentialsOptions::set_crl_provider(
     std::shared_ptr<CrlProvider> crl_provider) {
   grpc_tls_credentials_options_set_crl_provider(c_credentials_options_,
-                                                std::move(crl_provider));
+                                                crl_provider);
 }
 
 void TlsCredentialsOptions::watch_root_certs() {
@@ -85,22 +88,27 @@ void TlsCredentialsOptions::set_tls_session_key_log_file_path(
 
 void TlsCredentialsOptions::set_certificate_verifier(
     std::shared_ptr<CertificateVerifier> certificate_verifier) {
-  certificate_verifier_ = std::move(certificate_verifier);
+  certificate_verifier_ = certificate_verifier;
   if (certificate_verifier_ != nullptr) {
     grpc_tls_credentials_options_set_certificate_verifier(
         c_credentials_options_, certificate_verifier_->c_verifier());
   }
 }
 
+grpc_tls_credentials_options* TlsCredentialsOptions::c_credentials_options()
+    const {
+  return grpc_tls_credentials_options_copy(c_credentials_options_);
+}
+
 void TlsCredentialsOptions::set_check_call_host(bool check_call_host) {
-  grpc_tls_credentials_options* options = c_credentials_options();
+  grpc_tls_credentials_options* options = mutable_c_credentials_options();
   GPR_ASSERT(options != nullptr);
   grpc_tls_credentials_options_set_check_call_host(options, check_call_host);
 }
 
 void TlsChannelCredentialsOptions::set_verify_server_certs(
     bool verify_server_certs) {
-  grpc_tls_credentials_options* options = c_credentials_options();
+  grpc_tls_credentials_options* options = mutable_c_credentials_options();
   GPR_ASSERT(options != nullptr);
   grpc_tls_credentials_options_set_verify_server_cert(options,
                                                       verify_server_certs);
@@ -108,7 +116,7 @@ void TlsChannelCredentialsOptions::set_verify_server_certs(
 
 void TlsServerCredentialsOptions::set_cert_request_type(
     grpc_ssl_client_certificate_request_type cert_request_type) {
-  grpc_tls_credentials_options* options = c_credentials_options();
+  grpc_tls_credentials_options* options = mutable_c_credentials_options();
   GPR_ASSERT(options != nullptr);
   grpc_tls_credentials_options_set_cert_request_type(options,
                                                      cert_request_type);
@@ -116,7 +124,7 @@ void TlsServerCredentialsOptions::set_cert_request_type(
 
 void TlsServerCredentialsOptions::set_send_client_ca_list(
     bool send_client_ca_list) {
-  grpc_tls_credentials_options* options = c_credentials_options();
+  grpc_tls_credentials_options* options = mutable_c_credentials_options();
   GPR_ASSERT(options != nullptr);
   grpc_tls_credentials_options_set_send_client_ca_list(options,
                                                        send_client_ca_list);

src/ruby/ext/grpc/rb_grpc_imports.generated.c

@@ -180,6 +180,8 @@ grpc_tls_certificate_provider_static_data_create_type grpc_tls_certificate_provi
 grpc_tls_certificate_provider_file_watcher_create_type grpc_tls_certificate_provider_file_watcher_create_import;
 grpc_tls_certificate_provider_release_type grpc_tls_certificate_provider_release_import;
 grpc_tls_credentials_options_create_type grpc_tls_credentials_options_create_import;
+grpc_tls_credentials_options_copy_type grpc_tls_credentials_options_copy_import;
+grpc_tls_credentials_options_destroy_type grpc_tls_credentials_options_destroy_import;
 grpc_tls_credentials_options_set_certificate_provider_type grpc_tls_credentials_options_set_certificate_provider_import;
 grpc_tls_credentials_options_watch_root_certs_type grpc_tls_credentials_options_watch_root_certs_import;
 grpc_tls_credentials_options_set_root_cert_name_type grpc_tls_credentials_options_set_root_cert_name_import;
@@ -467,6 +469,8 @@ void grpc_rb_load_imports(HMODULE library) {
   grpc_tls_certificate_provider_file_watcher_create_import = (grpc_tls_certificate_provider_file_watcher_create_type) GetProcAddress(library, "grpc_tls_certificate_provider_file_watcher_create");
   grpc_tls_certificate_provider_release_import = (grpc_tls_certificate_provider_release_type) GetProcAddress(library, "grpc_tls_certificate_provider_release");
   grpc_tls_credentials_options_create_import = (grpc_tls_credentials_options_create_type) GetProcAddress(library, "grpc_tls_credentials_options_create");
+  grpc_tls_credentials_options_copy_import = (grpc_tls_credentials_options_copy_type) GetProcAddress(library, "grpc_tls_credentials_options_copy");
+  grpc_tls_credentials_options_destroy_import = (grpc_tls_credentials_options_destroy_type) GetProcAddress(library, "grpc_tls_credentials_options_destroy");
   grpc_tls_credentials_options_set_certificate_provider_import = (grpc_tls_credentials_options_set_certificate_provider_type) GetProcAddress(library, "grpc_tls_credentials_options_set_certificate_provider");
   grpc_tls_credentials_options_watch_root_certs_import = (grpc_tls_credentials_options_watch_root_certs_type) GetProcAddress(library, "grpc_tls_credentials_options_watch_root_certs");
   grpc_tls_credentials_options_set_root_cert_name_import = (grpc_tls_credentials_options_set_root_cert_name_type) GetProcAddress(library, "grpc_tls_credentials_options_set_root_cert_name");

src/ruby/ext/grpc/rb_grpc_imports.generated.h

@@ -515,6 +515,12 @@ extern grpc_tls_certificate_provider_release_type grpc_tls_certificate_provider_
 typedef grpc_tls_credentials_options*(*grpc_tls_credentials_options_create_type)(void);
 extern grpc_tls_credentials_options_create_type grpc_tls_credentials_options_create_import;
 #define grpc_tls_credentials_options_create grpc_tls_credentials_options_create_import
+typedef grpc_tls_credentials_options*(*grpc_tls_credentials_options_copy_type)(grpc_tls_credentials_options* options);
+extern grpc_tls_credentials_options_copy_type grpc_tls_credentials_options_copy_import;
+#define grpc_tls_credentials_options_copy grpc_tls_credentials_options_copy_import
+typedef void(*grpc_tls_credentials_options_destroy_type)(grpc_tls_credentials_options* options);
+extern grpc_tls_credentials_options_destroy_type grpc_tls_credentials_options_destroy_import;
+#define grpc_tls_credentials_options_destroy grpc_tls_credentials_options_destroy_import
 typedef void(*grpc_tls_credentials_options_set_certificate_provider_type)(grpc_tls_credentials_options* options, grpc_tls_certificate_provider* provider);
 extern grpc_tls_credentials_options_set_certificate_provider_type grpc_tls_credentials_options_set_certificate_provider_import;
 #define grpc_tls_credentials_options_set_certificate_provider grpc_tls_credentials_options_set_certificate_provider_import

test/cpp/client/credentials_test.cc

@@ -47,10 +47,14 @@ constexpr const char* kIdentityCertName = "identity_cert_name";
 constexpr const char* kIdentityCertPrivateKey = "identity_private_key";
 constexpr const char* kIdentityCertContents = "identity_cert_contents";
 
+using ::grpc::experimental::CreateStaticCrlProvider;
 using ::grpc::experimental::ExternalCertificateVerifier;
 using ::grpc::experimental::FileWatcherCertificateProvider;
 using ::grpc::experimental::HostNameCertificateVerifier;
+using ::grpc::experimental::NoOpCertificateVerifier;
 using ::grpc::experimental::StaticDataCertificateProvider;
+using ::grpc::experimental::TlsChannelCredentialsOptions;
+using ::grpc::experimental::TlsCredentialsOptions;
 
 }  // namespace
 
@@ -419,8 +423,82 @@ TEST(CredentialsTest, TlsChannelCredentialsWithCrlProviderAndDirectory) {
   GPR_ASSERT(channel_credentials.get() != nullptr);
 }
 
-// TODO(gtcooke94) - Add test to make sure Tls*CredentialsOptions does not leak
+TEST(CredentialsTest, TlsCredentialsOptionsDoesNotLeak) {
-// when not moved into TlsCredentials
+  TlsCredentialsOptions options;
+  (void)options;
+}
+
+TEST(CredentialsTest, MultipleOptionsOneCertificateProviderDoesNotLeak) {
+  auto provider = std::make_shared<StaticDataCertificateProvider>("root-pem");
+  TlsCredentialsOptions options_1;
+  options_1.set_certificate_provider(provider);
+  TlsCredentialsOptions options_2;
+  options_2.set_certificate_provider(provider);
+}
+
+TEST(CredentialsTest, MultipleOptionsOneCertificateVerifierDoesNotLeak) {
+  auto verifier = std::make_shared<NoOpCertificateVerifier>();
+  TlsCredentialsOptions options_1;
+  options_1.set_certificate_verifier(verifier);
+  TlsCredentialsOptions options_2;
+  options_2.set_certificate_verifier(verifier);
+}
+
+TEST(CredentialsTest, MultipleOptionsOneCrlProviderDoesNotLeak) {
+  auto crl_provider = CreateStaticCrlProvider(/*crls=*/{});
+  EXPECT_TRUE(crl_provider.ok());
+  TlsCredentialsOptions options_1;
+  options_1.set_crl_provider(*crl_provider);
+  TlsCredentialsOptions options_2;
+  options_2.set_crl_provider(*crl_provider);
+}
+
+TEST(CredentialsTest, TlsChannelCredentialsDoesNotLeak) {
+  TlsChannelCredentialsOptions options;
+  auto channel_creds = TlsCredentials(options);
+  EXPECT_NE(channel_creds, nullptr);
+}
+
+TEST(CredentialsTest, MultipleChannelCredentialsSameOptionsDoesNotLeak) {
+  TlsChannelCredentialsOptions options;
+  auto channel_creds_1 = TlsCredentials(options);
+  EXPECT_NE(channel_creds_1, nullptr);
+  auto channel_creds_2 = TlsCredentials(options);
+  EXPECT_NE(channel_creds_2, nullptr);
+}
+
+TEST(CredentialsTest,
+     MultipleChannelCredentialsOneCertificateProviderDoesNotLeak) {
+  TlsChannelCredentialsOptions options;
+  auto provider = std::make_shared<StaticDataCertificateProvider>("root-pem");
+  options.set_certificate_provider(provider);
+  auto channel_creds_1 = TlsCredentials(options);
+  EXPECT_NE(channel_creds_1, nullptr);
+  auto channel_creds_2 = TlsCredentials(options);
+  EXPECT_NE(channel_creds_2, nullptr);
+}
+
+TEST(CredentialsTest,
+     MultipleChannelCredentialsOneCertificateVerifierDoesNotLeak) {
+  TlsChannelCredentialsOptions options;
+  auto verifier = std::make_shared<NoOpCertificateVerifier>();
+  options.set_certificate_verifier(verifier);
+  auto channel_creds_1 = TlsCredentials(options);
+  EXPECT_NE(channel_creds_1, nullptr);
+  auto channel_creds_2 = TlsCredentials(options);
+  EXPECT_NE(channel_creds_2, nullptr);
+}
+
+TEST(CredentialsTest, MultipleChannelCredentialsOneCrlProviderDoesNotLeak) {
+  TlsChannelCredentialsOptions options;
+  auto provider = CreateStaticCrlProvider(/*crls=*/{});
+  EXPECT_TRUE(provider.ok());
+  options.set_crl_provider(*provider);
+  auto channel_creds_1 = TlsCredentials(options);
+  EXPECT_NE(channel_creds_1, nullptr);
+  auto channel_creds_2 = TlsCredentials(options);
+  EXPECT_NE(channel_creds_2, nullptr);
+}
 
 }  // namespace
 }  // namespace testing

test/cpp/server/credentials_test.cc

@@ -41,9 +41,13 @@ constexpr const char* kIdentityCertName = "identity_cert_name";
 constexpr const char* kIdentityCertPrivateKey = "identity_private_key";
 constexpr const char* kIdentityCertContents = "identity_cert_contents";
 
+using ::grpc::experimental::CreateStaticCrlProvider;
 using ::grpc::experimental::ExternalCertificateVerifier;
 using ::grpc::experimental::FileWatcherCertificateProvider;
+using ::grpc::experimental::NoOpCertificateVerifier;
 using ::grpc::experimental::StaticDataCertificateProvider;
+using ::grpc::experimental::TlsServerCredentials;
+using ::grpc::experimental::TlsServerCredentialsOptions;
 
 }  // namespace
 
@@ -200,8 +204,78 @@ TEST(CredentialsTest, TlsServerCredentialsWithCrlProviderAndDirectory) {
   GPR_ASSERT(server_credentials != nullptr);
 }
 
-// TODO(gtcooke94) - Add test to make sure Tls*CredentialsOptions does not leak
+TEST(CredentialsTest, TlsCredentialsOptionsDoesNotLeak) {
-// when not moved into TlsCredentials
+  auto provider = std::make_shared<StaticDataCertificateProvider>("root-pem");
+  TlsServerCredentialsOptions options(provider);
+  (void)options;
+}
+
+TEST(CredentialsTest, MultipleOptionsOneCertificateProviderDoesNotLeak) {
+  auto provider = std::make_shared<StaticDataCertificateProvider>("root-pem");
+  TlsServerCredentialsOptions options_1(provider);
+  (void)options_1;
+  TlsServerCredentialsOptions options_2(provider);
+  (void)options_2;
+}
+
+TEST(CredentialsTest, MultipleOptionsOneCertificateVerifierDoesNotLeak) {
+  auto provider = std::make_shared<StaticDataCertificateProvider>("root-pem");
+  auto verifier = std::make_shared<NoOpCertificateVerifier>();
+  TlsServerCredentialsOptions options_1(provider);
+  options_1.set_certificate_verifier(verifier);
+  TlsServerCredentialsOptions options_2(provider);
+  options_2.set_certificate_verifier(verifier);
+}
+
+TEST(CredentialsTest, MultipleOptionsOneCrlProviderDoesNotLeak) {
+  auto provider = std::make_shared<StaticDataCertificateProvider>("root-pem");
+  auto crl_provider = CreateStaticCrlProvider(/*crls=*/{});
+  EXPECT_TRUE(crl_provider.ok());
+  TlsServerCredentialsOptions options_1(provider);
+  options_1.set_crl_provider(*crl_provider);
+  TlsServerCredentialsOptions options_2(provider);
+  options_2.set_crl_provider(*crl_provider);
+}
+
+TEST(CredentialsTest, TlsServerCredentialsDoesNotLeak) {
+  auto provider = std::make_shared<StaticDataCertificateProvider>("root-pem");
+  TlsServerCredentialsOptions options(provider);
+  auto server_creds = TlsServerCredentials(options);
+  EXPECT_NE(server_creds, nullptr);
+}
+
+TEST(CredentialsTest, MultipleServerCredentialsOneOptionsDoesNotLeak) {
+  auto provider = std::make_shared<StaticDataCertificateProvider>("root-pem");
+  TlsServerCredentialsOptions options(provider);
+  auto server_creds_1 = TlsServerCredentials(options);
+  EXPECT_NE(server_creds_1, nullptr);
+  auto server_creds_2 = TlsServerCredentials(options);
+  EXPECT_NE(server_creds_2, nullptr);
+}
+
+TEST(CredentialsTest,
+     MultipleServerCredentialsOneCertificateVerifierDoesNotLeak) {
+  auto provider = std::make_shared<StaticDataCertificateProvider>("root-pem");
+  TlsServerCredentialsOptions options(provider);
+  auto verifier = std::make_shared<NoOpCertificateVerifier>();
+  options.set_certificate_verifier(verifier);
+  auto server_creds_1 = TlsServerCredentials(options);
+  EXPECT_NE(server_creds_1, nullptr);
+  auto server_creds_2 = TlsServerCredentials(options);
+  EXPECT_NE(server_creds_2, nullptr);
+}
+
+TEST(CredentialsTest, MultipleServerCredentialsOneCrlProviderDoesNotLeak) {
+  auto provider = std::make_shared<StaticDataCertificateProvider>("root-pem");
+  TlsServerCredentialsOptions options(provider);
+  auto crl_provider = CreateStaticCrlProvider(/*crls=*/{});
+  EXPECT_TRUE(crl_provider.ok());
+  options.set_crl_provider(*crl_provider);
+  auto server_creds_1 = TlsServerCredentials(options);
+  EXPECT_NE(server_creds_1, nullptr);
+  auto server_creds_2 = TlsServerCredentials(options);
+  EXPECT_NE(server_creds_2, nullptr);
+}
 
 }  // namespace
 }  // namespace testing

tools/codegen/core/gen_grpc_tls_credentials_options.py

@@ -319,6 +319,7 @@ print(
 struct grpc_tls_credentials_options
     : public grpc_core::RefCounted<grpc_tls_credentials_options> {
  public:
+  grpc_tls_credentials_options() = default;
   ~grpc_tls_credentials_options() override = default;
 """,
     file=H,
@@ -394,6 +395,25 @@ for i in range(len(_DATA_MEMBERS)):
         operator_equal_content += " &&\n"
 print(operator_equal_content + ";\n  }", file=H)
 
+# Write out copy constructor
+print(
+    "\n  grpc_tls_credentials_options(grpc_tls_credentials_options& other) :",
+    file=H,
+)
+operator_equal_content = "      "
+for i in range(len(_DATA_MEMBERS)):
+    if i != 0:
+        operator_equal_content += "      "
+    if i == len(_DATA_MEMBERS) - 1:
+        operator_equal_content += (
+            _DATA_MEMBERS[i].name + "_(other." + _DATA_MEMBERS[i].name + "_)"
+        )
+    else:
+        operator_equal_content += (
+            _DATA_MEMBERS[i].name + "_(other." + _DATA_MEMBERS[i].name + "_),\n"
+        )
+print(operator_equal_content + "  {}", file=H)
+
 # Print out data member declarations
 print("\n private:", file=H)
 for data_member in _DATA_MEMBERS: