Commit f0b38904 authored by Luis Soares's avatar Luis Soares

BUG#48993: valgrind errors in mysqlbinlog

I found three issues during the analysis:
 1. Memory leak caused by temp_buf not being freed;
 2. Memory leak caused when handling argv;
 3. Conditional jump that depended on unitialized values.

Issue #1
--------

  DESCRIPTION: when mysqlbinlog is reading from a remote location
  the event temp_buf references the incoming stream (in NET
  object), which is not freed by mysqlbinlog explicitly. On the
  other hand, when it is reading local binary log, it points to a
  temporary buffer that needs to be explicitly freed. For both
  cases, the temp_buf was not freed by mysqlbinlog, instead was
  set to 0.  This clearly disregards the free required in the
  second case, thence creating a memory leak.

  FIX: we make temp_buf to be conditionally freed depending on
  the value of remote_opt. Found out that similar fix is already
  in most recent codebases.

Issue #2 
--------

  DESCRIPTION: load_defaults is called by parse_args, and it
  reads default options from configuration files and put them
  BEFORE the arguments that are already in argc and argv. This is
  done resorting to MEM_ROOT. However, parse_args calls
  handle_options immediately after which changes argv. Later when
  freeing the defaults, pointers to MEM_ROOT won't match, causing
  the memory not to be freed:

  void free_defaults(char **argv)
  {
    MEM_ROOT ptr
    memcpy_fixed((char*) &ptr,(char *) argv - sizeof(ptr), sizeof(ptr));
    free_root(&ptr,MYF(0));
  }

  FIX: we remove load_defaults from parse_args and call it
  before. Then we save argv with defaults in defaults_argv BEFORE
  calling parse_args (which inside can then call handle_options
  at will). Actually, found out that this is in fact kind of a
  backport for BUG#38468 into 5.1, so I merged in the test case
  as well and added error check for load_defaults call.

  Fix based on:
  revid:zhenxing.he@sun.com-20091002081840-uv26f0flw4uvo33y


Issue #3 
--------

  DESCRIPTION: the structure st_print_event_info constructor
  would not initialize the sql_mode member, although it did for
  sql_mode_inited (set to false). This would later raise the
  warning in valgrind when printing the sql_mode in the event
  header, as this print out is protected by a check against
  sql_mode_inited and sql_mode variables. Given that sql_mode was
  not initialized valgrind would output the warning.

  FIX: we add initialization of sql_mode to the
  st_print_event_info constructor.
 

client/mysqlbinlog.cc:
  - Conditionally free ev->temp_buf.
  - save defaults_argv before handle_options is called.
mysql-test/t/mysqlbinlog.test:
  Added test case from BUG#38468.
sql/log_event.cc:
  Added initialization of sql_mode for st_print_event_info.
parent 32058ba9
...@@ -832,7 +832,11 @@ Exit_status process_event(PRINT_EVENT_INFO *print_event_info, Log_event *ev, ...@@ -832,7 +832,11 @@ Exit_status process_event(PRINT_EVENT_INFO *print_event_info, Log_event *ev,
print_event_info->common_header_len= print_event_info->common_header_len=
glob_description_event->common_header_len; glob_description_event->common_header_len;
ev->print(result_file, print_event_info); ev->print(result_file, print_event_info);
ev->temp_buf= 0; // as the event ref is zeroed if (!remote_opt)
ev->free_temp_buf(); // free memory allocated in dump_local_log_entries
else
// disassociate but not free dump_remote_log_entries time memory
ev->temp_buf= 0;
/* /*
We don't want this event to be deleted now, so let's hide it (I We don't want this event to be deleted now, so let's hide it (I
(Guilhem) should later see if this triggers a non-serious Valgrind (Guilhem) should later see if this triggers a non-serious Valgrind
...@@ -1362,7 +1366,6 @@ static int parse_args(int *argc, char*** argv) ...@@ -1362,7 +1366,6 @@ static int parse_args(int *argc, char*** argv)
int ho_error; int ho_error;
result_file = stdout; result_file = stdout;
load_defaults("my",load_default_groups,argc,argv);
if ((ho_error=handle_options(argc, argv, my_long_options, get_one_option))) if ((ho_error=handle_options(argc, argv, my_long_options, get_one_option)))
exit(ho_error); exit(ho_error);
if (debug_info_flag) if (debug_info_flag)
...@@ -2019,8 +2022,10 @@ int main(int argc, char** argv) ...@@ -2019,8 +2022,10 @@ int main(int argc, char** argv)
my_init_time(); // for time functions my_init_time(); // for time functions
if (load_defaults("my", load_default_groups, &argc, &argv))
exit(1);
defaults_argv= argv;
parse_args(&argc, (char***)&argv); parse_args(&argc, (char***)&argv);
defaults_argv=argv;
if (!argc) if (!argc)
{ {
......
...@@ -443,3 +443,27 @@ FLUSH LOGS; ...@@ -443,3 +443,27 @@ FLUSH LOGS;
--echo End of 5.0 tests --echo End of 5.0 tests
--echo End of 5.1 tests --echo End of 5.1 tests
#
# BUG#38468 Memory leak detected when using mysqlbinlog utility;
#
disable_query_log;
RESET MASTER;
CREATE TABLE t1 SELECT 1;
FLUSH LOGS;
DROP TABLE t1;
enable_query_log;
# Write an empty file for comparison
write_file $MYSQLTEST_VARDIR/tmp/mysqlbinlog.warn.empty;
EOF
# Before fix of BUG#38468, this would generate some warnings
--exec $MYSQL_BINLOG $MYSQLD_DATADIR/master-bin.000001 >/dev/null 2> $MYSQLTEST_VARDIR/tmp/mysqlbinlog.warn
# Make sure the command above does not generate any error or warnings
diff_files $MYSQLTEST_VARDIR/tmp/mysqlbinlog.warn $MYSQLTEST_VARDIR/tmp/mysqlbinlog.warn.empty;
# Cleanup for this part of test
remove_file $MYSQLTEST_VARDIR/tmp/mysqlbinlog.warn.empty;
remove_file $MYSQLTEST_VARDIR/tmp/mysqlbinlog.warn;
...@@ -9478,7 +9478,7 @@ Incident_log_event::write_data_body(IO_CACHE *file) ...@@ -9478,7 +9478,7 @@ Incident_log_event::write_data_body(IO_CACHE *file)
they will always be printed for the first event. they will always be printed for the first event.
*/ */
st_print_event_info::st_print_event_info() st_print_event_info::st_print_event_info()
:flags2_inited(0), sql_mode_inited(0), :flags2_inited(0), sql_mode_inited(0), sql_mode(0),
auto_increment_increment(0),auto_increment_offset(0), charset_inited(0), auto_increment_increment(0),auto_increment_offset(0), charset_inited(0),
lc_time_names_number(~0), lc_time_names_number(~0),
charset_database_number(ILLEGAL_CHARSET_INFO_NUMBER), charset_database_number(ILLEGAL_CHARSET_INFO_NUMBER),
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment