Re: bzr commit into mysql-6.0-backup branch (oystein.grovlen:2731) Bug#40480

[Rafal Somla]

Hi Oystein,

I have located the problem with double free. The reason was that Stream class 
contained another instance of a logger, instead of a reference to the main one 
(a typo?):

=== modified file 'sql/backup/stream.h'
--- sql/backup/stream.h 2008-10-15 20:00:48 +0000
+++ sql/backup/stream.h 2008-11-12 10:23:28 +0000
@@ -98,7 +98,7 @@ class Stream: public fd_stream
    ::String  *m_path;
    int     m_flags;  ///< flags used when opening the file
    size_t  m_block_size;
-  Logger  m_log;
+  Logger  &m_log;

After changing this, deleting backup_log inside ~Logger() works. I strongly 
recommend doing this, because backup_log is allocated inside Logger.

Other than this the patch looks OK. I have one more suggestion though: rename 
Logger::report_cleanup() to something else. The name is confusing, because 
Logger has other report_*() methods and they all are used for reporting 
information about ongoing BACKUP/RESTORE operation. But this one is not used for 
reporting, but rather it matches the Logger::init() method by freeing resources 
allocated there. I propose two alternatives:

a) Remove report_cleanup() and do cleanup inside the destructor.

b) Rename it to e.g., Logger::cleanup(). Also, place its definition next to 
Logger::init() so that it is easier to see that each resource allocated in 
init() is freed in cleanup().

Rafal


Oystein.Grovlen@stripped wrote:
> #At file:///home/oysteing/mysql/mysql-6.0-backup-valgrind/
> 
>  2731 oystein.grovlen@stripped	2008-11-11
>       Bug#40480 - valgrind errors which are not seen in PB
>       
>       Fixes all backup related issues reported by valgrind when running 
>       the backup test suite.
> modified:
>   sql/backup/kernel.cc
>   sql/backup/logger.h
>   sql/backup/stream_v1.c
> 
> per-file messages:
>   sql/backup/kernel.cc
>     1. Move report_cleanup() to Backup_Restore_ctx destructor to make sure clean up
> is always done.  Earlier it was sometimes skipped in error scenarios.  (I tried doing the
> cleanup in the Logger destructor, but that created problems.  For some strange reason the
> logger destructor was called twice on the same object.)
>     
>     2. Free allocated table space object in case of errors.
>   sql/backup/logger.h
>     1. Set backup_up pointer to NULL after destruction to prevent attempts to
> destruct it twice.
>     
>     2. Make sure to initialize error in Logger constructor.
>   sql/backup/stream_v1.c
>     Make sure iterator is freed on errors.
> === modified file 'sql/backup/kernel.cc'
> --- a/sql/backup/kernel.cc	2008-10-30 20:02:15 +0000
> +++ b/sql/backup/kernel.cc	2008-11-11 08:56:31 +0000
> @@ -331,7 +331,6 @@ int send_reply(Backup_restore_ctx &conte
>      goto err;
>    }
>    my_eof(context.thd());                        // Never errors
> -  context.report_cleanup();                     // Never errors
>    DBUG_RETURN(0);
>  
>   err:
> @@ -399,7 +398,8 @@ Backup_restore_ctx::Backup_restore_ctx(T
>  Backup_restore_ctx::~Backup_restore_ctx()
>  {
>    close();
> -  
> +  report_cleanup();                     // Never errors
> +
>    delete mem_alloc;
>    delete m_catalog;  
>    delete m_stream;
> @@ -1971,6 +1971,7 @@ int bcat_create_item(st_bstream_image_he
>      {
>        DBUG_PRINT("restore",(" tablespace has changed on the server - aborting"));
>        info->m_ctx.fatal_error(ER_BACKUP_TS_CHANGE, desc);
> +      delete ts;
>        return BSTREAM_ERROR;
>      }
>    }
> 
> === modified file 'sql/backup/logger.h'
> --- a/sql/backup/logger.h	2008-10-30 17:53:24 +0000
> +++ b/sql/backup/logger.h	2008-11-11 08:56:31 +0000
> @@ -67,7 +67,7 @@ class Logger
>       DBUG_ASSERT(backup_log);
>       return backup_log->get_backup_id(); 
>     }
> -   void report_cleanup() { delete backup_log; }
> +   void report_cleanup() { delete backup_log; backup_log = 0; }
>     
>     void save_errors();
>     void stop_save_errors();
> @@ -96,7 +96,10 @@ inline
>  Logger::Logger(THD *thd) 
>    :m_type(BACKUP), m_state(CREATED),
>     m_thd(thd), m_save_errors(FALSE), m_push_errors(TRUE), backup_log(0)
> -{}
> +{
> +  clear_saved_errors();
> +}
> + 
>  
>  inline
>  Logger::~Logger()
> 
> === modified file 'sql/backup/stream_v1.c'
> --- a/sql/backup/stream_v1.c	2008-09-30 08:08:16 +0000
> +++ b/sql/backup/stream_v1.c	2008-11-11 08:56:31 +0000
> @@ -1277,10 +1277,9 @@ int bstream_wr_meta_data(backup_stream *
>      CHECK_WR_RES(bstream_wr_item_def(s,cat,PER_TABLE_ITEM,item));
>    }
>  
> +wr_error:
>    bcat_iterator_free(cat,iter);
>  
> -  wr_error:
> -
>    return ret;
>  }
>  
> 
>