From 200fdecb8d870db39abe622f3ff875cd1abb0948 Mon Sep 17 00:00:00 2001 From: Rene Cannao Date: Fri, 13 Feb 2026 19:26:57 +0000 Subject: [PATCH] Apply AI agent review fixes to PR #5374 Critical Fixes: - Fix NULL pointer dereferences in MySQL_Logger.cpp: * Add null checks for myds->myconn->parent in log_audit_entry() * Fix cleanup code in log_request() and log_audit_entry() * Change sprintf to snprintf for buffer safety - Remove redundant pthread_rwlock_init in debug.cpp: * filters_rwlock already initialized with PTHREAD_RWLOCK_INITIALIZER * Re-initialization caused undefined behavior per POSIX Major Improvements: - Add bounded wait helper wait_for_glo_mth() in proxysql_utils.h: * Replaces indefinite while loops with 10s timeout * Applied to 26 locations across 4 files Build System: - Require C++17 in Makefile (remove C++11 fallback): * Code uses C++17 structured bindings extensively * Cleanest solution vs adding fallback guards to 27+ locations Compilation fixes: - Add unistd.h include for usleep - Remove conflicting GloMyMon extern declaration Files changed: - Makefile - include/proxysql_utils.h - lib/MySQL_Logger.cpp - lib/debug.cpp - lib/MySQL_Monitor.cpp - lib/ProxySQL_Admin.cpp - lib/ClickHouse_Server.cpp - src/SQLite3_Server.cpp --- Makefile | 10 +-- include/proxysql_utils.h | 21 ++++++ lib/ClickHouse_Server.cpp | 8 +- lib/MySQL_Logger.cpp | 16 ++-- lib/MySQL_Monitor.cpp | 152 ++++++++++++-------------------------- lib/ProxySQL_Admin.cpp | 6 +- lib/debug.cpp | 3 +- src/SQLite3_Server.cpp | 6 +- 8 files changed, 85 insertions(+), 137 deletions(-) diff --git a/Makefile b/Makefile index 4678a1e20..c8a9edacc 100644 --- a/Makefile +++ b/Makefile @@ -74,16 +74,12 @@ DEBUG := $(ALL_DEBUG) #export EXTRALINK export MAKE -### detect compiler support for c++11/17 +### detect compiler support for c++17 (required) CPLUSPLUS := $(shell ${CC} -std=c++17 -dM -E -x c++ /dev/null 2>/dev/null | grep -F __cplusplus | egrep -o '[0-9]{6}L') ifneq ($(CPLUSPLUS),201703L) - CPLUSPLUS := $(shell ${CC} -std=c++11 -dM -E -x c++ /dev/null 2>/dev/null| grep -F __cplusplus | egrep -o '[0-9]{6}L') - LEGACY_BUILD := 1 -ifneq ($(CPLUSPLUS),201103L) - $(error Compiler must support at least c++11) -endif + $(error Compiler must support at least c++17) endif -STDCPP := -std=c++$(shell echo $(CPLUSPLUS) | cut -c3-4) -DCXX$(shell echo $(CPLUSPLUS) | cut -c3-4) +STDCPP := -std=c++17 -DCXX17 ### detect distro DISTRO := Unknown diff --git a/include/proxysql_utils.h b/include/proxysql_utils.h index 2aeaf1cfc..2b150719a 100644 --- a/include/proxysql_utils.h +++ b/include/proxysql_utils.h @@ -11,6 +11,7 @@ #include #include #include +#include #include #include "../deps/json/json.hpp" @@ -371,4 +372,24 @@ std::string get_client_addr(struct sockaddr* client_addr); */ int check_port_availability(int port_num, bool* port_free); +// Forward declaration for GloMTH wait helper +class MySQL_Threads_Handler; +extern MySQL_Threads_Handler *GloMTH; + +/** + * @brief Wait for GloMTH to be initialized with a bounded timeout + * + * Waits up to 10 seconds for GloMTH initialization. + * Returns true if GloMTH is initialized, false if timeout. + * + * @return bool true if GloMTH is ready, false otherwise + */ +static inline bool wait_for_glo_mth() { + for (int i = 0; i < 200; ++i) { // ~10s total + if (GloMTH) return true; + usleep(50000); + } + return false; +} + #endif diff --git a/lib/ClickHouse_Server.cpp b/lib/ClickHouse_Server.cpp index e76fcf837..b58f4fdeb 100644 --- a/lib/ClickHouse_Server.cpp +++ b/lib/ClickHouse_Server.cpp @@ -11,6 +11,7 @@ #include "MySQL_Logger.hpp" #include "MySQL_Data_Stream.h" #include "MySQL_Query_Processor.h" +#include "proxysql_utils.h" #include #include @@ -1387,11 +1388,8 @@ static void *child_mysql(void *arg) { nfds_t nfds=1; int rc; pthread_mutex_unlock(&sock_mutex); - while (GloMTH==NULL) { - usleep(50000); - } - usleep(100000); - if (!GloMTH) return NULL; + // Wait for GloMTH to be initialized + if (!wait_for_glo_mth()) return NULL; // quick exit during shutdown/restart MySQL_Thread *mysql_thr=new MySQL_Thread(); mysql_thr->curtime=monotonic_time(); diff --git a/lib/MySQL_Logger.cpp b/lib/MySQL_Logger.cpp index 580ae7636..749d7466a 100644 --- a/lib/MySQL_Logger.cpp +++ b/lib/MySQL_Logger.cpp @@ -1519,7 +1519,7 @@ void MySQL_Logger::log_request(MySQL_Session *sess, MySQL_Data_Stream *myds, con sl+=strlen(sa); if (sl && myds && myds->myconn && myds->myconn->parent && myds->myconn->parent->port) { sa=(char *)malloc(sl+9); - sprintf(sa,"%s:%d", myds->myconn->parent->address, myds->myconn->parent->port); + snprintf(sa, sl+9, "%s:%d", myds->myconn->parent->address, myds->myconn->parent->port); } sl=strlen(sa); if (sl) { @@ -1562,7 +1562,7 @@ void MySQL_Logger::log_request(MySQL_Session *sess, MySQL_Data_Stream *myds, con if (cl && sess->client_myds->addr.port) { free(ca); } - if (sl && myds->myconn->parent->port) { + if (sl && myds && myds->myconn && myds->myconn->parent && myds->myconn->parent->port) { free(sa); } } @@ -1676,15 +1676,13 @@ void MySQL_Logger::log_audit_entry(log_event_type _et, MySQL_Session *sess, MySQ */ int sl=0; char *sa=(char *)""; // default - if (myds) { - if (myds->myconn) { - sa=myds->myconn->parent->address; - } + if (myds && myds->myconn && myds->myconn->parent) { + sa=myds->myconn->parent->address; } sl+=strlen(sa); - if (sl && myds->myconn->parent->port) { + if (sl && myds && myds->myconn && myds->myconn->parent && myds->myconn->parent->port) { sa=(char *)malloc(sl+9); - sprintf(sa,"%s:%d", myds->myconn->parent->address, myds->myconn->parent->port); + snprintf(sa, sl+9, "%s:%d", myds->myconn->parent->address, myds->myconn->parent->port); } sl=strlen(sa); @@ -1710,7 +1708,7 @@ void MySQL_Logger::log_audit_entry(log_event_type _et, MySQL_Session *sess, MySQ if (cl && sess->client_myds->addr.port) { free(ca); } - if (sl && myds->myconn->parent->port) { + if (sl && myds && myds->myconn && myds->myconn->parent && myds->myconn->parent->port) { free(sa); } } diff --git a/lib/MySQL_Monitor.cpp b/lib/MySQL_Monitor.cpp index 7350b213e..4ac945d3e 100644 --- a/lib/MySQL_Monitor.cpp +++ b/lib/MySQL_Monitor.cpp @@ -778,10 +778,8 @@ void * monitor_connect_pthread(void *arg) { mallctl("thread.tcache.enabled", NULL, NULL, &cache, sizeof(bool)); #endif set_thread_name("MonitorConnect", GloVars.set_thread_name); - while (GloMTH==NULL) { - usleep(50000); - } - usleep(100000); + // Wait for GloMTH to be initialized + if (!wait_for_glo_mth()) return NULL; // quick exit during shutdown/restart GloMyMon->monitor_connect(); return NULL; } @@ -792,10 +790,8 @@ void * monitor_ping_pthread(void *arg) { mallctl("thread.tcache.enabled", NULL, NULL, &cache, sizeof(bool)); #endif set_thread_name("MonitorPing", GloVars.set_thread_name); - while (GloMTH==NULL) { - usleep(50000); - } - usleep(100000); + // Wait for GloMTH to be initialized + if (!wait_for_glo_mth()) return NULL; // quick exit during shutdown/restart GloMyMon->monitor_ping(); return NULL; } @@ -806,10 +802,8 @@ void * monitor_read_only_pthread(void *arg) { mallctl("thread.tcache.enabled", NULL, NULL, &cache, sizeof(bool)); #endif set_thread_name("MonitorReadOnly", GloVars.set_thread_name); - while (GloMTH==NULL) { - usleep(50000); - } - usleep(100000); + // Wait for GloMTH to be initialized + if (!wait_for_glo_mth()) return NULL; // quick exit during shutdown/restart GloMyMon->monitor_read_only(); return NULL; } @@ -820,10 +814,8 @@ void * monitor_group_replication_pthread(void *arg) { mallctl("thread.tcache.enabled", NULL, NULL, &cache, sizeof(bool)); #endif set_thread_name("MonitorGR", GloVars.set_thread_name); - while (GloMTH==NULL) { - usleep(50000); - } - usleep(100000); + // Wait for GloMTH to be initialized + if (!wait_for_glo_mth()) return NULL; // quick exit during shutdown/restart // GloMyMon->monitor_group_replication(); GloMyMon->monitor_group_replication_2(); return NULL; @@ -835,10 +827,8 @@ void * monitor_galera_pthread(void *arg) { mallctl("thread.tcache.enabled", NULL, NULL, &cache, sizeof(bool)); #endif set_thread_name("MonitorGalera", GloVars.set_thread_name); - while (GloMTH==NULL) { - usleep(50000); - } - usleep(100000); + // Wait for GloMTH to be initialized + if (!wait_for_glo_mth()) return NULL; // quick exit during shutdown/restart GloMyMon->monitor_galera(); return NULL; } @@ -849,10 +839,8 @@ void * monitor_aws_aurora_pthread(void *arg) { // mallctl("thread.tcache.enabled", NULL, NULL, &cache, sizeof(bool)); //#endif set_thread_name("MonitorAurora", GloVars.set_thread_name); - while (GloMTH==NULL) { - usleep(50000); - } - usleep(100000); + // Wait for GloMTH to be initialized + if (!wait_for_glo_mth()) return NULL; // quick exit during shutdown/restart GloMyMon->monitor_aws_aurora(); return NULL; } @@ -863,10 +851,8 @@ void * monitor_replication_lag_pthread(void *arg) { mallctl("thread.tcache.enabled", NULL, NULL, &cache, sizeof(bool)); #endif set_thread_name("MonitReplicLag", GloVars.set_thread_name); - while (GloMTH==NULL) { - usleep(50000); - } - usleep(100000); + // Wait for GloMTH to be initialized + if (!wait_for_glo_mth()) return NULL; // quick exit during shutdown/restart GloMyMon->monitor_replication_lag(); return NULL; } @@ -1340,11 +1326,8 @@ void * monitor_connect_thread(const std::vector& mmsd mysql_close(mysql_init(NULL)); MySQL_Monitor_State_Data *mmsd = mmsds.front(); // Wait for GloMTH to be initialized - while (GloMTH==NULL) { - usleep(50000); - } - usleep(100000); - if (!GloMTH) return NULL; // quick exit during shutdown/restart + // Wait for GloMTH to be initialized + if (!wait_for_glo_mth()) return NULL; // quick exit during shutdown/restart MySQL_Thread * mysql_thr = new MySQL_Thread(); mysql_thr->curtime=monotonic_time(); mysql_thr->refresh_variables(); @@ -1403,11 +1386,8 @@ void * monitor_ping_thread(const std::vector& mmsds) assert(!mmsds.empty()); mysql_close(mysql_init(NULL)); MySQL_Monitor_State_Data *mmsd = mmsds.front(); - while (GloMTH==NULL) { - usleep(50000); - } - usleep(100000); - if (!GloMTH) return NULL; // quick exit during shutdown/restart + // Wait for GloMTH to be initialized + if (!wait_for_glo_mth()) return NULL; // quick exit during shutdown/restart MySQL_Thread * mysql_thr = new MySQL_Thread(); mysql_thr->curtime=monotonic_time(); mysql_thr->refresh_variables(); @@ -1640,11 +1620,8 @@ void * monitor_read_only_thread(const std::vector& da mysql_close(mysql_init(NULL)); bool timeout_reached = false; MySQL_Monitor_State_Data *mmsd = data.front(); - while (GloMTH==NULL) { - usleep(50000); - } - usleep(100000); - if (!GloMTH) return NULL; // quick exit during shutdown/restart + // Wait for GloMTH to be initialized + if (!wait_for_glo_mth()) return NULL; // quick exit during shutdown/restart MySQL_Thread * mysql_thr = new MySQL_Thread(); mysql_thr->curtime=monotonic_time(); mysql_thr->refresh_variables(); @@ -1922,11 +1899,8 @@ void * monitor_group_replication_thread(const std::vectorcurtime=monotonic_time(); mysql_thr->refresh_variables(); @@ -2278,11 +2252,8 @@ void * monitor_galera_thread(const std::vector& data) assert(!data.empty()); mysql_close(mysql_init(NULL)); MySQL_Monitor_State_Data *mmsd = data.front(); - while (GloMTH==NULL) { - usleep(50000); - } - usleep(100000); - if (!GloMTH) return NULL; // quick exit during shutdown/restart + // Wait for GloMTH to be initialized + if (!wait_for_glo_mth()) return NULL; // quick exit during shutdown/restart MySQL_Thread * mysql_thr = new MySQL_Thread(); mysql_thr->curtime=monotonic_time(); mysql_thr->refresh_variables(); @@ -2683,11 +2654,8 @@ void * monitor_replication_lag_thread(const std::vectorcurtime=monotonic_time(); mysql_thr->refresh_variables(); @@ -3048,11 +3016,8 @@ __fast_exit_monitor_replication_lag_thread: void * MySQL_Monitor::monitor_connect() { // initialize the MySQL Thread (note: this is not a real thread, just the structures associated with it) - while (GloMTH==NULL) { - usleep(50000); - } - usleep(100000); - if (!GloMTH) return NULL; // quick exit during shutdown/restart + // Wait for GloMTH to be initialized + if (!wait_for_glo_mth()) return NULL; // quick exit during shutdown/restart unsigned int MySQL_Monitor__thread_MySQL_Thread_Variables_version; MySQL_Thread * mysql_thr = new MySQL_Thread(); mysql_thr->curtime=monotonic_time(); @@ -3177,11 +3142,8 @@ __sleep_monitor_connect_loop: void * MySQL_Monitor::monitor_ping() { mysql_close(mysql_init(NULL)); // initialize the MySQL Thread (note: this is not a real thread, just the structures associated with it) - while (GloMTH==NULL) { - usleep(50000); - } - usleep(100000); - if (!GloMTH) return NULL; // quick exit during shutdown/restart + // Wait for GloMTH to be initialized + if (!wait_for_glo_mth()) return NULL; // quick exit during shutdown/restart // struct event_base *libevent_base; unsigned int MySQL_Monitor__thread_MySQL_Thread_Variables_version; MySQL_Thread * mysql_thr = new MySQL_Thread(); @@ -3529,11 +3491,8 @@ bool MySQL_Monitor::is_aws_rds_multi_az_db_cluster_topology(const std::vectorcurtime=monotonic_time(); @@ -4157,10 +4116,8 @@ void async_gr_mon_actions_handler(MySQL_Monitor_State_Data* mmsd) { * @return The created and initialized 'MySQL_Thread'. */ unique_ptr init_mysql_thread_struct() { - while (GloMTH==NULL) { - usleep(50000); - } - usleep(100000); + // Wait for GloMTH to be initialized + if (!wait_for_glo_mth()) return NULL; // quick exit during shutdown/restart if (!GloMTH) return NULL; unique_ptr mysql_thr { new MySQL_Thread() }; mysql_thr->curtime = monotonic_time(); @@ -4407,11 +4364,8 @@ void* MySQL_Monitor::monitor_group_replication_2() { void * MySQL_Monitor::monitor_group_replication() { mysql_close(mysql_init(NULL)); // initialize the MySQL Thread (note: this is not a real thread, just the structures associated with it) - while (GloMTH==NULL) { - usleep(50000); - } - usleep(100000); - if (!GloMTH) return NULL; // quick exit during shutdown/restart + // Wait for GloMTH to be initialized + if (!wait_for_glo_mth()) return NULL; // quick exit during shutdown/restart // struct event_base *libevent_base; unsigned int MySQL_Monitor__thread_MySQL_Thread_Variables_version; MySQL_Thread * mysql_thr = new MySQL_Thread(); @@ -4518,11 +4472,8 @@ __sleep_monitor_group_replication: void * MySQL_Monitor::monitor_galera() { mysql_close(mysql_init(NULL)); // initialize the MySQL Thread (note: this is not a real thread, just the structures associated with it) - while (GloMTH==NULL) { - usleep(50000); - } - usleep(100000); - if (!GloMTH) return NULL; // quick exit during shutdown/restart + // Wait for GloMTH to be initialized + if (!wait_for_glo_mth()) return NULL; // quick exit during shutdown/restart // struct event_base *libevent_base; unsigned int MySQL_Monitor__thread_MySQL_Thread_Variables_version; MySQL_Thread * mysql_thr = new MySQL_Thread(); @@ -4593,11 +4544,8 @@ __sleep_monitor_galera: void * MySQL_Monitor::monitor_replication_lag() { mysql_close(mysql_init(NULL)); // initialize the MySQL Thread (note: this is not a real thread, just the structures associated with it) - while (GloMTH==NULL) { - usleep(50000); - } - usleep(100000); - if (!GloMTH) return NULL; // quick exit during shutdown/restart + // Wait for GloMTH to be initialized + if (!wait_for_glo_mth()) return NULL; // quick exit during shutdown/restart unsigned int MySQL_Monitor__thread_MySQL_Thread_Variables_version; MySQL_Thread * mysql_thr = new MySQL_Thread(); mysql_thr->curtime=monotonic_time(); @@ -5167,10 +5115,8 @@ void* MySQL_Monitor::monitor_dns_cache() { void * MySQL_Monitor::run() { - while (GloMTH==NULL) { - usleep(50000); - } - usleep(100000); + // Wait for GloMTH to be initialized + if (!wait_for_glo_mth()) return NULL; // quick exit during shutdown/restart // initialize the MySQL Thread (note: this is not a real thread, just the structures associated with it) unsigned int MySQL_Monitor__thread_MySQL_Thread_Variables_version; MySQL_Thread * mysql_thr = new MySQL_Thread(); @@ -6062,11 +6008,8 @@ void * monitor_AWS_Aurora_thread_HG(void *arg) { set_thread_name("MonitorAuroraHG", GloVars.set_thread_name); proxy_info("Started Monitor thread for AWS Aurora writer HG %u\n", wHG); - while (GloMTH==NULL) { - usleep(50000); - } - usleep(100000); - if (!GloMTH) return NULL; // quick exit during shutdown/restart + // Wait for GloMTH to be initialized + if (!wait_for_glo_mth()) return NULL; // quick exit during shutdown/restart unsigned int MySQL_Monitor__thread_MySQL_Thread_Variables_version; MySQL_Thread * mysql_thr = new MySQL_Thread(); mysql_thr->curtime=monotonic_time(); @@ -6520,11 +6463,8 @@ __exit_monitor_AWS_Aurora_thread_HG_now: void * MySQL_Monitor::monitor_aws_aurora() { // initialize the MySQL Thread (note: this is not a real thread, just the structures associated with it) - while (GloMTH==NULL) { - usleep(50000); - } - usleep(100000); - if (!GloMTH) return NULL; // quick exit during shutdown/restart + // Wait for GloMTH to be initialized + if (!wait_for_glo_mth()) return NULL; // quick exit during shutdown/restart unsigned int MySQL_Monitor__thread_MySQL_Thread_Variables_version; MySQL_Thread * mysql_thr = new MySQL_Thread(); mysql_thr->curtime=monotonic_time(); diff --git a/lib/ProxySQL_Admin.cpp b/lib/ProxySQL_Admin.cpp index d700e9544..d52096852 100644 --- a/lib/ProxySQL_Admin.cpp +++ b/lib/ProxySQL_Admin.cpp @@ -2099,10 +2099,8 @@ void *child_mysql(void *arg) { nfds_t nfds=1; int rc; pthread_mutex_unlock(&sock_mutex); - while (GloMTH==NULL) { - usleep(50000); - } - usleep(100000); + // Wait for GloMTH to be initialized + if (!wait_for_glo_mth()) return NULL; // quick exit during shutdown/restart if (!GloMTH) return NULL; MySQL_Thread *mysql_thr=new MySQL_Thread(); mysql_thr->curtime=monotonic_time(); diff --git a/lib/debug.cpp b/lib/debug.cpp index cebcd0048..0eaa52b97 100644 --- a/lib/debug.cpp +++ b/lib/debug.cpp @@ -524,10 +524,9 @@ void print_backtrace(void) } #ifdef DEBUG -void init_debug_struct() { +void init_debug_struct() { int i; pthread_mutex_init(&debug_mutex,NULL); - pthread_rwlock_init(&filters_rwlock, NULL); GloVars.global.gdbg_lvl= (debug_level *) malloc(PROXY_DEBUG_UNKNOWN*sizeof(debug_level)); for (i=0;icurtime=monotonic_time();