MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Tor Didriksen Date:September 23 2010 1:58pm
Subject:Re: bzr commit into mysql-5.1-bugteam branch (Alexey.Kopytov:3514) Bug#56709
View as plain text  
Hi Alexey

comments inline below

-- didrik

On Wed, Sep 22, 2010 at 9:33 PM, Alexey Kopytov <Alexey.Kopytov@stripped>wrote:

> #At file:///data/src/bzr/bugteam/mysql-5.1-bugteam/ based on
> revid:alfranio.correia@stripped
>
>  3514 Alexey Kopytov    2010-09-22
>      Bug #56709: Memory leaks at running the 5.1 test suite
>
>      Fixed a number of memory leaks discovered by valgrind.
>     @ dbug/dbug.c
>        This is actually an addendum to the fix for bug #52629:
>
>        - there is no point in limiting the fix to just global
>        variables, session ones are also affected.
>        - zero all fields when allocating a new 'state' structure so
>        that FreeState() does not deal with unitialized data later.
>        - add a check for a NULL pointer in DBUGCloseFile()
>     @ mysql-test/r/partition_error.result
>        Added a test case for bug #56709.
>     @ mysql-test/r/variables_debug.result
>        Added a test case for bug #56709.
>     @ mysql-test/t/partition_error.test
>        Added a test case for bug #56709.
>     @ mysql-test/t/variables_debug.test
>        Added a test case for bug #56709.
>     @ sql/item_timefunc.cc
>        There is no point in declaring 'value' as a member of
>        Item_extract and dynamically allocating memory for it in
>        Item_extract::fix_length_and_dec(), since this string is only
>        used as a temporary storage in Item_extract::val_int().
>     @ sql/item_timefunc.h
>        Removed 'value' from the Item_extract class definition.
>     @ sql/sql_load.cc
>        - we may need to deallocate 'buffer' even when 'error' is
>          non-zero in some cases, since 'error' is public, and there is
>          external code modifying it.
>        - assign NULL to buffer when deallocating it so that we don't
>          do it twice in the destructor
>        - there is no point in changing 'error' in the destructor.
>
>    modified:
>      dbug/dbug.c
>      mysql-test/r/partition_error.result
>      mysql-test/r/variables_debug.result
>      mysql-test/t/partition_error.test
>      mysql-test/t/variables_debug.test
>      sql/item_timefunc.cc
>      sql/item_timefunc.h
>      sql/sql_load.cc
> === modified file 'dbug/dbug.c'
> --- a/dbug/dbug.c       2010-05-20 06:31:03 +0000
> +++ b/dbug/dbug.c       2010-09-22 19:33:18 +0000
> @@ -455,13 +455,8 @@ static void DbugParse(CODE_STATE *cs, co
>   rel= control[0] == '+' || control[0] == '-';
>   if ((!rel || (!stack->out_file && !stack->next)))
>   {
> -    /*
> -      We need to free what's already in init_settings, because unlike
> -      the thread related stack frames there's a chance that something
> -      is in these variables already.
> -    */
> -    if (stack == &init_settings)
> -      FreeState(cs, stack, 0);
> +    /* Free memory associated with the state before resetting its members
> */
> +    FreeState(cs, stack, 0);
>     stack->flags= 0;
>     stack->delay= 0;
>     stack->maxdepth= 0;
> @@ -1447,8 +1442,8 @@ static void PushState(CODE_STATE *cs)
>   struct settings *new_malloc;
>
>   new_malloc= (struct settings *) DbugMalloc(sizeof(struct settings));
> +  bzero(new_malloc, sizeof(struct settings));
>

man bzero:
This function is deprecated (marked as LEGACY in POSIX.1-2001): use
memset(3) in new programs.


>   new_malloc->next= cs->stack;
> -  new_malloc->out_file= NULL;
>   cs->stack= new_malloc;
>  }
>
> @@ -1957,7 +1952,7 @@ static FILE *OpenProfile(CODE_STATE *cs,
>
>  static void DBUGCloseFile(CODE_STATE *cs, FILE *fp)
>  {
> -  if (fp != stderr && fp != stdout && fclose(fp) == EOF)
> +  if (fp != NULL && fp != stderr && fp != stdout &&
> fclose(fp) == EOF)
>   {
>     pthread_mutex_lock(&THR_LOCK_dbug);
>     (void) fprintf(cs->stack->out_file, ERR_CLOSE, cs->process);
>
> === modified file 'mysql-test/r/partition_error.result'
> --- a/mysql-test/r/partition_error.result       2010-05-25 13:41:00 +0000
> +++ b/mysql-test/r/partition_error.result       2010-09-22 19:33:18 +0000
> @@ -1008,4 +1008,14 @@ PARTITION p VALUES LESS THAN (1219089600
>  PARTITION pmax VALUES LESS THAN MAXVALUE);
>  ERROR HY000: Constant, random or timezone-dependent expressions in
> (sub)partitioning function are not allowed
>  DROP TABLE old;
> +#
> +# Bug #56709: Memory leaks at running the 5.1 test suite
> +#
> +CREATE TABLE t1 (a TIMESTAMP NOT NULL PRIMARY KEY);
> +ALTER TABLE t1
> +PARTITION BY RANGE (EXTRACT(DAY FROM a)) (
> +PARTITION p VALUES LESS THAN (18),
> +PARTITION pmax VALUES LESS THAN MAXVALUE);
> +ERROR HY000: Constant, random or timezone-dependent expressions in
> (sub)partitioning function are not allowed
> +DROP TABLE t1;
>  End of 5.1 tests
>
> === modified file 'mysql-test/r/variables_debug.result'
> --- a/mysql-test/r/variables_debug.result       2010-05-20 06:31:03 +0000
> +++ b/mysql-test/r/variables_debug.result       2010-09-22 19:33:18 +0000
> @@ -24,4 +24,17 @@ SELECT @@global.debug;
>  @@global.debug
>
>  SET GLOBAL debug=@old_debug;
> +#
> +# Bug #56709: Memory leaks at running the 5.1 test suite
> +#
> +SET @old_local_debug = @@debug;
> +SET @@debug='d,foo';
> +SELECT @@debug;
> +@@debug
> +d,foo
> +SET @@debug='';
> +SELECT @@debug;
> +@@debug
> +
> +SET @@debug = @old_local_debug;
>  End of 5.1 tests
>
> === modified file 'mysql-test/t/partition_error.test'
> --- a/mysql-test/t/partition_error.test 2010-06-01 07:02:28 +0000
> +++ b/mysql-test/t/partition_error.test 2010-09-22 19:33:18 +0000
> @@ -1252,4 +1252,18 @@ PARTITION pmax VALUES LESS THAN MAXVALUE
>
>  DROP TABLE old;
>
> +--echo #
> +--echo # Bug #56709: Memory leaks at running the 5.1 test suite
> +--echo #
> +
> +CREATE TABLE t1 (a TIMESTAMP NOT NULL PRIMARY KEY);
> +
> +--error ER_WRONG_EXPR_IN_PARTITION_FUNC_ERROR
> +ALTER TABLE t1
> +PARTITION BY RANGE (EXTRACT(DAY FROM a)) (
> +PARTITION p VALUES LESS THAN (18),
> +PARTITION pmax VALUES LESS THAN MAXVALUE);
> +
> +DROP TABLE t1;
> +
>  --echo End of 5.1 tests
>
> === modified file 'mysql-test/t/variables_debug.test'
> --- a/mysql-test/t/variables_debug.test 2010-05-20 06:31:03 +0000
> +++ b/mysql-test/t/variables_debug.test 2010-09-22 19:33:18 +0000
> @@ -25,4 +25,17 @@ SELECT @@global.debug;
>
>  SET GLOBAL debug=@old_debug;
>
> +--echo #
> +--echo # Bug #56709: Memory leaks at running the 5.1 test suite
> +--echo #
> +
> +SET @old_local_debug = @@debug;
> +
> +SET @@debug='d,foo';
> +SELECT @@debug;
> +SET @@debug='';
> +SELECT @@debug;
> +
> +SET @@debug = @old_local_debug;
> +
>  --echo End of 5.1 tests
>
> === modified file 'sql/item_timefunc.cc'
> --- a/sql/item_timefunc.cc      2010-07-09 12:00:17 +0000
> +++ b/sql/item_timefunc.cc      2010-09-22 19:33:18 +0000
> @@ -2270,8 +2270,6 @@ void Item_extract::print(String *str, en
>
>  void Item_extract::fix_length_and_dec()
>  {
> -  value.alloc(32);                             // alloc buffer
> -
>   maybe_null=1;                                        // If wrong date
>   switch (int_type) {
>   case INTERVAL_YEAR:          max_length=4; date_value=1; break;
> @@ -2314,6 +2312,8 @@ longlong Item_extract::val_int()
>   }
>   else
>   {
> +    char buf[40];
> +    String value(buf, sizeof(buf), &my_charset_bin);;
>     String *res= args[0]->val_str(&value);
>     if (!res || str_to_time_with_warn(res->ptr(), res->length(), &ltime))
>     {
>
>
I agree that there is no need for the member field, so this solves the
immediate problem.
However, the leak indicates that the Item_extract instance has not been
destroyed.
This seems to be a more general solution:

=== modified file 'sql/sql_partition.cc'
--- sql/sql_partition.cc        2010-07-29 03:00:57 +0000
+++ sql/sql_partition.cc        2010-09-23 13:06:25 +0000
@@ -1025,6 +1025,7 @@
     if (is_create_table_ind)
     {
       my_error(ER_WRONG_EXPR_IN_PARTITION_FUNC_ERROR, MYF(0));
+      thd->free_items();
       goto end;
     }
     else



> === modified file 'sql/item_timefunc.h'
> --- a/sql/item_timefunc.h       2010-08-13 13:05:46 +0000
> +++ b/sql/item_timefunc.h       2010-09-22 19:33:18 +0000
> @@ -702,7 +702,6 @@ public:
>
>  class Item_extract :public Item_int_func
>  {
> -  String value;
>   bool date_value;
>  public:
>   const interval_type int_type; // keep it public
>
> === modified file 'sql/sql_load.cc'
> --- a/sql/sql_load.cc   2010-08-03 02:22:19 +0000
> +++ b/sql/sql_load.cc   2010-09-22 19:33:18 +0000
> @@ -1116,6 +1116,7 @@ READ_INFO::READ_INFO(File file_par, uint
>                      MYF(MY_WME)))
>     {
>       my_free((uchar*) buffer,MYF(0)); /* purecov: inspected */
> +      buffer= NULL;
>       error=1;
>     }
>     else
> @@ -1142,13 +1143,10 @@ READ_INFO::READ_INFO(File file_par, uint
>
>  READ_INFO::~READ_INFO()
>  {
> -  if (!error)
> -  {
> -    if (need_end_io_cache)
> -      ::end_io_cache(&cache);
> -    my_free((uchar*) buffer,MYF(0));
> -    error=1;
> -  }
> +  if (!error && need_end_io_cache)
> +    ::end_io_cache(&cache);
> +
> +  my_free(buffer, MYF(MY_ALLOW_ZERO_PTR));
>  }
>
>
>
>
>
> --
> MySQL Code Commits Mailing List
> For list archives: http://lists.mysql.com/commits
> To unsubscribe:
> http://lists.mysql.com/commits?unsub=1
>

Thread
bzr commit into mysql-5.1-bugteam branch (Alexey.Kopytov:3514)Bug#56709Alexey Kopytov22 Sep
  • Re: bzr commit into mysql-5.1-bugteam branch (Alexey.Kopytov:3514) Bug#56709Tor Didriksen23 Sep
    • Re: bzr commit into mysql-5.1-bugteam branch (Alexey.Kopytov:3514)Bug#56709Alexey Kopytov24 Sep
      • Re: bzr commit into mysql-5.1-bugteam branch (Alexey.Kopytov:3514) Bug#56709Tor Didriksen30 Sep
        • Re: bzr commit into mysql-5.1-bugteam branch (Alexey.Kopytov:3514)Bug#56709Davi Arnaut30 Sep
          • Re: bzr commit into mysql-5.1-bugteam branch (Alexey.Kopytov:3514)Bug#56709Sergei Golubchik30 Sep
            • Re: bzr commit into mysql-5.1-bugteam branch (Alexey.Kopytov:3514)Bug#56709Davi Arnaut30 Sep
              • Re: bzr commit into mysql-5.1-bugteam branch (Alexey.Kopytov:3514)Bug#56709Davi Arnaut30 Sep
            • Re: bzr commit into mysql-5.1-bugteam branch (Alexey.Kopytov:3514) Bug#56709Tor Didriksen1 Oct
              • Re: bzr commit into mysql-5.1-bugteam branch (Alexey.Kopytov:3514)Bug#56709Alexey Kopytov1 Oct
                • Re: bzr commit into mysql-5.1-bugteam branch (Alexey.Kopytov:3514)Bug#56709Davi Arnaut1 Oct
                  • Re: bzr commit into mysql-5.1-bugteam branch (Alexey.Kopytov:3514)Bug#56709Alexey Kopytov2 Oct
                • Re: bzr commit into mysql-5.1-bugteam branch (Alexey.Kopytov:3514) Bug#56709Tor Didriksen1 Oct
                  • Re: bzr commit into mysql-5.1-bugteam branch (Alexey.Kopytov:3514)Bug#56709Alexey Kopytov1 Oct
Re: bzr commit into mysql-5.1-bugteam branch (Alexey.Kopytov:3514)Bug#56709Davi Arnaut1 Oct
  • Re: bzr commit into mysql-5.1-bugteam branch (Alexey.Kopytov:3514)Bug#56709Alexey Kopytov1 Oct
Re: bzr commit into mysql-5.1-bugteam branch (Alexey.Kopytov:3514)Bug#56709Alexey Kopytov1 Oct
  • Re: bzr commit into mysql-5.1-bugteam branch (Alexey.Kopytov:3514)Bug#56709Davi Arnaut1 Oct
    • Re: bzr commit into mysql-5.1-bugteam branch (Alexey.Kopytov:3514)Bug#56709Alexey Kopytov1 Oct
      • Re: bzr commit into mysql-5.1-bugteam branch (Alexey.Kopytov:3514)Bug#56709Davi Arnaut1 Oct
        • Re: bzr commit into mysql-5.1-bugteam branch (Alexey.Kopytov:3514)Bug#56709Alexey Kopytov1 Oct
          • Re: bzr commit into mysql-5.1-bugteam branch (Alexey.Kopytov:3514)Bug#56709Davi Arnaut1 Oct