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.

Author: montywi

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;
 }
 

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

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,19 +2458,23 @@ 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);
 		return true;
 	}
 	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 @@ void setup_error_messages()
 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;
 }

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); 

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

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

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
   {

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));