From 6f2798abe0afa4198704c67b2f572880b254e917 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javier=20Jaramago=20Fern=C3=A1ndez?= Date: Wed, 25 Jun 2025 12:31:04 +0200 Subject: [PATCH 1/2] Fix memory leak in 'ProxySQL_Admin::stats___mysql_global' - Closes #4976 The resource leak was associated to prepared statements (sqlite3_stmt). Since the detection of these leaks is tricky, because the resources held by 'SQLite3' never lose their references, a helper type ('stmt_unique_ptr') and a safer variation for 'prepare_v2' have been introduced. This helper type should automatically handle the lifetime of the statements, avoiding these kind of leaks in the future. The non-safe alternative has been flagged as 'deprecated', so it can be easily identified in later refactors or slowly removed during development. --- include/sqlite3db.h | 21 +++++++++++++++++++++ lib/ProxySQL_Admin_Stats.cpp | 13 +++++++++---- lib/sqlite3db.cpp | 20 ++++++++++++++++++++ 3 files changed, 50 insertions(+), 4 deletions(-) diff --git a/include/sqlite3db.h b/include/sqlite3db.h index 9aa2ac5f85..679aa4e5f4 100644 --- a/include/sqlite3db.h +++ b/include/sqlite3db.h @@ -6,7 +6,9 @@ #undef min #undef max #include +#include #include +#include #define PROXYSQL_SQLITE3DB_PTHREAD_MUTEX #ifndef SAFE_SQLITE3_STEP2 @@ -165,6 +167,18 @@ class SQLite3_result { void dump_to_stderr(); }; +/** + * @brief Helper type for finalizing 'sqlite3_stmt' managed by smart pointers. + */ +struct stmt_deleter_t { + void operator()(sqlite3_stmt* x) const; +}; + +/** + * @brief Safe type for automatically deallocation of 'sqlite3_stmt'. + */ +using stmt_unique_ptr = std::unique_ptr; + class SQLite3DB { private: char *url; @@ -191,7 +205,14 @@ class SQLite3DB { int check_table_structure(char *table_name, char *table_def); bool build_table(char *table_name, char *table_def, bool dropit); bool check_and_build_table(char *table_name, char *table_def); + [[deprecated("Use safer alternative 'prepare_v2(const char *)'")]] int prepare_v2(const char *, sqlite3_stmt **); + /** + * @brief Prepares a query as a statement in the SQLite3DB. + * @param query The query to be prepared as an 'sqlite3_stmt'. + * @return A pair of with shape { err_code, stmt_unique_ptr }. + */ + std::pair prepare_v2(const char* query); static void LoadPlugin(const char *); }; diff --git a/lib/ProxySQL_Admin_Stats.cpp b/lib/ProxySQL_Admin_Stats.cpp index c1fdaae0ce..08069d3a94 100644 --- a/lib/ProxySQL_Admin_Stats.cpp +++ b/lib/ProxySQL_Admin_Stats.cpp @@ -521,12 +521,17 @@ void ProxySQL_Admin::stats___mysql_global() { "INSERT INTO stats_mysql_global VALUES " + generate_multi_rows_query(32, 2) }; - sqlite3_stmt* row_stmt = nullptr; - int rc = statsdb->prepare_v2(q_row_insert.c_str(), &row_stmt); + int rc = 0; + + stmt_unique_ptr u_row_stmt { nullptr }; + std::tie(rc, u_row_stmt) = statsdb->prepare_v2(q_row_insert.c_str()); ASSERT_SQLITE_OK(rc, statsdb); - sqlite3_stmt* bulk_stmt = nullptr; - rc = statsdb->prepare_v2(q_bulk_insert.c_str(), &bulk_stmt); + sqlite3_stmt* const row_stmt { u_row_stmt.get() }; + + stmt_unique_ptr u_bulk_stmt { nullptr }; + std::tie(rc, u_bulk_stmt) = statsdb->prepare_v2(q_bulk_insert.c_str()); ASSERT_SQLITE_OK(rc, statsdb); + sqlite3_stmt* const bulk_stmt { u_bulk_stmt.get() }; sqlite3_bulk_step(statsdb, row_stmt, bulk_stmt, resultset, stats_mysql_global___bind_row); diff --git a/lib/sqlite3db.cpp b/lib/sqlite3db.cpp index 7dee44aaad..3b435ac536 100644 --- a/lib/sqlite3db.cpp +++ b/lib/sqlite3db.cpp @@ -260,6 +260,26 @@ int SQLite3DB::prepare_v2(const char *str, sqlite3_stmt **statement) { return rc; } +void stmt_deleter_t::operator()(sqlite3_stmt* x) const { + proxy_sqlite3_finalize(x); +} + +std::pair SQLite3DB::prepare_v2(const char* query) { + int rc { 0 }; + sqlite3_stmt* stmt { nullptr }; + + do { + rc = (*proxy_sqlite3_prepare_v2)(db, query, -1, &stmt, nullptr); + + if (rc==SQLITE_LOCKED || rc==SQLITE_BUSY) { + struct timespec ts { .tv_sec = 0, .tv_nsec = USLEEP_SQLITE_LOCKED * 1000 }; + nanosleep(&ts, nullptr); + } + } while (rc==SQLITE_LOCKED || rc==SQLITE_BUSY); + + return { rc, stmt_unique_ptr(stmt) }; +} + /** * @brief Executes a SQL statement and returns the result set. * From 25d5681c617df4f961812cf11f45082ebf51fffb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javier=20Jaramago=20Fern=C3=A1ndez?= Date: Tue, 1 Jul 2025 17:21:39 +0200 Subject: [PATCH 2/2] Fix potential overflow for 'sqlite3_status' memory metrics The previous call have been replaced by 'sqlite3_status64' when querying 'SQLITE_STATUS_MEMORY_USED' to avoid potential integer overflows. --- include/sqlite3db.h | 3 +++ lib/ProxySQL_Admin_Stats.cpp | 28 +++++++++++++++------------- lib/sqlite3db.cpp | 3 +++ 3 files changed, 21 insertions(+), 13 deletions(-) diff --git a/include/sqlite3db.h b/include/sqlite3db.h index 679aa4e5f4..ad9d0107b3 100644 --- a/include/sqlite3db.h +++ b/include/sqlite3db.h @@ -42,6 +42,7 @@ extern int (*proxy_sqlite3_close_v2)(sqlite3*); extern int (*proxy_sqlite3_get_autocommit)(sqlite3*); extern void (*proxy_sqlite3_free)(void*); extern int (*proxy_sqlite3_status)(int op, int *pCurrent, int *pHighwater, int resetFlag); +extern int (*proxy_sqlite3_status64)(int op, long long *pCurrent, long long *pHighwater, int resetFlag); extern int (*proxy_sqlite3_changes)(sqlite3*); extern int (*proxy_sqlite3_step)(sqlite3_stmt*); extern int (*proxy_sqlite3_config)(int, ...); @@ -89,6 +90,8 @@ int (*proxy_sqlite3_close_v2)(sqlite3*); int (*proxy_sqlite3_get_autocommit)(sqlite3*); void (*proxy_sqlite3_free)(void*); int (*proxy_sqlite3_status)(int op, int *pCurrent, int *pHighwater, int resetFlag); +int (*proxy_sqlite3_status64)(int op, long long *pCurrent, long long *pHighwater, int resetFlag); + int (*proxy_sqlite3_changes)(sqlite3*); int (*proxy_sqlite3_step)(sqlite3_stmt*); int (*proxy_sqlite3_config)(int, ...); diff --git a/lib/ProxySQL_Admin_Stats.cpp b/lib/ProxySQL_Admin_Stats.cpp index 08069d3a94..e5a7d750a5 100644 --- a/lib/ProxySQL_Admin_Stats.cpp +++ b/lib/ProxySQL_Admin_Stats.cpp @@ -109,9 +109,9 @@ void ProxySQL_Admin::p_stats___memory_metrics() { this->metrics.p_gauge_array[p_admin_gauge::connpool_memory_bytes]->Set(connpool_mem); // proxysql_sqlite3_memory_bytes metric - int highwater = 0; - int current = 0; - (*proxy_sqlite3_status)(SQLITE_STATUS_MEMORY_USED, ¤t, &highwater, 0); + long long highwater = 0; + long long current = 0; + (*proxy_sqlite3_status64)(SQLITE_STATUS_MEMORY_USED, ¤t, &highwater, 0); this->metrics.p_gauge_array[p_admin_gauge::sqlite3_memory_bytes]->Set(current); // proxysql_jemalloc_* memory metrics @@ -206,8 +206,8 @@ void ProxySQL_Admin::stats___memory_metrics() { if (!GloMTH) return; SQLite3_result * resultset = NULL; - int highwater; - int current; + long long highwater = 0; + long long current = 0; char bu[32]; char *vn=NULL; char *query=NULL; @@ -218,9 +218,9 @@ void ProxySQL_Admin::stats___memory_metrics() { delete resultset; resultset=NULL; } - (*proxy_sqlite3_status)(SQLITE_STATUS_MEMORY_USED, ¤t, &highwater, 0); + (*proxy_sqlite3_status64)(SQLITE_STATUS_MEMORY_USED, ¤t, &highwater, 0); vn=(char *)"SQLite3_memory_bytes"; - sprintf(bu,"%d",current); + sprintf(bu,"%lld",current); query=(char *)malloc(strlen(a)+strlen(vn)+strlen(bu)+16); sprintf(query,a,vn,bu); statsdb->execute(query); @@ -492,6 +492,8 @@ const void sqlite3_global_stats_row_step( sprintf(buf, "%lu", val); } else if constexpr (std::is_same_v) { sprintf(buf, "%llu", val); + } else if constexpr (std::is_same_v) { + sprintf(buf, "%lld", val); } else if constexpr (std::is_same_v) { sprintf(buf, "%s", val ? "true" : "false"); } else { @@ -547,8 +549,8 @@ void ProxySQL_Admin::stats___mysql_global() { } { - int highwater, current = 0; - (*proxy_sqlite3_status)(SQLITE_STATUS_MEMORY_USED, ¤t, &highwater, 0); + long long highwater, current = 0; + (*proxy_sqlite3_status64)(SQLITE_STATUS_MEMORY_USED, ¤t, &highwater, 0); sqlite3_global_stats_row_step(statsdb, row_stmt, "SQLite3_memory_bytes", current); } @@ -652,14 +654,14 @@ void ProxySQL_Admin::stats___pgsql_global() { resultset = NULL; } - int highwater; - int current; - (*proxy_sqlite3_status)(SQLITE_STATUS_MEMORY_USED, ¤t, &highwater, 0); + long long highwater = 0; + long long current = 0; + (*proxy_sqlite3_status64)(SQLITE_STATUS_MEMORY_USED, ¤t, &highwater, 0); char bu[32]; char* vn = NULL; char* query = NULL; vn = (char*)"SQLite3_memory_bytes"; - sprintf(bu, "%d", current); + sprintf(bu, "%lld", current); query = (char*)malloc(strlen(a) + strlen(vn) + strlen(bu) + 16); sprintf(query, a, vn, bu); statsdb->execute(query); diff --git a/lib/sqlite3db.cpp b/lib/sqlite3db.cpp index 3b435ac536..3acae23c09 100644 --- a/lib/sqlite3db.cpp +++ b/lib/sqlite3db.cpp @@ -1008,6 +1008,7 @@ void SQLite3DB::LoadPlugin(const char *plugin_name) { proxy_sqlite3_get_autocommit = NULL; proxy_sqlite3_free = NULL; proxy_sqlite3_status = NULL; + proxy_sqlite3_status64 = NULL; proxy_sqlite3_changes = NULL; proxy_sqlite3_step = NULL; proxy_sqlite3_shutdown = NULL; @@ -1086,6 +1087,7 @@ void SQLite3DB::LoadPlugin(const char *plugin_name) { proxy_sqlite3_get_autocommit = sqlite3_get_autocommit; proxy_sqlite3_free = sqlite3_free; proxy_sqlite3_status = sqlite3_status; + proxy_sqlite3_status64 = sqlite3_status64; proxy_sqlite3_changes = sqlite3_changes; proxy_sqlite3_step = sqlite3_step; proxy_sqlite3_shutdown = sqlite3_shutdown; @@ -1114,6 +1116,7 @@ void SQLite3DB::LoadPlugin(const char *plugin_name) { assert(proxy_sqlite3_get_autocommit); assert(proxy_sqlite3_free); assert(proxy_sqlite3_status); + assert(proxy_sqlite3_status64); assert(proxy_sqlite3_changes); assert(proxy_sqlite3_step); assert(proxy_sqlite3_shutdown);