From 6b47f0a3b34c265d13e8097ec4380f38720ec41f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Canna=C3=B2?= Date: Sun, 23 Apr 2023 11:02:55 +0000 Subject: [PATCH 1/3] Alternative to Fix invalid memory accesses to 'GloVars::checksums_values' during shutdown This is an alternative implementation of commit cfb34ef256a29f3610ea24104a6a7445e5227756 because it was failing to compile on clang 14 , that was complaining about a missing unnamed destructor --- include/proxysql_glovars.hpp | 48 +++++++++++++++++------------------- lib/ProxySQL_GloVars.cpp | 7 +----- 2 files changed, 24 insertions(+), 31 deletions(-) diff --git a/include/proxysql_glovars.hpp b/include/proxysql_glovars.hpp index 28778c45b8..70d02cafd9 100644 --- a/include/proxysql_glovars.hpp +++ b/include/proxysql_glovars.hpp @@ -31,11 +31,13 @@ class ProxySQL_Checksum_Value { char *checksum; unsigned long long version; unsigned long long epoch; + bool in_shutdown; ProxySQL_Checksum_Value() { checksum = (char *)malloc(20); memset(checksum,0,20); version = 0; epoch = 0; + in_shutdown = false; } void set_checksum(char *c) { memset(checksum,0,20); @@ -43,8 +45,15 @@ class ProxySQL_Checksum_Value { replace_checksum_zeros(checksum); } ~ProxySQL_Checksum_Value() { - free(checksum); - checksum = NULL; + if (in_shutdown == false) { + /** + * @brief the in_shutdown flag is false by default, but set to true + * in the destructor of ProxySQL_GlobalVariables. + * See comments there for futher details. + */ + free(checksum); + checksum = NULL; + } } }; @@ -125,29 +134,18 @@ class ProxySQL_GlobalVariables { } statuses; pthread_mutex_t checksum_mutex; time_t epoch_version; - /** - * @brief Anonymous union holding just the member 'checksums_values'. - * @details This encapsulation is performed to prevent the call to 'ProxySQL_Checksum_Value' destructor for - * 'checksums_values' members. These checksums memory is never freed during ProxySQL execution - * lifetime (only reused during update operations), and they are concurrently access by multiple threads, - * including during shutdown phase. Since 'GloVars' (unique instance of this class) is declared global, - * it's impossible to control de destruction order with respect to the other modules. To avoid invalid - * memory accesses during shutdown, we avoid calling the destructor of the members at all. - */ - union { - struct { - ProxySQL_Checksum_Value admin_variables; - ProxySQL_Checksum_Value mysql_query_rules; - ProxySQL_Checksum_Value mysql_servers; - ProxySQL_Checksum_Value mysql_users; - ProxySQL_Checksum_Value mysql_variables; - ProxySQL_Checksum_Value ldap_variables; - ProxySQL_Checksum_Value proxysql_servers; - uint64_t global_checksum; - unsigned long long updates_cnt; - unsigned long long dumped_at; - } checksums_values; - }; + struct { + ProxySQL_Checksum_Value admin_variables; + ProxySQL_Checksum_Value mysql_query_rules; + ProxySQL_Checksum_Value mysql_servers; + ProxySQL_Checksum_Value mysql_users; + ProxySQL_Checksum_Value mysql_variables; + ProxySQL_Checksum_Value ldap_variables; + ProxySQL_Checksum_Value proxysql_servers; + uint64_t global_checksum; + unsigned long long updates_cnt; + unsigned long long dumped_at; + } checksums_values; uint64_t generate_global_checksum(); ProxySQL_GlobalVariables(); ~ProxySQL_GlobalVariables(); diff --git a/lib/ProxySQL_GloVars.cpp b/lib/ProxySQL_GloVars.cpp index e07cdf6fcc..1df01f5313 100644 --- a/lib/ProxySQL_GloVars.cpp +++ b/lib/ProxySQL_GloVars.cpp @@ -79,13 +79,8 @@ ProxySQL_GlobalVariables::~ProxySQL_GlobalVariables() { } }; -/** - * @brief ProxySQL_GlobalVariables constructor - * @details 'checksums_values' constructor is required to be explicitly called as it's encapsulated in a - * anonymous union. - */ ProxySQL_GlobalVariables::ProxySQL_GlobalVariables() : - prometheus_registry(std::make_shared()), checksums_values() + prometheus_registry(std::make_shared()) { confFile=NULL; __cmd_proxysql_config_file=NULL; From 34b4e3720d639f65a827009ea28f7e4f99e872b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Canna=C3=B2?= Date: Sun, 23 Apr 2023 11:39:10 +0000 Subject: [PATCH 2/3] Code erroneously skipped from previous commit --- lib/ProxySQL_GloVars.cpp | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/lib/ProxySQL_GloVars.cpp b/lib/ProxySQL_GloVars.cpp index 1df01f5313..66cb8f8362 100644 --- a/lib/ProxySQL_GloVars.cpp +++ b/lib/ProxySQL_GloVars.cpp @@ -77,6 +77,22 @@ ProxySQL_GlobalVariables::~ProxySQL_GlobalVariables() { free(ldap_auth_plugin); ldap_auth_plugin = NULL; } + /** + * @brief set in_shutdown flag just the member 'checksums_values'. + * @details This is performed to prevent the free() inside the 'ProxySQL_Checksum_Value' destructor for + * 'checksums_values' members. These checksums memory is never freed during ProxySQL execution + * lifetime (only reused during update operations), and they are concurrently access by multiple threads, + * including during shutdown phase. Since 'GloVars' (unique instance of this class) is declared global, + * it's impossible to control de destruction order with respect to the other modules. To avoid invalid + * memory accesses during shutdown, we avoid calling free() inside the destructor of the members. + */ + checksums_values.admin_variables.in_shutdown = true; + checksums_values.mysql_query_rules.in_shutdown = true; + checksums_values.mysql_servers.in_shutdown = true; + checksums_values.mysql_users.in_shutdown = true; + checksums_values.mysql_variables.in_shutdown = true; + checksums_values.ldap_variables.in_shutdown = true; + checksums_values.proxysql_servers.in_shutdown = true; }; ProxySQL_GlobalVariables::ProxySQL_GlobalVariables() : From 4f7859ba3f074f1dd13fa16564c9f8b83da3f6ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Canna=C3=B2?= Date: Tue, 25 Apr 2023 09:11:26 +0000 Subject: [PATCH 3/3] Remove unnecessary call to ProxySQL_Checksum_Value constructor in ProxySQL_Checksum_Value_2 constructor --- include/ProxySQL_Cluster.hpp | 1 - 1 file changed, 1 deletion(-) diff --git a/include/ProxySQL_Cluster.hpp b/include/ProxySQL_Cluster.hpp index 0dd72a3ede..b0d7002545 100644 --- a/include/ProxySQL_Cluster.hpp +++ b/include/ProxySQL_Cluster.hpp @@ -61,7 +61,6 @@ class ProxySQL_Checksum_Value_2: public ProxySQL_Checksum_Value { time_t last_changed; unsigned int diff_check; ProxySQL_Checksum_Value_2() { - ProxySQL_Checksum_Value(); last_changed = 0; last_updated = 0; diff_check = 0;