From f33367f2ab19e2cc8f1af1875cbc33d6980700a1 Mon Sep 17 00:00:00 2001 From: Monty Date: Thu, 29 May 2025 18:09:16 +0300 Subject: [PATCH] Fixed a LOT of memory leaks in mariabackup This was generally good to get done but also needed to be able to run mariabackup test under asan. Things freed: - Allocated variables (mysql_tmpdir_list, opt_passwd etc) - InnoDB variables - Results from SQL queries (A lot of sql queries did not free their result) - Allocated sys_vars - Server variables (mysql_server_end()) - Memory allocated by plugins (encryption) - Free variables allocated by my_default. (Old code had a bug that caused these to not be freed) Other things: - Moved freeing of mysql_tmpdir_list to main, as the old code did not free the last mysqltmp_dir allocation. Now we also initialize the variable only once. - Fixed a serious, potentially 'crashing at end' bug where we called free_defaults() with wrong pointers. - Fixed a bug related to update_malloc_size() where we did not take into account the it was not changed. - Fixed a bug in Sys_var_charptr_base where we did not allocate default values. This could lead to trying to free not allocated values in xtrabackup. - Added sf_have_memory_leak() to be able to easily check if there was a memory leak when using safemalloc() - sf_report_leaked_memory() now returns 1 if a memory leak was found. --- extra/mariabackup/backup_mysql.cc | 36 +++++++++++------------ extra/mariabackup/backup_mysql.h | 1 - extra/mariabackup/xtrabackup.cc | 47 +++++++++++++++++++++++++------ include/my_sys.h | 5 +++- mysys/my_malloc.c | 6 ++-- mysys/safemalloc.c | 11 ++++++-- sql/sys_vars.inl | 5 +++- sql/wsrep_var.cc | 9 ++++++ 8 files changed, 84 insertions(+), 36 deletions(-) diff --git a/extra/mariabackup/backup_mysql.cc b/extra/mariabackup/backup_mysql.cc index 04d5ce50deb..6537d05db34 100644 --- a/extra/mariabackup/backup_mysql.cc +++ b/extra/mariabackup/backup_mysql.cc @@ -77,6 +77,7 @@ bool have_lock_wait_timeout = false; bool have_galera_enabled = false; bool have_multi_threaded_slave = false; bool have_gtid_slave = false; +bool innobase_data_file_path_allocated= false; /* Kill long selects */ static mysql_mutex_t kill_query_thread_mutex; @@ -498,21 +499,19 @@ bool get_mysql_vars(MYSQL *connection) } if (innodb_data_file_path_var && *innodb_data_file_path_var) - innobase_data_file_path= my_strdup(PSI_NOT_INSTRUMENTED, - innodb_data_file_path_var, MYF(MY_FAE)); + innobase_data_file_path= my_once_strdup(innodb_data_file_path_var, + MYF(MY_FAE)); if (innodb_data_home_dir_var) - innobase_data_home_dir= my_strdup(PSI_NOT_INSTRUMENTED, - innodb_data_home_dir_var, MYF(MY_FAE)); + innobase_data_home_dir= my_once_strdup(innodb_data_home_dir_var, + MYF(MY_FAE)); if (innodb_log_group_home_dir_var && *innodb_log_group_home_dir_var) - srv_log_group_home_dir= my_strdup(PSI_NOT_INSTRUMENTED, - innodb_log_group_home_dir_var, - MYF(MY_FAE)); + srv_log_group_home_dir= my_once_strdup(innodb_log_group_home_dir_var, + MYF(MY_FAE)); if (innodb_undo_directory_var && *innodb_undo_directory_var) - srv_undo_dir= my_strdup(PSI_NOT_INSTRUMENTED, innodb_undo_directory_var, - MYF(MY_FAE)); + srv_undo_dir= my_once_strdup(innodb_undo_directory_var, MYF(MY_FAE)); if (innodb_log_file_size_var) { @@ -534,10 +533,7 @@ bool get_mysql_vars(MYSQL *connection) } if (aria_log_dir_path_var) - { - aria_log_dir_path= my_strdup(PSI_NOT_INSTRUMENTED, - aria_log_dir_path_var, MYF(MY_FAE)); - } + aria_log_dir_path= my_once_strdup(aria_log_dir_path_var, MYF(MY_FAE)); if (page_zip_level_var != NULL) { @@ -550,11 +546,11 @@ bool get_mysql_vars(MYSQL *connection) xb_load_list_string(ignore_db_dirs, ",", register_ignore_db_dirs_filter); out: - free_mysql_variables(mysql_vars); return (ret); } + static bool select_incremental_lsn_from_history(lsn_t *incremental_lsn) @@ -930,7 +926,7 @@ lock_for_backup_stage_flush(MYSQL *connection) { if (opt_kill_long_queries_timeout) { start_query_killer(); } - xb_mysql_query(connection, "BACKUP STAGE FLUSH", true); + xb_mysql_query(connection, "BACKUP STAGE FLUSH", false); if (opt_kill_long_queries_timeout) { stop_query_killer(); } @@ -942,7 +938,7 @@ lock_for_backup_stage_block_ddl(MYSQL *connection) { if (opt_kill_long_queries_timeout) { start_query_killer(); } - xb_mysql_query(connection, "BACKUP STAGE BLOCK_DDL", true); + xb_mysql_query(connection, "BACKUP STAGE BLOCK_DDL", false); DBUG_MARIABACKUP_EVENT("after_backup_stage_block_ddl", {}); if (opt_kill_long_queries_timeout) { stop_query_killer(); @@ -955,7 +951,7 @@ lock_for_backup_stage_commit(MYSQL *connection) { if (opt_kill_long_queries_timeout) { start_query_killer(); } - xb_mysql_query(connection, "BACKUP STAGE BLOCK_COMMIT", true); + xb_mysql_query(connection, "BACKUP STAGE BLOCK_COMMIT", false); DBUG_MARIABACKUP_EVENT("after_backup_stage_block_commit", {}); if (opt_kill_long_queries_timeout) { stop_query_killer(); @@ -966,12 +962,12 @@ lock_for_backup_stage_commit(MYSQL *connection) { bool backup_lock(MYSQL *con, const char *table_name) { static const std::string backup_lock_prefix("BACKUP LOCK "); std::string backup_lock_query = backup_lock_prefix + table_name; - xb_mysql_query(con, backup_lock_query.c_str(), true); + xb_mysql_query(con, backup_lock_query.c_str(), false); return true; } bool backup_unlock(MYSQL *con) { - xb_mysql_query(con, "BACKUP UNLOCK", true); + xb_mysql_query(con, "BACKUP UNLOCK", false); return true; } @@ -985,6 +981,8 @@ get_tables_in_use(MYSQL *con) { msg("Table %s is in use", tk.c_str()); result.insert(std::move(tk)); } + if (q_res) + mysql_free_result(q_res); return result; } diff --git a/extra/mariabackup/backup_mysql.h b/extra/mariabackup/backup_mysql.h index ce7755d17af..55700dddf6d 100644 --- a/extra/mariabackup/backup_mysql.h +++ b/extra/mariabackup/backup_mysql.h @@ -97,5 +97,4 @@ bool write_slave_info(ds_ctxt *datasink, MYSQL *connection); ulonglong get_current_lsn(MYSQL *connection); - #endif diff --git a/extra/mariabackup/xtrabackup.cc b/extra/mariabackup/xtrabackup.cc index 9609a3fc82c..047b1fe5fa7 100644 --- a/extra/mariabackup/xtrabackup.cc +++ b/extra/mariabackup/xtrabackup.cc @@ -129,6 +129,7 @@ int sd_notifyf() { return 0; } } int sys_var_init(); +void sys_var_end(); extern const char* fts_common_tables[]; extern const fts_index_selector_t fts_index_selector[]; @@ -396,6 +397,7 @@ char *opt_incremental_history_uuid; char *opt_user; const char *opt_password; +bool free_opt_password; char *opt_host; char *opt_defaults_group; char *opt_socket; @@ -2456,6 +2458,7 @@ xb_get_one_option(const struct my_option *opt, static bool innodb_init_param() { + static bool mysql_tmpdir_list_set= 0; if (!ut_is_2pow(log_sys.write_size)) { msg("InnoDB: innodb_log_write_ahead_size=%u" " is not a power of two", log_sys.write_size); @@ -2463,12 +2466,15 @@ static bool innodb_init_param() } srv_is_being_started = TRUE; /* === some variables from mysqld === */ - memset((G_PTR) &mysql_tmpdir_list, 0, sizeof(mysql_tmpdir_list)); - - if (init_tmpdir(&mysql_tmpdir_list, opt_mysql_tmpdir)) { - msg("init_tmpdir() failed"); - return true; - } + if (!mysql_tmpdir_list_set) + { + mysql_tmpdir_list_set= 1; + memset((G_PTR) &mysql_tmpdir_list, 0, sizeof(mysql_tmpdir_list)); + if (init_tmpdir(&mysql_tmpdir_list, opt_mysql_tmpdir)) { + msg("init_tmpdir() failed"); + return true; + } + } xtrabackup_tmpdir = my_tmpdir(&mysql_tmpdir_list); /* dummy for initialize all_charsets[] */ get_charset_name(0); @@ -6628,7 +6634,6 @@ void innodb_free_param() { srv_sys_space.shutdown(); - free_tmpdir(&mysql_tmpdir_list); } @@ -7283,6 +7288,8 @@ order: void handle_options(int argc, char **argv, char ***argv_server, char ***argv_client, char ***argv_backup) { + char **save_argv_server, **save_argv_client, **save_argv_backup; + /* Setup some variables for Innodb.*/ srv_operation = SRV_OPERATION_RESTORE; @@ -7400,6 +7407,7 @@ void handle_options(int argc, char **argv, char ***argv_server, load_defaults_or_exit(conf_file, &server_default_groups[0], &argc_server, argv_server); + save_argv_server= *argv_server; int n; for (n = 0; (*argv_server)[n]; n++) {}; @@ -7445,6 +7453,7 @@ void handle_options(int argc, char **argv, char ***argv_server, load_defaults_or_exit(conf_file, xb_client_default_groups, &argc_client, argv_client); + save_argv_client= *argv_client; for (n = 0; (*argv_client)[n]; n++) {}; argc_client = n; @@ -7465,6 +7474,8 @@ void handle_options(int argc, char **argv, char ***argv_server, load_defaults_or_exit(conf_file, backup_default_groups, &argc_backup, argv_backup); + save_argv_backup= *argv_backup; + for (n= 0; (*argv_backup)[n]; n++) { }; @@ -7498,6 +7509,7 @@ void handle_options(int argc, char **argv, char ***argv_server, char *start= (char*) opt_password; opt_password= my_strdup(PSI_NOT_INSTRUMENTED, opt_password, MYF(MY_FAE)); + free_opt_password= 1; while (*argument) *argument++= 'x'; // Destroy argument if (*start) @@ -7543,6 +7555,14 @@ void handle_options(int argc, char **argv, char ***argv_server, } } } + /* + Restore load defaults argument to the value after + load_defaults_or_exit(). This is needed for caller + when calling free_defaults() + */ + *argv_server= save_argv_server; + *argv_client= save_argv_client; + *argv_backup= save_argv_backup; } static int main_low(char** argv); @@ -7641,10 +7661,21 @@ int main(int argc, char **argv) cleanup_errmsgs(); free_error_messages(); mysql_mutex_destroy(&LOCK_error_log); + free_tmpdir(&mysql_tmpdir_list); + if (free_opt_password) + my_free((char*) opt_password); + plugin_shutdown(); + free_list(opt_plugin_load_list_ptr); + mysql_server_end(); + sys_var_end(); if (status == EXIT_SUCCESS) { - msg("completed OK!"); + msg("completed OK!"); } + my_end(MY_CHECK_ERROR); + sf_leaking_memory= 0; + if (SAFEMALLOC_HAVE_MEMORY_LEAK) + status= EXIT_FAILURE; return status; } diff --git a/include/my_sys.h b/include/my_sys.h index 23799f661c4..ec760c7bd19 100644 --- a/include/my_sys.h +++ b/include/my_sys.h @@ -152,12 +152,15 @@ char *guess_malloc_library(); /* If we have our own safemalloc (for debugging) */ #if defined(SAFEMALLOC) -void sf_report_leaked_memory(my_thread_id id); +my_bool sf_report_leaked_memory(my_thread_id id); int sf_sanity(); +my_bool sf_have_memory_leak(); extern my_thread_id (*sf_malloc_dbug_id)(void); #define SAFEMALLOC_REPORT_MEMORY(X) if (!sf_leaking_memory) sf_report_leaked_memory(X) +#define SAFEMALLOC_HAVE_MEMORY_LEAK sf_have_memory_leak() #else #define SAFEMALLOC_REPORT_MEMORY(X) do {} while(0) +#define SAFEMALLOC_HAVE_MEMORY_LEAK 0 #endif typedef void (*MALLOC_SIZE_CB) (long long size, my_bool is_thread_specific); diff --git a/mysys/my_malloc.c b/mysys/my_malloc.c index 5a5ed6c8283..076165b56fe 100644 --- a/mysys/my_malloc.c +++ b/mysys/my_malloc.c @@ -107,7 +107,7 @@ void *my_malloc(PSI_memory_key key, size_t size, myf my_flags) int flag= MY_TEST(my_flags & MY_THREAD_SPECIFIC); mh->m_size= size | flag; mh->m_key= PSI_CALL_memory_alloc(key, size, & mh->m_owner); - if (update_malloc_size) + if (update_malloc_size != dummy) { mh->m_size|=2; update_malloc_size(size + HEADER_SIZE, flag); @@ -176,7 +176,7 @@ void *my_realloc(PSI_memory_key key, void *old_point, size_t size, myf my_flags) { mh->m_size= size | old_flags; mh->m_key= PSI_CALL_memory_realloc(key, old_size, size, & mh->m_owner); - if (update_malloc_size && (old_flags & 2)) + if (update_malloc_size != dummy && (old_flags & 2)) update_malloc_size((longlong)size - (longlong)old_size, old_flags & 1); point= HEADER_TO_USER(mh); } @@ -207,7 +207,7 @@ void my_free(void *ptr) old_flags= mh->m_size & 3; PSI_CALL_memory_free(mh->m_key, old_size, mh->m_owner); - if (update_malloc_size && (old_flags & 2)) + if (update_malloc_size != dummy && (old_flags & 2)) update_malloc_size(- (longlong) old_size - HEADER_SIZE, old_flags & 1); #ifndef SAFEMALLOC diff --git a/mysys/safemalloc.c b/mysys/safemalloc.c index 2fc34a9231b..727d004423d 100644 --- a/mysys/safemalloc.c +++ b/mysys/safemalloc.c @@ -372,7 +372,7 @@ int sf_sanity() if (count || irem) { warn("Error: Safemalloc link list destroyed"); - flag= 1; + flag++; } return flag; } @@ -383,7 +383,7 @@ int sf_sanity() @param id Id of thread to report. 0 if all */ -void sf_report_leaked_memory(my_thread_id id) +my_bool sf_report_leaked_memory(my_thread_id id) { size_t total= 0; struct st_irem *irem; @@ -411,7 +411,7 @@ void sf_report_leaked_memory(my_thread_id id) if (total) fprintf(stderr, "Memory lost: %lu bytes in %u chunks of %u total chunks\n", (ulong) total, chunks, sf_malloc_count); - return; + return total != 0; } static void sf_terminate() @@ -422,4 +422,9 @@ static void sf_terminate() pthread_mutex_destroy(&sf_mutex); } +my_bool sf_have_memory_leak() +{ + return sf_malloc_root != 0; +} + #endif diff --git a/sql/sys_vars.inl b/sql/sys_vars.inl index 49a9d5b637a..0094751ad91 100644 --- a/sql/sys_vars.inl +++ b/sql/sys_vars.inl @@ -520,7 +520,10 @@ public: (think of an exit because of an error right after my_getopt) */ option.var_type|= (flags & ALLOCATED) ? GET_STR_ALLOC : GET_STR; - global_var(const char*)= def_val; + if (def_val && (flags & ALLOCATED)) + global_var(const char*)= my_strdup(PSI_INSTRUMENT_ME, def_val, MYF(0)); + else + global_var(const char*)= def_val; } void cleanup() override { diff --git a/sql/wsrep_var.cc b/sql/wsrep_var.cc index c21fdf882f7..7fdaedba281 100644 --- a/sql/wsrep_var.cc +++ b/sql/wsrep_var.cc @@ -33,6 +33,15 @@ ulong wsrep_reject_queries; int wsrep_init_vars() { + my_free((char*) wsrep_provider); + my_free((char*) wsrep_provider_options); + my_free((char*) wsrep_cluster_address); + my_free((char*) wsrep_cluster_name); + my_free((char*) wsrep_node_name); + my_free((char*) wsrep_node_address); + my_free((char*) wsrep_node_incoming_address); + my_free((char*) wsrep_start_position); + wsrep_provider = my_strdup(PSI_INSTRUMENT_ME, WSREP_NONE, MYF(MY_WME)); wsrep_provider_options= my_strdup(PSI_INSTRUMENT_ME, "", MYF(MY_WME)); wsrep_cluster_address = my_strdup(PSI_INSTRUMENT_ME, "", MYF(MY_WME));