From a10ae35328ec800eff63c0bbb06d279e44c0a5a1 Mon Sep 17 00:00:00 2001 From: Davi Arnaut Date: Thu, 8 Jul 2010 18:20:08 -0300 Subject: Bug#34043: Server loops excessively in _checkchunk() when safemalloc is enabled Essentially, the problem is that safemalloc is excruciatingly slow as it checks all allocated blocks for overrun at each memory management primitive, yielding a almost exponential slowdown for the memory management functions (malloc, realloc, free). The overrun check basically consists of verifying some bytes of a block for certain magic keys, which catches some simple forms of overrun. Another minor problem is violation of aliasing rules and that its own internal list of blocks is prone to corruption. Another issue with safemalloc is rather the maintenance cost as the tool has a significant impact on the server code. Given the magnitude of memory debuggers available nowadays, especially those that are provided with the platform malloc implementation, maintenance of a in-house and largely obsolete memory debugger becomes a burden that is not worth the effort due to its slowness and lack of support for detecting more common forms of heap corruption. Since there are third-party tools that can provide the same functionality at a lower or comparable performance cost, the solution is to simply remove safemalloc. Third-party tools can provide the same functionality at a lower or comparable performance cost. The removal of safemalloc also allows a simplification of the malloc wrappers, removing quite a bit of kludge: redefinition of my_malloc, my_free and the removal of the unused second argument of my_free. Since free() always check whether the supplied pointer is null, redudant checks are also removed. Also, this patch adds unit testing for my_malloc and moves my_realloc implementation into the same file as the other memory allocation primitives. --- client/mysqltest.cc | 102 ++++++++++++++++++++++++---------------------------- 1 file changed, 46 insertions(+), 56 deletions(-) (limited to 'client/mysqltest.cc') diff --git a/client/mysqltest.cc b/client/mysqltest.cc index 75b23ea9af7..1d821c18111 100644 --- a/client/mysqltest.cc +++ b/client/mysqltest.cc @@ -84,10 +84,10 @@ static my_bool get_one_option(int optid, const struct my_option *, C_MODE_END enum { - OPT_SKIP_SAFEMALLOC=OPT_MAX_CLIENT_OPTION, - OPT_PS_PROTOCOL, OPT_SP_PROTOCOL, OPT_CURSOR_PROTOCOL, OPT_VIEW_PROTOCOL, - OPT_MAX_CONNECT_RETRIES, OPT_MAX_CONNECTIONS, OPT_MARK_PROGRESS, - OPT_LOG_DIR, OPT_TAIL_LINES, OPT_RESULT_FORMAT_VERSION + OPT_PS_PROTOCOL=OPT_MAX_CLIENT_OPTION, OPT_SP_PROTOCOL, + OPT_CURSOR_PROTOCOL, OPT_VIEW_PROTOCOL, OPT_MAX_CONNECT_RETRIES, + OPT_MAX_CONNECTIONS, OPT_MARK_PROGRESS, OPT_LOG_DIR, + OPT_TAIL_LINES, OPT_RESULT_FORMAT_VERSION }; static int record= 0, opt_sleep= -1; @@ -156,7 +156,7 @@ static struct st_block *cur_block, *block_stack_end; struct st_test_file { FILE* file; - const char *file_name; + char *file_name; uint lineno; /* Current line in file */ }; @@ -1106,9 +1106,9 @@ void close_connections() mysql_close(&next_con->mysql); if (next_con->util_mysql) mysql_close(next_con->util_mysql); - my_free(next_con->name, MYF(MY_ALLOW_ZERO_PTR)); + my_free(next_con->name); } - my_free(connections, MYF(MY_WME)); + my_free(connections); DBUG_VOID_RETURN; } @@ -1137,7 +1137,7 @@ void close_files() DBUG_PRINT("info", ("closing file: %s", cur_file->file_name)); fclose(cur_file->file); } - my_free((uchar*) cur_file->file_name, MYF(MY_ALLOW_ZERO_PTR)); + my_free(cur_file->file_name); cur_file->file_name= 0; } DBUG_VOID_RETURN; @@ -1157,22 +1157,22 @@ void free_used_memory() for (i= 0 ; i < q_lines.elements ; i++) { struct st_command **q= dynamic_element(&q_lines, i, struct st_command**); - my_free((*q)->query_buf,MYF(MY_ALLOW_ZERO_PTR)); + my_free((*q)->query_buf); if ((*q)->content.str) dynstr_free(&(*q)->content); - my_free((*q),MYF(0)); + my_free((*q)); } for (i= 0; i < 10; i++) { if (var_reg[i].alloced_len) - my_free(var_reg[i].str_val, MYF(MY_WME)); + my_free(var_reg[i].str_val); } while (embedded_server_arg_count > 1) - my_free(embedded_server_args[--embedded_server_arg_count],MYF(0)); + my_free(embedded_server_args[--embedded_server_arg_count]); delete_dynamic(&q_lines); dynstr_free(&ds_res); free_all_replace(); - my_free(opt_pass,MYF(MY_ALLOW_ZERO_PTR)); + my_free(opt_pass); free_defaults(default_argv); free_re(); #ifdef __WIN__ @@ -1933,9 +1933,10 @@ static uchar *get_var_key(const uchar* var, size_t *len, static void var_free(void *v) { - my_free(((VAR*) v)->str_val, MYF(MY_WME)); - if (((VAR*)v)->alloced) - my_free(v, MYF(MY_WME)); + VAR *var= (VAR*) v; + my_free(var->str_val); + if (var->alloced) + my_free(var); } C_MODE_END @@ -4824,7 +4825,7 @@ void do_close_connection(struct st_command *command) con->util_mysql= 0; con->pending= FALSE; - my_free(con->name, MYF(0)); + my_free(con->name); /* When the connection is closed set name to "-closed_connection-" @@ -5491,7 +5492,7 @@ int read_line(char *buf, int size) fclose(cur_file->file); cur_file->file= 0; } - my_free((uchar*) cur_file->file_name, MYF(MY_ALLOW_ZERO_PTR)); + my_free(cur_file->file_name); cur_file->file_name= 0; if (cur_file == file_stack) { @@ -5967,9 +5968,6 @@ static struct my_option my_long_options[] = 0, 0, 0}, {"silent", 's', "Suppress all normal output. Synonym for --quiet.", &silent, &silent, 0, GET_BOOL, NO_ARG, 0, 0, 0, 0, 0, 0}, - {"skip-safemalloc", OPT_SKIP_SAFEMALLOC, - "Don't use the memory allocation checking.", 0, 0, 0, GET_NO_ARG, NO_ARG, - 0, 0, 0, 0, 0, 0}, {"sleep", 'T', "Always sleep this many seconds on sleep commands.", &opt_sleep, &opt_sleep, 0, GET_INT, REQUIRED_ARG, -1, -1, 0, 0, 0, 0}, @@ -6125,7 +6123,7 @@ get_one_option(int optid, const struct my_option *, char *argument) argument= (char*) ""; // Don't require password if (argument) { - my_free(opt_pass, MYF(MY_ALLOW_ZERO_PTR)); + my_free(opt_pass); opt_pass= my_strdup(argument, MYF(MY_FAE)); while (*argument) *argument++= 'x'; /* Destroy argument */ tty_password= 0; @@ -6158,11 +6156,6 @@ get_one_option(int optid, const struct my_option *, char *argument) case 'F': read_embedded_server_arguments(argument); break; - case OPT_SKIP_SAFEMALLOC: -#ifdef SAFEMALLOC - sf_malloc_quick=1; -#endif - break; case OPT_RESULT_FORMAT_VERSION: set_result_format_version(opt_result_format_version); break; @@ -6321,7 +6314,7 @@ void init_win_path_patterns() /* Don't insert zero length strings in patterns array */ if (strlen(p) == 0) { - my_free(p, MYF(0)); + my_free(p); continue; } @@ -6345,7 +6338,7 @@ void free_win_path_patterns() for (i=0 ; i < patterns.elements ; i++) { const char** pattern= dynamic_element(&patterns, i, const char**); - my_free((char*) *pattern, MYF(0)); + my_free(*pattern); } delete_dynamic(&patterns); } @@ -6538,12 +6531,12 @@ void append_stmt_result(DYNAMIC_STRING *ds, MYSQL_STMT *stmt, for (i= 0; i < num_fields; i++) { /* Free data for output */ - my_free(my_bind[i].buffer, MYF(MY_WME | MY_FAE)); + my_free(my_bind[i].buffer); } /* Free array with bind structs, lengths and NULL flags */ - my_free(my_bind , MYF(MY_WME | MY_FAE)); - my_free(length , MYF(MY_WME | MY_FAE)); - my_free(is_null , MYF(MY_WME | MY_FAE)); + my_free(my_bind); + my_free(length); + my_free(is_null); } @@ -8489,11 +8482,11 @@ void do_get_replace_column(struct st_command *command) if (!*from) die("Wrong number of arguments to replace_column in '%s'", command->query); to= get_string(&buff, &from, command); - my_free(replace_column[column_number-1], MY_ALLOW_ZERO_PTR); + my_free(replace_column[column_number-1]); replace_column[column_number-1]= my_strdup(to, MYF(MY_WME | MY_FAE)); set_if_bigger(max_replace_column, column_number); } - my_free(start, MYF(0)); + my_free(start); command->last_argument= command->end; DBUG_VOID_RETURN; @@ -8507,7 +8500,7 @@ void free_replace_column() { if (replace_column[i]) { - my_free(replace_column[i], 0); + my_free(replace_column[i]); replace_column[i]= 0; } } @@ -8588,7 +8581,7 @@ void do_get_replace(struct st_command *command) die("Can't initialize replace from '%s'", command->query); free_pointer_array(&from_array); free_pointer_array(&to_array); - my_free(start, MYF(0)); + my_free(start); command->last_argument= command->end; DBUG_VOID_RETURN; } @@ -8597,11 +8590,8 @@ void do_get_replace(struct st_command *command) void free_replace() { DBUG_ENTER("free_replace"); - if (glob_replace) - { - my_free(glob_replace,MYF(0)); - glob_replace=0; - } + my_free(glob_replace); + glob_replace= NULL; DBUG_VOID_RETURN; } @@ -8821,7 +8811,7 @@ struct st_replace_regex* init_replace_regex(char* expr) return res; err: - my_free(res,0); + my_free(res); die("Error parsing replace_regex \"%s\"", expr); return 0; } @@ -8913,9 +8903,9 @@ void free_replace_regex() if (glob_replace_regex) { delete_dynamic(&glob_replace_regex->regex_arr); - my_free(glob_replace_regex->even_buf,MYF(MY_ALLOW_ZERO_PTR)); - my_free(glob_replace_regex->odd_buf,MYF(MY_ALLOW_ZERO_PTR)); - my_free(glob_replace_regex,MYF(0)); + my_free(glob_replace_regex->even_buf); + my_free(glob_replace_regex->odd_buf); + my_free(glob_replace_regex); glob_replace_regex=0; } } @@ -9110,7 +9100,7 @@ int reg_replace(char** buf_p, int* buf_len_p, char *pattern, str_p= str_end; } } - my_free(subs, MYF(0)); + my_free(subs); my_regfree(&r); *res_p= 0; *buf_p= buf; @@ -9242,7 +9232,7 @@ REPLACE *init_replace(char * *from, char * *to,uint count, if (!(follow=(FOLLOWS*) my_malloc((states+2)*sizeof(FOLLOWS),MYF(MY_WME)))) { free_sets(&sets); - my_free(found_set,MYF(0)); + my_free(found_set); DBUG_RETURN(0); } @@ -9437,9 +9427,9 @@ REPLACE *init_replace(char * *from, char * *to,uint count, replace[i].next[j]=(REPLACE*) (rep_str+(-sets.set[i].next[j]-1)); } } - my_free(follow,MYF(0)); + my_free(follow); free_sets(&sets); - my_free(found_set,MYF(0)); + my_free(found_set); DBUG_PRINT("exit",("Replace table has %d states",sets.count)); DBUG_RETURN(replace); } @@ -9455,7 +9445,7 @@ int init_sets(REP_SETS *sets,uint states) if (!(sets->bit_buffer=(uint*) my_malloc(sizeof(uint)*sets->size_of_bits* SET_MALLOC_HUNC,MYF(MY_WME)))) { - my_free(sets->set,MYF(0)); + my_free(sets->set); return 1; } return 0; @@ -9516,8 +9506,8 @@ void free_last_set(REP_SETS *sets) void free_sets(REP_SETS *sets) { - my_free(sets->set_buffer,MYF(0)); - my_free(sets->bit_buffer,MYF(0)); + my_free(sets->set_buffer); + my_free(sets->bit_buffer); return; } @@ -9657,7 +9647,7 @@ int insert_pointer_name(reg1 POINTER_ARRAY *pa,char * name) if (!(pa->str= (uchar*) my_malloc((uint) (PS_MALLOC-MALLOC_OVERHEAD), MYF(MY_WME)))) { - my_free((char*) pa->typelib.type_names,MYF(0)); + my_free(pa->typelib.type_names); DBUG_RETURN (-1); } pa->max_count=(PC_MALLOC-MALLOC_OVERHEAD)/(sizeof(uchar*)+ @@ -9718,9 +9708,9 @@ void free_pointer_array(POINTER_ARRAY *pa) if (pa->typelib.count) { pa->typelib.count=0; - my_free((char*) pa->typelib.type_names,MYF(0)); + my_free(pa->typelib.type_names); pa->typelib.type_names=0; - my_free(pa->str,MYF(0)); + my_free(pa->str); } } /* free_pointer_array */ -- cgit v1.2.1 From dbf76b0deb58ef188585b85f93a0add4d8fcc9d8 Mon Sep 17 00:00:00 2001 From: Davi Arnaut Date: Thu, 8 Jul 2010 22:19:57 -0300 Subject: Bug#34043: Server loops excessively in _checkchunk() when safemalloc is enabled Post-merge fix: cast argument and correct type in assignment. --- client/mysqltest.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'client/mysqltest.cc') diff --git a/client/mysqltest.cc b/client/mysqltest.cc index 1d821c18111..9845a3ec060 100644 --- a/client/mysqltest.cc +++ b/client/mysqltest.cc @@ -6338,7 +6338,7 @@ void free_win_path_patterns() for (i=0 ; i < patterns.elements ; i++) { const char** pattern= dynamic_element(&patterns, i, const char**); - my_free(*pattern); + my_free((void *) *pattern); } delete_dynamic(&patterns); } -- cgit v1.2.1 From f54a1182494db9bababccfa83692630bec51ca95 Mon Sep 17 00:00:00 2001 From: Davi Arnaut Date: Thu, 15 Jul 2010 08:13:30 -0300 Subject: WL#5486: Remove code for unsupported platforms Remove Netware specific code. --- client/mysqltest.cc | 5 ----- 1 file changed, 5 deletions(-) (limited to 'client/mysqltest.cc') diff --git a/client/mysqltest.cc b/client/mysqltest.cc index 9845a3ec060..9e28183c735 100644 --- a/client/mysqltest.cc +++ b/client/mysqltest.cc @@ -3707,7 +3707,6 @@ void do_send_quit(struct st_command *command) void do_change_user(struct st_command *command) { MYSQL *mysql = &cur_con->mysql; - /* static keyword to make the NetWare compiler happy. */ static DYNAMIC_STRING ds_user, ds_passwd, ds_db; const struct command_arg change_user_args[] = { { "user", ARG_STRING, FALSE, &ds_user, "User to connect as" }, @@ -6006,8 +6005,6 @@ static struct my_option my_long_options[] = }; -#include - void print_version(void) { printf("%s Ver %s Distrib %s, for %s (%s)\n",my_progname,MTEST_VERSION, @@ -6026,8 +6023,6 @@ void usage() my_print_variables(my_long_options); } -#include - /* Read arguments for embedded server and put them into -- cgit v1.2.1