From c45a97d8cde6c4c2d9fd7afca060ec8abff8a525 Mon Sep 17 00:00:00 2001 From: Matthew Stevenson Date: Tue, 8 Oct 2019 19:15:31 -0700 Subject: [PATCH] Changed delete per Yang's comment. --- include/grpc/grpc_security.h | 6 +++++ .../tls/spiffe_security_connector.cc | 6 +++++ src/cpp/common/tls_credentials_options.cc | 2 ++ .../common/tls_credentials_options_util.cc | 20 ++++++++++++--- src/cpp/common/tls_credentials_options_util.h | 9 +++++-- test/cpp/client/credentials_test.cc | 25 ++++++++++++++++--- 6 files changed, 58 insertions(+), 10 deletions(-) diff --git a/include/grpc/grpc_security.h b/include/grpc/grpc_security.h index 4c1a2a02cbe..2bd30d912b7 100644 --- a/include/grpc/grpc_security.h +++ b/include/grpc/grpc_security.h @@ -818,6 +818,8 @@ typedef void (*grpc_tls_on_credential_reload_done_cb)( instance that this argument corresponds to. - context is a pointer to a wrapped language implementation of this grpc_tls_credential_reload_arg instance. + - destroy_context is a pointer to a caller-provided method that cleans + up any data associated with the context pointer. It is used for experimental purposes for now and subject to change. */ struct grpc_tls_credential_reload_arg { @@ -828,6 +830,7 @@ struct grpc_tls_credential_reload_arg { const char* error_details; grpc_tls_credential_reload_config* config; void* context; + void (*destroy_context)(void* ctx); }; /** Create a grpc_tls_credential_reload_config instance. @@ -889,6 +892,8 @@ typedef void (*grpc_tls_on_server_authorization_check_done_cb)( corresponds to. - context is a pointer to a wrapped language implementation of this grpc_tls_server_authorization_check_arg instance. + - destroy_context is a pointer to a caller-provided method that cleans + up any data associated with the context pointer. It is used for experimental purpose for now and subject to change. */ struct grpc_tls_server_authorization_check_arg { @@ -901,6 +906,7 @@ struct grpc_tls_server_authorization_check_arg { const char* error_details; grpc_tls_server_authorization_check_config* config; void* context; + void (*destroy_context)(void* ctx); }; /** Create a grpc_tls_server_authorization_check_config instance. diff --git a/src/core/lib/security/security_connector/tls/spiffe_security_connector.cc b/src/core/lib/security/security_connector/tls/spiffe_security_connector.cc index bac32c08813..b56559458f3 100644 --- a/src/core/lib/security/security_connector/tls/spiffe_security_connector.cc +++ b/src/core/lib/security/security_connector/tls/spiffe_security_connector.cc @@ -104,6 +104,9 @@ grpc_status_code TlsFetchKeyMaterials( } } gpr_free((void*)arg->error_details); + if (arg->destroy_context != nullptr) { + arg->destroy_context(arg->context); + } grpc_core::Delete(arg); } return status; @@ -392,6 +395,9 @@ void SpiffeChannelSecurityConnector::ServerAuthorizationCheckArgDestroy( gpr_free((void*)arg->target_name); gpr_free((void*)arg->peer_cert); gpr_free((void*)arg->error_details); + if (arg->destroy_context != nullptr) { + arg->destroy_context(arg->context); + } grpc_core::Delete(arg); } diff --git a/src/cpp/common/tls_credentials_options.cc b/src/cpp/common/tls_credentials_options.cc index 7c16241717b..d06b47737ec 100644 --- a/src/cpp/common/tls_credentials_options.cc +++ b/src/cpp/common/tls_credentials_options.cc @@ -51,6 +51,7 @@ TlsCredentialReloadArg::TlsCredentialReloadArg( gpr_log(GPR_ERROR, "c_arg context has already been set"); } c_arg_->context = static_cast(this); + c_arg_->destroy_context = &TlsCredentialReloadArgDestroyContext; } TlsCredentialReloadArg::~TlsCredentialReloadArg() {} @@ -166,6 +167,7 @@ TlsServerAuthorizationCheckArg::TlsServerAuthorizationCheckArg( gpr_log(GPR_ERROR, "c_arg context has already been set"); } c_arg_->context = static_cast(this); + c_arg_->destroy_context = &TlsServerAuthorizationCheckArgDestroyContext; } TlsServerAuthorizationCheckArg::~TlsServerAuthorizationCheckArg() {} diff --git a/src/cpp/common/tls_credentials_options_util.cc b/src/cpp/common/tls_credentials_options_util.cc index 0f644638d75..89709005dcb 100644 --- a/src/cpp/common/tls_credentials_options_util.cc +++ b/src/cpp/common/tls_credentials_options_util.cc @@ -70,7 +70,6 @@ int TlsCredentialReloadConfigCSchedule(void* config_user_data, static_cast(arg->config->context()); TlsCredentialReloadArg* cpp_arg = new TlsCredentialReloadArg(arg); int schedule_result = cpp_config->Schedule(cpp_arg); - delete cpp_arg; return schedule_result; } @@ -90,7 +89,14 @@ void TlsCredentialReloadConfigCCancel(void* config_user_data, TlsCredentialReloadArg* cpp_arg = static_cast(arg->context); cpp_config->Cancel(cpp_arg); - delete cpp_arg; +} + +void TlsCredentialReloadArgDestroyContext(void* context) { + if (context != nullptr) { + TlsCredentialReloadArg* cpp_arg = + static_cast(context); + delete cpp_arg; + } } /** The C schedule and cancel functions for the server authorization check @@ -109,7 +115,6 @@ int TlsServerAuthorizationCheckConfigCSchedule( TlsServerAuthorizationCheckArg* cpp_arg = new TlsServerAuthorizationCheckArg(arg); int schedule_result = cpp_config->Schedule(cpp_arg); - delete cpp_arg; return schedule_result; } @@ -131,7 +136,14 @@ void TlsServerAuthorizationCheckConfigCCancel( TlsServerAuthorizationCheckArg* cpp_arg = static_cast(arg->context); cpp_config->Cancel(cpp_arg); - delete cpp_arg; +} + +void TlsServerAuthorizationCheckArgDestroyContext(void* context) { + if (context != nullptr) { + TlsServerAuthorizationCheckArg* cpp_arg = + static_cast(context); + delete cpp_arg; + } } } // namespace experimental diff --git a/src/cpp/common/tls_credentials_options_util.h b/src/cpp/common/tls_credentials_options_util.h index d5e20846254..93e94562398 100644 --- a/src/cpp/common/tls_credentials_options_util.h +++ b/src/cpp/common/tls_credentials_options_util.h @@ -33,8 +33,7 @@ grpc_tls_key_materials_config* ConvertToCKeyMaterialsConfig( /** The following 4 functions convert the user-provided schedule or cancel * 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. **/ + * functions, not meant to be accessed by the user. **/ int TlsCredentialReloadConfigCSchedule(void* config_user_data, grpc_tls_credential_reload_arg* arg); @@ -47,6 +46,12 @@ int TlsServerAuthorizationCheckConfigCSchedule( void TlsServerAuthorizationCheckConfigCCancel( void* config_user_data, grpc_tls_server_authorization_check_arg* arg); +/** The following 2 functions cleanup data created in the above C schedule + * functions. **/ +void TlsCredentialReloadArgDestroyContext(void* context); + +void TlsServerAuthorizationCheckArgDestroyContext(void* context); + } // namespace experimental } // namespace grpc_impl diff --git a/test/cpp/client/credentials_test.cc b/test/cpp/client/credentials_test.cc index 4d216d1609d..3eb6fb52b9e 100644 --- a/test/cpp/client/credentials_test.cc +++ b/test/cpp/client/credentials_test.cc @@ -369,7 +369,10 @@ TEST_F(CredentialsTest, TlsCredentialReloadConfigSchedule) { // Cleanup. gpr_free(const_cast(error_details_before_schedule)); grpc_core::Delete(c_arg->key_materials_config); - delete arg; + // delete arg; + if (c_arg->destroy_context != nullptr) { + c_arg->destroy_context(c_arg->context); + } gpr_free(c_arg); gpr_free(config->c_config()); } @@ -422,6 +425,7 @@ TEST_F(CredentialsTest, TlsCredentialReloadConfigCppToC) { c_arg.key_materials_config; // Cleanup. + c_arg.destroy_context(c_arg.context); ::grpc_core::Delete(config.c_config()); } @@ -501,7 +505,10 @@ TEST_F(CredentialsTest, TlsServerAuthorizationCheckConfigSchedule) { gpr_free(const_cast(c_arg->target_name)); gpr_free(const_cast(c_arg->peer_cert)); gpr_free(const_cast(c_arg->error_details)); - delete arg; + // delete arg; + if (c_arg->destroy_context != nullptr) { + c_arg->destroy_context(c_arg->context); + } gpr_free(c_arg); gpr_free(config.c_config()); } @@ -530,6 +537,7 @@ TEST_F(CredentialsTest, TlsServerAuthorizationCheckConfigCppToC) { // Cleanup. gpr_free(c_arg.cb_user_data); + c_arg.destroy_context(c_arg.context); gpr_free(const_cast(c_arg.error_details)); gpr_free(const_cast(c_arg.target_name)); gpr_free(const_cast(c_arg.peer_cert)); @@ -632,6 +640,9 @@ TEST_F(CredentialsTest, TlsCredentialsOptionsCppToC) { // Cleanup. ::grpc_core::Delete(c_credential_reload_arg.key_materials_config); + c_credential_reload_arg.destroy_context(c_credential_reload_arg.context); + c_server_authorization_check_arg.destroy_context( + c_server_authorization_check_arg.context); gpr_free(c_server_authorization_check_arg.cb_user_data); gpr_free(const_cast(c_server_authorization_check_arg.target_name)); gpr_free(const_cast(c_server_authorization_check_arg.peer_cert)); @@ -683,7 +694,10 @@ TEST_F(CredentialsTest, TlsCredentialReloadConfigErrorMessages) { // Cleanup. gpr_free(const_cast(c_arg->error_details)); - delete arg; + // delete arg; + if (c_arg->destroy_context != nullptr) { + c_arg->destroy_context(c_arg->context); + } delete c_arg; gpr_free(config->c_config()); } @@ -713,7 +727,10 @@ TEST_F(CredentialsTest, TlsServerAuthorizationCheckConfigErrorMessages) { // Cleanup. gpr_free(const_cast(c_arg->error_details)); - delete arg; + // delete arg; + if (c_arg->destroy_context != nullptr) { + c_arg->destroy_context(c_arg->context); + } delete c_arg; gpr_free(config->c_config()); }