Implemented changes outlined in addendum to design doc.

include/grpc/grpc_security.h

@@ -816,6 +816,8 @@ typedef void (*grpc_tls_on_credential_reload_done_cb)(
       errors occurred when a credential reload request is scheduled/cancelled.
     - config is a pointer to the unique grpc_tls_credential_reload_config
       instance that this argument corresponds to.
+    - context is a pointer to a wrapped language implementation of this
+      grpc_tls_credential_reload_arg instance.
     It is used for experimental purposes for now and subject to change.
 */
 struct grpc_tls_credential_reload_arg {
@@ -825,6 +827,7 @@ struct grpc_tls_credential_reload_arg {
   grpc_ssl_certificate_config_reload_status status;
   const char* error_details;
   grpc_tls_credential_reload_config* config;
+  void* context;
 };
 
 /** Create a grpc_tls_credential_reload_config instance.
@@ -884,6 +887,8 @@ typedef void (*grpc_tls_on_server_authorization_check_done_cb)(
    - config is a pointer to the unique
      grpc_tls_server_authorization_check_config instance that this argument
      corresponds to.
+   - context is a pointer to a wrapped language implementation of this
+     grpc_tls_server_authorization_check_arg instance.
    It is used for experimental purpose for now and subject to change.
 */
 struct grpc_tls_server_authorization_check_arg {
@@ -895,6 +900,7 @@ struct grpc_tls_server_authorization_check_arg {
   grpc_status_code status;
   const char* error_details;
   grpc_tls_server_authorization_check_config* config;
+  void* context;
 };
 
 /** Create a grpc_tls_server_authorization_check_config instance.

include/grpcpp/security/tls_credentials_options.h

@@ -80,7 +80,7 @@ class TlsCredentialReloadArg {
   /** TlsCredentialReloadArg does not take ownership of the C arg that is passed
    * to the constructor. One must remember to free any memory allocated to the C
    * arg after using the setter functions below. **/
-  TlsCredentialReloadArg(grpc_tls_credential_reload_arg* arg) : c_arg_(arg) {}
+  TlsCredentialReloadArg(grpc_tls_credential_reload_arg* arg);
   ~TlsCredentialReloadArg();
 
   /** Getters for member fields. The callback function is not exposed.
@@ -118,13 +118,9 @@ class TlsCredentialReloadArg {
 struct TlsCredentialReloadInterface {
   virtual ~TlsCredentialReloadInterface() = default;
   /** A callback that invokes the credential reload. **/
-  virtual int Schedule(std::unique_ptr<TlsCredentialReloadArg>& arg) = 0;
+  virtual int Schedule(TlsCredentialReloadArg* arg) = 0;
   /** A callback that cancels a credential reload request. **/
-  virtual void Cancel(std::unique_ptr<TlsCredentialReloadArg>& arg) {}
-  /** A callback that cleans up any data associated to the
-   * interface or the config. It will be called when the config is no longer
-   * using the interface. **/
-  virtual void Release() {}
+  virtual void Cancel(TlsCredentialReloadArg* arg) {}
 };
 
 /** TLS credential reloag config, wraps grpc_tls_credential_reload_config. It is
@@ -136,11 +132,11 @@ class TlsCredentialReloadConfig {
                                 credential_reload_interface);
   ~TlsCredentialReloadConfig();
 
-  int Schedule(std::unique_ptr<TlsCredentialReloadArg>& arg) const {
+  int Schedule(TlsCredentialReloadArg* arg) const {
     return credential_reload_interface_->Schedule(arg);
   }
 
-  void Cancel(std::unique_ptr<TlsCredentialReloadArg>& arg) const {
+  void Cancel(TlsCredentialReloadArg* arg) const {
     credential_reload_interface_->Cancel(arg);
   }
 
@@ -166,8 +162,7 @@ class TlsServerAuthorizationCheckArg {
   /** TlsServerAuthorizationCheckArg does not take ownership of the C arg passed
    * to the constructor. One must remember to free any memory allocated to the
    * C arg after using the setter functions below. **/
-  TlsServerAuthorizationCheckArg(grpc_tls_server_authorization_check_arg* arg)
-      : c_arg_(arg) {}
+  TlsServerAuthorizationCheckArg(grpc_tls_server_authorization_check_arg* arg);
   ~TlsServerAuthorizationCheckArg();
 
   /** Getters for member fields. They return the corresponding fields of the
@@ -208,13 +203,9 @@ class TlsServerAuthorizationCheckArg {
 struct TlsServerAuthorizationCheckInterface {
   virtual ~TlsServerAuthorizationCheckInterface() = default;
   /** A callback that invokes the server authorization check. **/
-  virtual int Schedule(
-      std::unique_ptr<TlsServerAuthorizationCheckArg>& arg) = 0;
+  virtual int Schedule(TlsServerAuthorizationCheckArg* arg) = 0;
   /** A callback that cancels a server authorization check request. **/
-  virtual void Cancel(std::unique_ptr<TlsServerAuthorizationCheckArg>& arg) {}
-  /** A callback that cleans up any data associated to the
-   * interface or the config. **/
-  virtual void Release() {}
+  virtual void Cancel(TlsServerAuthorizationCheckArg* arg) {}
 };
 
 /** TLS server authorization check config, wraps
@@ -229,11 +220,11 @@ class TlsServerAuthorizationCheckConfig {
           server_authorization_check_interface);
   ~TlsServerAuthorizationCheckConfig();
 
-  int Schedule(std::unique_ptr<TlsServerAuthorizationCheckArg>& arg) const {
+  int Schedule(TlsServerAuthorizationCheckArg* arg) const {
     return server_authorization_check_interface_->Schedule(arg);
   }
 
-  void Cancel(std::unique_ptr<TlsServerAuthorizationCheckArg>& arg) const {
+  void Cancel(TlsServerAuthorizationCheckArg* arg) const {
     server_authorization_check_interface_->Cancel(arg);
   }
 

src/cpp/common/tls_credentials_options.cc

@@ -33,7 +33,16 @@ void TlsKeyMaterialsConfig::set_key_materials(
 }
 
 /** TLS credential reload arg API implementation **/
-TlsCredentialReloadArg::~TlsCredentialReloadArg() {}
+TlsCredentialReloadArg::TlsCredentialReloadArg(
+    grpc_tls_credential_reload_arg* arg)
+    : c_arg_(arg) {
+  if (c_arg_ != nullptr && c_arg_->context != nullptr) {
+    gpr_log(GPR_ERROR, "c_arg context has already been set");
+  }
+  c_arg_->context = static_cast<void*>(this);
+}
+
+TlsCredentialReloadArg::~TlsCredentialReloadArg() { c_arg_->context = nullptr; }
 
 void* TlsCredentialReloadArg::cb_user_data() const {
   return c_arg_->cb_user_data;
@@ -95,14 +104,21 @@ TlsCredentialReloadConfig::TlsCredentialReloadConfig(
   c_config_->set_context(static_cast<void*>(this));
 }
 
-TlsCredentialReloadConfig::~TlsCredentialReloadConfig() {
-  if (credential_reload_interface_ != nullptr) {
-    credential_reload_interface_->Release();
+TlsCredentialReloadConfig::~TlsCredentialReloadConfig() {}
+
+/** gRPC TLS server authorization check arg API implementation **/
+TlsServerAuthorizationCheckArg::TlsServerAuthorizationCheckArg(
+    grpc_tls_server_authorization_check_arg* arg)
+    : c_arg_(arg) {
+  if (c_arg_ != nullptr && c_arg_->context != nullptr) {
+    gpr_log(GPR_ERROR, "c_arg context has already been set");
   }
+  c_arg_->context = static_cast<void*>(this);
 }
 
-/** gRPC TLS server authorization check arg API implementation **/
-TlsServerAuthorizationCheckArg::~TlsServerAuthorizationCheckArg() {}
+TlsServerAuthorizationCheckArg::~TlsServerAuthorizationCheckArg() {
+  c_arg_->context = nullptr;
+}
 
 void* TlsServerAuthorizationCheckArg::cb_user_data() const {
   return c_arg_->cb_user_data;
@@ -176,11 +192,7 @@ TlsServerAuthorizationCheckConfig::TlsServerAuthorizationCheckConfig(
   c_config_->set_context(static_cast<void*>(this));
 }
 
-TlsServerAuthorizationCheckConfig::~TlsServerAuthorizationCheckConfig() {
-  if (server_authorization_check_interface_ != nullptr) {
-    server_authorization_check_interface_->Release();
-  }
-}
+TlsServerAuthorizationCheckConfig::~TlsServerAuthorizationCheckConfig() {}
 
 /** gRPC TLS credential options API implementation **/
 TlsCredentialsOptions::TlsCredentialsOptions(

src/cpp/common/tls_credentials_options_util.cc

@@ -92,9 +92,10 @@ int TlsCredentialReloadConfigCSchedule(void* config_user_data,
   }
   TlsCredentialReloadConfig* cpp_config =
       static_cast<TlsCredentialReloadConfig*>(arg->config->context());
-  std::unique_ptr<TlsCredentialReloadArg> cpp_arg(
-      new TlsCredentialReloadArg(arg));
-  return cpp_config->Schedule(cpp_arg);
+  TlsCredentialReloadArg* cpp_arg = new TlsCredentialReloadArg(arg);
+  int schedule_result = cpp_config->Schedule(cpp_arg);
+  delete cpp_arg;
+  return schedule_result;
 }
 
 void TlsCredentialReloadConfigCCancel(void* config_user_data,
@@ -104,11 +105,16 @@ void TlsCredentialReloadConfigCCancel(void* config_user_data,
     gpr_log(GPR_ERROR, "credential reload arg was not properly initialized");
     return;
   }
+  if (arg->context == nullptr) {
+    gpr_log(GPR_ERROR, "credential reload arg schedule has already completed");
+    return;
+  }
   TlsCredentialReloadConfig* cpp_config =
       static_cast<TlsCredentialReloadConfig*>(arg->config->context());
-  std::unique_ptr<TlsCredentialReloadArg> cpp_arg(
-      new TlsCredentialReloadArg(arg));
+  TlsCredentialReloadArg* cpp_arg =
+      static_cast<TlsCredentialReloadArg*>(arg->context);
   cpp_config->Cancel(cpp_arg);
+  delete cpp_arg;
 }
 
 /** The C schedule and cancel functions for the server authorization check
@@ -124,9 +130,11 @@ int TlsServerAuthorizationCheckConfigCSchedule(
   }
   TlsServerAuthorizationCheckConfig* cpp_config =
       static_cast<TlsServerAuthorizationCheckConfig*>(arg->config->context());
-  std::unique_ptr<TlsServerAuthorizationCheckArg> cpp_arg(
-      new TlsServerAuthorizationCheckArg(arg));
-  return cpp_config->Schedule(cpp_arg);
+  TlsServerAuthorizationCheckArg* cpp_arg =
+      new TlsServerAuthorizationCheckArg(arg);
+  int schedule_result = cpp_config->Schedule(cpp_arg);
+  delete cpp_arg;
+  return schedule_result;
 }
 
 void TlsServerAuthorizationCheckConfigCCancel(
@@ -137,11 +145,17 @@ void TlsServerAuthorizationCheckConfigCCancel(
             "server authorization check arg was not properly initialized");
     return;
   }
+  if (arg->context == nullptr) {
+    gpr_log(GPR_ERROR,
+            "server authorization check arg schedule has already completed");
+    return;
+  }
   TlsServerAuthorizationCheckConfig* cpp_config =
       static_cast<TlsServerAuthorizationCheckConfig*>(arg->config->context());
-  std::unique_ptr<TlsServerAuthorizationCheckArg> cpp_arg(
-      new TlsServerAuthorizationCheckArg(arg));
+  TlsServerAuthorizationCheckArg* cpp_arg =
+      static_cast<TlsServerAuthorizationCheckArg*>(arg->context);
   cpp_config->Cancel(cpp_arg);
+  delete cpp_arg;
 }
 
 }  // namespace experimental

src/cpp/common/tls_credentials_options_util.h

@@ -35,7 +35,9 @@ std::shared_ptr<TlsKeyMaterialsConfig> ConvertToCppKeyMaterialsConfig(
     const grpc_tls_key_materials_config* config);
 
 /** The following 4 functions convert the user-provided schedule or cancel
- * functions into C style schedule or cancel functions. **/
+ *  functions into C style schedule or cancel functions. These are internal
+ *  functions, not meant to be accessed by the user. They are exposed for
+ *  testing purposes only. **/
 int TlsCredentialReloadConfigCSchedule(void* config_user_data,
                                        grpc_tls_credential_reload_arg* arg);
 

test/cpp/client/credentials_test.cc

@@ -51,7 +51,7 @@ static void tls_credential_reload_callback(
 }
 
 class TestTlsCredentialReload : public TlsCredentialReloadInterface {
-  int Schedule(std::unique_ptr<TlsCredentialReloadArg>& arg) override {
+  int Schedule(TlsCredentialReloadArg* arg) override {
     GPR_ASSERT(arg != nullptr);
     struct TlsKeyMaterialsConfig::PemKeyCertPair pair3 = {"private_key3",
                                                           "cert_chain3"};
@@ -69,7 +69,7 @@ class TestTlsCredentialReload : public TlsCredentialReloadInterface {
     return 0;
   }
 
-  void Cancel(std::unique_ptr<TlsCredentialReloadArg>& arg) override {
+  void Cancel(TlsCredentialReloadArg* arg) override {
     GPR_ASSERT(arg != nullptr);
     arg->set_status(GRPC_SSL_CERTIFICATE_CONFIG_RELOAD_FAIL);
     arg->set_error_details("cancelled");
@@ -90,7 +90,7 @@ static void tls_server_authorization_check_callback(
 
 class TestTlsServerAuthorizationCheck
     : public TlsServerAuthorizationCheckInterface {
-  int Schedule(std::unique_ptr<TlsServerAuthorizationCheckArg>& arg) override {
+  int Schedule(TlsServerAuthorizationCheckArg* arg) override {
     GPR_ASSERT(arg != nullptr);
     grpc::string cb_user_data = "cb_user_data";
     arg->set_cb_user_data(static_cast<void*>(gpr_strdup(cb_user_data.c_str())));
@@ -102,7 +102,7 @@ class TestTlsServerAuthorizationCheck
     return 1;
   }
 
-  void Cancel(std::unique_ptr<TlsServerAuthorizationCheckArg>& arg) override {
+  void Cancel(TlsServerAuthorizationCheckArg* arg) override {
     GPR_ASSERT(arg != nullptr);
     arg->set_status(GRPC_STATUS_PERMISSION_DENIED);
     arg->set_error_details("cancelled");
@@ -348,9 +348,8 @@ TEST_F(CredentialsTest, TlsCredentialReloadConfigSchedule) {
       new TestTlsCredentialReload());
   TlsCredentialReloadConfig config(std::move(test_credential_reload));
   grpc_tls_credential_reload_arg c_arg;
-  std::unique_ptr<TlsCredentialReloadArg> arg(
-      new TlsCredentialReloadArg(&c_arg));
-  arg->set_cb_user_data(static_cast<void*>(nullptr));
+  TlsCredentialReloadArg arg(&c_arg);
+  arg.set_cb_user_data(static_cast<void*>(nullptr));
   std::shared_ptr<TlsKeyMaterialsConfig> key_materials_config(
       new TlsKeyMaterialsConfig());
   struct TlsKeyMaterialsConfig::PemKeyCertPair pair1 = {"private_key1",
@@ -359,20 +358,19 @@ TEST_F(CredentialsTest, TlsCredentialReloadConfigSchedule) {
                                                         "cert_chain2"};
   std::vector<TlsKeyMaterialsConfig::PemKeyCertPair> pair_list = {pair1, pair2};
   key_materials_config->set_key_materials("pem_root_certs", pair_list);
-  arg->set_key_materials_config(key_materials_config);
-  arg->set_status(GRPC_SSL_CERTIFICATE_CONFIG_RELOAD_NEW);
-  arg->set_error_details("error_details");
+  arg.set_key_materials_config(key_materials_config);
+  arg.set_status(GRPC_SSL_CERTIFICATE_CONFIG_RELOAD_NEW);
+  arg.set_error_details("error_details");
   grpc_tls_key_materials_config* key_materials_config_before_schedule =
       c_arg.key_materials_config;
   const char* error_details_before_schedule = c_arg.error_details;
 
-  int schedule_output = config.Schedule(arg);
-  GPR_ASSERT(arg != nullptr);
+  int schedule_output = config.Schedule(&arg);
   EXPECT_EQ(schedule_output, 0);
-  EXPECT_EQ(arg->cb_user_data(), nullptr);
-  EXPECT_STREQ(arg->key_materials_config()->pem_root_certs().c_str(),
+  EXPECT_EQ(arg.cb_user_data(), nullptr);
+  EXPECT_STREQ(arg.key_materials_config()->pem_root_certs().c_str(),
                "new_pem_root_certs");
-  pair_list = arg->key_materials_config()->pem_key_cert_pair_list();
+  pair_list = arg.key_materials_config()->pem_key_cert_pair_list();
   EXPECT_EQ(static_cast<int>(pair_list.size()), 3);
   EXPECT_STREQ(pair_list[0].private_key.c_str(), "private_key01");
   EXPECT_STREQ(pair_list[0].cert_chain.c_str(), "cert_chain01");
@@ -380,8 +378,8 @@ TEST_F(CredentialsTest, TlsCredentialReloadConfigSchedule) {
   EXPECT_STREQ(pair_list[1].cert_chain.c_str(), "cert_chain2");
   EXPECT_STREQ(pair_list[2].private_key.c_str(), "private_key3");
   EXPECT_STREQ(pair_list[2].cert_chain.c_str(), "cert_chain3");
-  EXPECT_EQ(arg->status(), GRPC_SSL_CERTIFICATE_CONFIG_RELOAD_NEW);
-  EXPECT_STREQ(arg->error_details().c_str(), "error_details");
+  EXPECT_EQ(arg.status(), GRPC_SSL_CERTIFICATE_CONFIG_RELOAD_NEW);
+  EXPECT_STREQ(arg.error_details().c_str(), "error_details");
 
   // Cleanup.
   gpr_free(const_cast<char*>(error_details_before_schedule));
@@ -437,13 +435,8 @@ TEST_F(CredentialsTest, TlsCredentialReloadConfigCppToC) {
   grpc_tls_key_materials_config* key_materials_config_after_schedule =
       c_arg.key_materials_config;
 
-  c_config->Cancel(&c_arg);
-  EXPECT_EQ(c_arg.status, GRPC_SSL_CERTIFICATE_CONFIG_RELOAD_FAIL);
-  EXPECT_STREQ(c_arg.error_details, "cancelled");
-
   // Cleanup.
   ::grpc_core::Delete(key_materials_config_after_schedule);
-  gpr_free(const_cast<char*>(c_arg.error_details));
   ::grpc_core::Delete(config.c_config());
 }
 
@@ -490,29 +483,28 @@ TEST_F(CredentialsTest, TlsServerAuthorizationCheckConfigSchedule) {
   TlsServerAuthorizationCheckConfig config(
       std::move(test_server_authorization_check));
   grpc_tls_server_authorization_check_arg c_arg;
-  std::unique_ptr<TlsServerAuthorizationCheckArg> arg(
-      new TlsServerAuthorizationCheckArg(&c_arg));
-  arg->set_cb_user_data(nullptr);
-  arg->set_success(0);
-  arg->set_target_name("target_name");
-  arg->set_peer_cert("peer_cert");
-  arg->set_status(GRPC_STATUS_PERMISSION_DENIED);
-  arg->set_error_details("error_details");
+  TlsServerAuthorizationCheckArg arg(&c_arg);
+  arg.set_cb_user_data(nullptr);
+  arg.set_success(0);
+  arg.set_target_name("target_name");
+  arg.set_peer_cert("peer_cert");
+  arg.set_status(GRPC_STATUS_PERMISSION_DENIED);
+  arg.set_error_details("error_details");
   const char* target_name_before_schedule = c_arg.target_name;
   const char* peer_cert_before_schedule = c_arg.peer_cert;
   const char* error_details_before_schedule = c_arg.error_details;
 
-  int schedule_output = config.Schedule(arg);
+  int schedule_output = config.Schedule(&arg);
   EXPECT_EQ(schedule_output, 1);
-  EXPECT_STREQ(static_cast<char*>(arg->cb_user_data()), "cb_user_data");
-  EXPECT_EQ(arg->success(), 1);
-  EXPECT_STREQ(arg->target_name().c_str(), "sync_target_name");
-  EXPECT_STREQ(arg->peer_cert().c_str(), "sync_peer_cert");
-  EXPECT_EQ(arg->status(), GRPC_STATUS_OK);
-  EXPECT_STREQ(arg->error_details().c_str(), "sync_error_details");
+  EXPECT_STREQ(static_cast<char*>(arg.cb_user_data()), "cb_user_data");
+  EXPECT_EQ(arg.success(), 1);
+  EXPECT_STREQ(arg.target_name().c_str(), "sync_target_name");
+  EXPECT_STREQ(arg.peer_cert().c_str(), "sync_peer_cert");
+  EXPECT_EQ(arg.status(), GRPC_STATUS_OK);
+  EXPECT_STREQ(arg.error_details().c_str(), "sync_error_details");
 
   // Cleanup.
-  gpr_free(arg->cb_user_data());
+  gpr_free(arg.cb_user_data());
   gpr_free(const_cast<char*>(target_name_before_schedule));
   gpr_free(const_cast<char*>(peer_cert_before_schedule));
   gpr_free(const_cast<char*>(error_details_before_schedule));
@@ -547,18 +539,12 @@ TEST_F(CredentialsTest, TlsServerAuthorizationCheckConfigCppToC) {
 
   const char* target_name_after_schedule = c_arg.target_name;
   const char* peer_cert_after_schedule = c_arg.peer_cert;
-  const char* error_details_after_schedule = c_arg.error_details;
-
-  (c_arg.config)->Cancel(&c_arg);
-  EXPECT_EQ(c_arg.status, GRPC_STATUS_PERMISSION_DENIED);
-  EXPECT_STREQ(c_arg.error_details, "cancelled");
 
   // Cleanup.
   gpr_free(c_arg.cb_user_data);
   gpr_free(const_cast<char*>(c_arg.error_details));
   gpr_free(const_cast<char*>(target_name_after_schedule));
   gpr_free(const_cast<char*>(peer_cert_after_schedule));
-  gpr_free(const_cast<char*>(error_details_after_schedule));
   ::grpc_core::Delete(config.c_config());
 }