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

[Øystein Grøvlen]

Rafal Somla wrote:
> 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?):

Thanks for finding this, and for reminding me about C++ mechanisms that 
I had forgotten about. (destruction of member objects and shallow copy 
on assignment).

I will remove report_cleanup() and do the cleanup inside the destructor.
New patch will come soon.

Thanks,

--
Øystein

> 
> === 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;
>>  }
>>  
>>
>>
> 


-- 
Øystein Grøvlen, Senior Staff Engineer
Sun Microsystems, Database Group
Trondheim, Norway