From fc0ab04f0d0f709e1996b23dbb029b11cf9a2cdc Mon Sep 17 00:00:00 2001 From: Georgi Kodinov Date: Fri, 10 Feb 2023 10:49:04 +0100 Subject: [PATCH] Bug#34953650: L seen in weekly-trunk mysql_component_sys_variable_imp::register_variable Several problems stacked up together: 1. The component initialization, when failing should clean up after itself. Fixed the validate_password component's init method to properly clean up in case of failures. 2. The validate_password component had an REQUIRES_SERIVCE(registry). While this is not wrong per se, it collided with the implicit REQUIRES_SERVICE(registry) done by the BEGIN_COMPONENT_REQUIRES() macro in that it was using the same placeholder global variable. So now the same service handle was released twice on error or component unload. Fixed by removing the second REQUIRES_SERVICE(registry). 3. The dynamic loader is releasing the newly acquired service references for the required services on initialization error. However after doing that it was actually setting the service handle placeholder to NULL. This is not wrong, but combined with problem #2 was causing a reference to the registry service to be acquired twice, stored into the same placeholder and then released just once, since after the first release the placeholder was set to null and thus the second release is a no-op. Fixed by not resetting the handle placeholder after releasing the service reference. 4. The system variable registration service wouldn't release the intermediate memory slots it was allocating on error. Fixed by using std::unique_ptr to handle the proper releasing. Change-Id: Ib2c7ae80736c591838af8c182fda1980be1e1f0e --- components/libminchassis/dynamic_loader.cc | 4 +-- .../validate_password_imp.cc | 25 ++++++++++++++++--- .../component_sys_var_service.cc | 18 +++++++++---- 3 files changed, 35 insertions(+), 12 deletions(-) diff --git a/components/libminchassis/dynamic_loader.cc b/components/libminchassis/dynamic_loader.cc index 53e25e3b753e..0248f7ee5f75 100644 --- a/components/libminchassis/dynamic_loader.cc +++ b/components/libminchassis/dynamic_loader.cc @@ -846,10 +846,8 @@ bool mysql_dynamic_loader_imp::load_do_resolve_dependencies( /* List of services acquired for component dependencies. */ std::vector acquired_services; auto guard = create_scope_guard([&acquired_services]() { - for (my_h_service *service_storage : acquired_services) { + for (my_h_service *service_storage : acquired_services) mysql_registry_imp::release(*service_storage); - *service_storage = nullptr; - } }); /* Acquire services to meet component dependencies. */ diff --git a/components/validate_password/validate_password_imp.cc b/components/validate_password/validate_password_imp.cc index af4d3b560b8f..1dd6e5191efa 100644 --- a/components/validate_password/validate_password_imp.cc +++ b/components/validate_password/validate_password_imp.cc @@ -870,9 +870,27 @@ static mysql_service_status_t validate_password_init() { dictionary_words = new set_type(); init_validate_password_psi_keys(); mysql_rwlock_init(key_validate_password_LOCK_dict_file, &LOCK_dict_file); - if (log_service_init() || register_system_variables() || - register_status_variables()) + if (log_service_init()) { + delete dictionary_words; + dictionary_words = nullptr; + mysql_rwlock_destroy(&LOCK_dict_file); return true; + } + if (register_system_variables()) { + log_service_deinit(); + delete dictionary_words; + dictionary_words = nullptr; + mysql_rwlock_destroy(&LOCK_dict_file); + return true; + } + if (register_status_variables()) { + unregister_system_variables(); + log_service_deinit(); + delete dictionary_words; + dictionary_words = nullptr; + mysql_rwlock_destroy(&LOCK_dict_file); + return true; + } read_dictionary_file(); /* Check if validate_password_length needs readjustment */ readjust_validate_password_length(); @@ -931,8 +949,7 @@ REQUIRES_MYSQL_RWLOCK_SERVICE_PLACEHOLDER; component load time and disposes off them at unload. */ BEGIN_COMPONENT_REQUIRES(validate_password) -REQUIRES_SERVICE(registry), REQUIRES_SERVICE(log_builtins), - REQUIRES_SERVICE(log_builtins_string), +REQUIRES_SERVICE(log_builtins), REQUIRES_SERVICE(log_builtins_string), REQUIRES_SERVICE(mysql_string_factory), REQUIRES_SERVICE(mysql_string_case), REQUIRES_SERVICE(mysql_string_converter), REQUIRES_SERVICE(mysql_string_iterator), diff --git a/sql/server_component/component_sys_var_service.cc b/sql/server_component/component_sys_var_service.cc index 163160a020e7..ce529cf9c1c7 100644 --- a/sql/server_component/component_sys_var_service.cc +++ b/sql/server_component/component_sys_var_service.cc @@ -123,7 +123,7 @@ DEFINE_BOOL_METHOD(mysql_component_sys_variable_imp::register_variable, try { struct sys_var_chain chain = {nullptr, nullptr}; sys_var *sysvar [[maybe_unused]]; - char *com_sys_var_name, *optname; + char *com_sys_var_name, *optname, *com_sys_var_name_copy; int com_sys_var_len; SYS_VAR *opt = nullptr; my_option *opts = nullptr; @@ -163,6 +163,7 @@ DEFINE_BOOL_METHOD(mysql_component_sys_variable_imp::register_variable, opts->arg_source = opts_arg_source; opts->arg_source->m_path_name[0] = 0; opts->arg_source->m_source = enum_variable_source::COMPILED; + std::unique_ptr unique_opt(nullptr, &my_free); switch (flags & PLUGIN_VAR_WITH_SIGN_TYPEMASK) { case PLUGIN_VAR_BOOL: @@ -298,6 +299,7 @@ DEFINE_BOOL_METHOD(mysql_component_sys_variable_imp::register_variable, component_name); goto end; } + unique_opt.reset(opt); plugin_opt_set_limits(opts, opt); opts->value = opts->u_max_value = *(uchar ***)(opt + 1); @@ -379,14 +381,20 @@ DEFINE_BOOL_METHOD(mysql_component_sys_variable_imp::register_variable, } } - sysvar = reinterpret_cast(new sys_var_pluginvar( - &chain, my_strdup(key_memory_comp_sys_var, com_sys_var_name, MYF(0)), - opt)); + com_sys_var_name_copy = + my_strdup(key_memory_comp_sys_var, com_sys_var_name, MYF(0)); + if (com_sys_var_name_copy == nullptr) { + LogErr(ERROR_LEVEL, ER_SYS_VAR_COMPONENT_OOM, var_name); + goto end; + } + sysvar = reinterpret_cast( + new sys_var_pluginvar(&chain, com_sys_var_name_copy, opt)); if (sysvar == nullptr) { LogErr(ERROR_LEVEL, ER_SYS_VAR_COMPONENT_OOM, var_name); goto end; - } + } else + unique_opt.release(); sysvar->set_arg_source(opts->arg_source); sysvar->set_is_plugin(false);