Skip to content

Commit

Permalink
Fixed memory leaks in embedded server and mysqltest
Browse files Browse the repository at this point in the history
This commit fixes the following issues:
- memory leak checking enabled for mysqltest. This cover all cases except
  calls to 'die()' that only happens in case of internal failures in
  mysqltest. die() is not called anymore in the result files differs.
- One can now run mtr --embedded without failures (this crashed or hang
  before)
- cleanup_and_exit() has a new parameter that indicates that it is called
  from die(), in which case we should not do memory leak checks. We now
  always call cleanup_and_exit() instead of exit() to be able to free up
  memory and discover memory leaks.
- Lots of new assert to catch error conditions
- More DBUG statements.
- Fixed that all results are freed in mysqltest (Fixed a memory leak in
  mysqltest when using prepared statements).
- Fixed race condition in do_stmt_close() that caused embedded server
  to not free memory. (Memory leak in mysqltest with embedded server).
- Fixed two memory leaks in embedded server when using prepared statements.
  These memory leaks caused timeout hangs in mtr when server was compiled
  with safemalloc. This issue was not noticed (except as timeouts) as
  memory report checking was done but output of it was disabled.
  • Loading branch information
montywi authored and vuvova committed Mar 28, 2024
1 parent 33c31fe commit 98cb6aa
Show file tree
Hide file tree
Showing 2 changed files with 82 additions and 53 deletions.
133 changes: 80 additions & 53 deletions client/mysqltest.cc
Expand Up @@ -616,7 +616,7 @@ void replace_strings_append(struct st_replace *rep, DYNAMIC_STRING* ds,
const char *from);

ATTRIBUTE_NORETURN
static void cleanup_and_exit(int exit_code);
static void cleanup_and_exit(int exit_code, bool called_from_die);

ATTRIBUTE_NORETURN
static void really_die(const char *msg);
Expand Down Expand Up @@ -933,6 +933,7 @@ pthread_attr_t cn_thd_attrib;
pthread_handler_t connection_thread(void *arg)
{
struct st_connection *cn= (struct st_connection*)arg;
DBUG_ENTER("connection_thread");

mysql_thread_init();
while (cn->command != EMB_END_CONNECTION)
Expand All @@ -944,6 +945,7 @@ pthread_handler_t connection_thread(void *arg)
pthread_cond_wait(&cn->query_cond, &cn->query_mutex);
pthread_mutex_unlock(&cn->query_mutex);
}
DBUG_PRINT("info", ("executing command: %d", cn->command));
switch (cn->command)
{
case EMB_END_CONNECTION:
Expand All @@ -964,24 +966,26 @@ pthread_handler_t connection_thread(void *arg)
break;
case EMB_CLOSE_STMT:
cn->result= mysql_stmt_close(cn->stmt);
cn->stmt= 0;
break;
default:
DBUG_ASSERT(0);
}
cn->command= 0;
pthread_mutex_lock(&cn->result_mutex);
cn->query_done= 1;
cn->command= 0;
pthread_cond_signal(&cn->result_cond);
pthread_mutex_unlock(&cn->result_mutex);
}

end_thread:
cn->query_done= 1;
DBUG_ASSERT(cn->stmt == 0);
mysql_close(cn->mysql);
cn->mysql= 0;
cn->query_done= 1;
mysql_thread_end();
pthread_exit(0);
return 0;
DBUG_RETURN(0);
}

static void wait_query_thread_done(struct st_connection *con)
Expand All @@ -999,12 +1003,16 @@ static void wait_query_thread_done(struct st_connection *con)

static void signal_connection_thd(struct st_connection *cn, int command)
{
DBUG_ENTER("signal_connection_thd");
DBUG_PRINT("enter", ("command: %d", command));

DBUG_ASSERT(cn->has_thread);
cn->query_done= 0;
cn->command= command;
pthread_mutex_lock(&cn->query_mutex);
cn->command= command;
pthread_cond_signal(&cn->query_cond);
pthread_mutex_unlock(&cn->query_mutex);
DBUG_VOID_RETURN;
}


Expand Down Expand Up @@ -1066,27 +1074,38 @@ static int do_stmt_execute(struct st_connection *cn)

static int do_stmt_close(struct st_connection *cn)
{
/* The cn->stmt is already set. */
DBUG_ENTER("do_stmt_close");
if (!cn->has_thread)
return mysql_stmt_close(cn->stmt);
{
/* The cn->stmt is already set. */
int res= mysql_stmt_close(cn->stmt);
cn->stmt= 0;
DBUG_RETURN(res);
}
wait_query_thread_done(cn);
signal_connection_thd(cn, EMB_CLOSE_STMT);
wait_query_thread_done(cn);
return cn->result;
DBUG_ASSERT(cn->stmt == 0);
DBUG_RETURN(cn->result);
}


static void emb_close_connection(struct st_connection *cn)
{
DBUG_ENTER("emb_close_connection");
if (!cn->has_thread)
return;
DBUG_VOID_RETURN;
wait_query_thread_done(cn);
signal_connection_thd(cn, EMB_END_CONNECTION);
pthread_join(cn->tid, NULL);
cn->has_thread= FALSE;
DBUG_ASSERT(cn->mysql == 0);
DBUG_ASSERT(cn->stmt == 0);
pthread_mutex_destroy(&cn->query_mutex);
pthread_cond_destroy(&cn->query_cond);
pthread_mutex_destroy(&cn->result_mutex);
pthread_cond_destroy(&cn->result_cond);
DBUG_VOID_RETURN;
}


Expand All @@ -1110,7 +1129,13 @@ static void init_connection_thd(struct st_connection *cn)
#define do_read_query_result(cn) mysql_read_query_result(cn->mysql)
#define do_stmt_prepare(cn, q, q_len) mysql_stmt_prepare(cn->stmt, q, (ulong)q_len)
#define do_stmt_execute(cn) mysql_stmt_execute(cn->stmt)
#define do_stmt_close(cn) mysql_stmt_close(cn->stmt)

static int do_stmt_close(struct st_connection *cn)
{
int res= mysql_stmt_close(cn->stmt);
cn->stmt= 0;
return res;
}

#endif /*EMBEDDED_LIBRARY*/

Expand Down Expand Up @@ -1438,7 +1463,6 @@ void close_statements()
{
if (con->stmt)
do_stmt_close(con);
con->stmt= 0;
}
DBUG_VOID_RETURN;
}
Expand Down Expand Up @@ -1505,24 +1529,15 @@ void free_used_memory()
}


ATTRIBUTE_NORETURN static void cleanup_and_exit(int exit_code)
ATTRIBUTE_NORETURN static void cleanup_and_exit(int exit_code,
bool called_from_die)
{
free_used_memory();

/* Only call mysql_server_end if mysql_server_init has been called */
if (server_initialized)
mysql_server_end();

/*
mysqltest is fundamentally written in a way that makes impossible
to free all memory before exit (consider memory allocated
for frame local DYNAMIC_STRING's and die() invoked down the stack.
We close stderr here to stop unavoidable safemalloc reports
from polluting the output.
*/
fclose(stderr);

my_end(my_end_arg);

if (!silent) {
Expand All @@ -1542,6 +1557,11 @@ ATTRIBUTE_NORETURN static void cleanup_and_exit(int exit_code)
}
}

/*
Report memory leaks, if not called from 'die()', as die() will not release
all memory.
*/
sf_leaking_memory= called_from_die;
exit(exit_code);
}

Expand Down Expand Up @@ -1608,7 +1628,7 @@ static void really_die(const char *msg)
second time, just exit
*/
if (dying)
cleanup_and_exit(1);
cleanup_and_exit(1, 1);
dying= 1;

log_file.show_tail(opt_tail_lines);
Expand All @@ -1620,7 +1640,7 @@ static void really_die(const char *msg)
if (cur_con && !cur_con->pending)
show_warnings_before_error(cur_con->mysql);

cleanup_and_exit(1);
cleanup_and_exit(1, 1);
}

void report_or_die(const char *fmt, ...)
Expand Down Expand Up @@ -1674,7 +1694,7 @@ void abort_not_supported_test(const char *fmt, ...)
}
va_end(args);

cleanup_and_exit(62);
cleanup_and_exit(62, 0);
}


Expand Down Expand Up @@ -2220,22 +2240,25 @@ int dyn_string_cmp(DYNAMIC_STRING* ds, const char *fname)
check_result
RETURN VALUES
error - the function will not return
0 ok
1 error
*/

void check_result()
int check_result()
{
const char *mess= 0;

int error= 1;
DBUG_ENTER("check_result");
DBUG_ASSERT(result_file_name);
DBUG_PRINT("enter", ("result_file_name: %s", result_file_name));

switch (compare_files(log_file.file_name(), result_file_name)) {
case RESULT_OK:
if (!error_count)
{
error= 0;
break; /* ok */
}
mess= "Got errors while running test";
/* Fallthrough */
case RESULT_LENGTH_MISMATCH:
Expand Down Expand Up @@ -2274,14 +2297,13 @@ void check_result()
log_file.file_name(), reject_file, errno);

show_diff(NULL, result_file_name, reject_file);
die("%s", mess);
fprintf(stderr, "%s", mess);
break;
}
default: /* impossible */
die("Unknown error code from dyn_string_cmp()");
}

DBUG_VOID_RETURN;
DBUG_RETURN(error);
}


Expand Down Expand Up @@ -5631,7 +5653,6 @@ void do_close_connection(struct st_command *command)
#endif /*!EMBEDDED_LIBRARY*/
if (con->stmt)
do_stmt_close(con);
con->stmt= 0;
#ifdef EMBEDDED_LIBRARY
/*
As query could be still executed in a separate thread
Expand Down Expand Up @@ -7308,17 +7329,17 @@ get_one_option(const struct my_option *opt, const char *argument, const char *)
break;
case 'V':
print_version();
exit(0);
cleanup_and_exit(0,0);
case OPT_MYSQL_PROTOCOL:
#ifndef EMBEDDED_LIBRARY
if ((opt_protocol= find_type_with_warning(argument, &sql_protocol_typelib,
opt->name)) <= 0)
exit(1);
cleanup_and_exit(1,0);
#endif
break;
case '?':
usage();
exit(0);
cleanup_and_exit(0,0);
}
return 0;
}
Expand All @@ -7330,12 +7351,12 @@ int parse_args(int argc, char **argv)
default_argv= argv;

if ((handle_options(&argc, &argv, my_long_options, get_one_option)))
exit(1);
cleanup_and_exit(1, 0);

if (argc > 1)
{
usage();
exit(1);
cleanup_and_exit(1, 0);
}
if (argc == 1)
opt_db= *argv;
Expand Down Expand Up @@ -8445,7 +8466,7 @@ void run_query_stmt(struct st_connection *cn, struct st_command *command,
my_bool ds_res_1st_execution_init = FALSE;
my_bool compare_2nd_execution = TRUE;
int query_match_ps2_re;

MYSQL_RES *res;
DBUG_ENTER("run_query_stmt");
DBUG_PRINT("query", ("'%-.60s'", query));

Expand Down Expand Up @@ -8631,10 +8652,13 @@ void run_query_stmt(struct st_connection *cn, struct st_command *command,
The --enable_prepare_warnings command can be used to change this so
that warnings from both the prepare and execute phase are shown.
*/
if ((mysql_stmt_result_metadata(stmt) != NULL) &&
!disable_warnings &&
!prepare_warnings_enabled)
dynstr_set(&ds_prepare_warnings, NULL);
if ((res= mysql_stmt_result_metadata(stmt)))
{
if (!disable_warnings &&
!prepare_warnings_enabled)
dynstr_set(&ds_prepare_warnings, NULL);
mysql_free_result(res);
}

/*
Fetch info before fetching warnings, since it will be reset
Expand Down Expand Up @@ -9729,6 +9753,7 @@ static sig_handler signal_handler(int sig)
fflush(stderr);
my_write_core(sig);
#ifndef _WIN32
sf_leaking_memory= 1;
exit(1); // Shouldn't get here but just in case
#endif
}
Expand Down Expand Up @@ -9802,12 +9827,10 @@ int main(int argc, char **argv)
uint command_executed= 0, last_command_executed= 0;
char save_file[FN_REFLEN];
bool empty_result= FALSE;
int error= 0;
MY_INIT(argv[0]);
DBUG_ENTER("main");

/* mysqltest has no way to free all its memory correctly */
sf_leaking_memory= 1;

save_file[0]= 0;
TMPDIR[0]= 0;

Expand Down Expand Up @@ -10486,7 +10509,7 @@ int main(int argc, char **argv)
die("Test ended with parsing disabled");

/*
The whole test has been executed _successfully_.
The whole test has been executed successfully.
Time to compare result or save it to record file.
The entire output from test is in the log file
*/
Expand All @@ -10509,7 +10532,7 @@ int main(int argc, char **argv)
else
{
/* Check that the output from test is equal to result file */
check_result();
error= check_result();
}
}
}
Expand All @@ -10519,7 +10542,8 @@ int main(int argc, char **argv)
if (! result_file_name || record ||
compare_files (log_file.file_name(), result_file_name))
{
die("The test didn't produce any output");
fprintf(stderr, "mysqltest: The test didn't produce any output\n");
error= 1;
}
else
{
Expand All @@ -10528,12 +10552,15 @@ int main(int argc, char **argv)
}

if (!command_executed && result_file_name && !empty_result)
die("No queries executed but non-empty result file found!");
{
fprintf(stderr, "mysqltest: No queries executed but non-empty result file found!\n");
error= 1;
}

verbose_msg("Test has succeeded!");
if (!error)
verbose_msg("Test has succeeded!");
timer_output();
/* Yes, if we got this far the test has succeeded! Sakila smiles */
cleanup_and_exit(0);
cleanup_and_exit(error, 0);
return 0; /* Keep compiler happy too */
}

Expand Down

0 comments on commit 98cb6aa

Please sign in to comment.