MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Luis Soares Date:February 17 2010 6:07pm
Subject:bzr commit into mysql-5.1-bugteam branch (luis.soares:3341) Bug#48993
View as plain text  
#At file:///home/lsoares/Workspace/bzr/work/bugfixing/48993/mysql-5.1-bugteam/ based on revid:joro@stripped

 3341 Luis Soares	2010-02-17
      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@stripped
      
      
      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.

    modified:
      client/mysqlbinlog.cc
      mysql-test/t/mysqlbinlog.test
      sql/log_event.cc
=== modified file 'client/mysqlbinlog.cc'
--- a/client/mysqlbinlog.cc	2010-02-04 12:39:42 +0000
+++ b/client/mysqlbinlog.cc	2010-02-17 18:07:28 +0000
@@ -832,7 +832,11 @@ Exit_status process_event(PRINT_EVENT_IN
       print_event_info->common_header_len=
         glob_description_event->common_header_len;
       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
         (Guilhem) should later see if this triggers a non-serious Valgrind
@@ -1362,7 +1366,6 @@ static int parse_args(int *argc, char***
   int ho_error;
 
   result_file = stdout;
-  load_defaults("my",load_default_groups,argc,argv);
   if ((ho_error=handle_options(argc, argv, my_long_options, get_one_option)))
     exit(ho_error);
   if (debug_info_flag)
@@ -2019,8 +2022,10 @@ int main(int argc, char** argv)
 
   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);
-  defaults_argv=argv;
 
   if (!argc)
   {

=== modified file 'mysql-test/t/mysqlbinlog.test'
--- a/mysql-test/t/mysqlbinlog.test	2009-12-06 01:11:32 +0000
+++ b/mysql-test/t/mysqlbinlog.test	2010-02-17 18:07:28 +0000
@@ -443,3 +443,27 @@ FLUSH LOGS;
 --echo End of 5.0 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;

=== modified file 'sql/log_event.cc'
--- a/sql/log_event.cc	2010-02-05 17:48:01 +0000
+++ b/sql/log_event.cc	2010-02-17 18:07:28 +0000
@@ -9478,7 +9478,7 @@ Incident_log_event::write_data_body(IO_C
   they will always be printed for the first event.
 */
 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),
    lc_time_names_number(~0),
    charset_database_number(ILLEGAL_CHARSET_INFO_NUMBER),


Attachment: [text/bzr-bundle] bzr/luis.soares@sun.com-20100217180728-thfmmivi2t20e9em.bundle
Thread
bzr commit into mysql-5.1-bugteam branch (luis.soares:3341) Bug#48993Luis Soares17 Feb
  • Re: bzr commit into mysql-5.1-bugteam branch (luis.soares:3341)Bug#48993Alfranio Correia22 Feb
    • Re: bzr commit into mysql-5.1-bugteam branch (luis.soares:3341)Bug#48993Luís Soares22 Feb
      • Re: bzr commit into mysql-5.1-bugteam branch (luis.soares:3341)Bug#48993Alfranio Correia22 Feb