Re: bk commit into 5.0 tree (sven:1.2501) BUG#30752

[Mats Kindahl]

Hi Sven!

Patch looks good. Patch OK to push.

Just my few cents,
Mats Kindahl

Sven Sandberg wrote:
> Below is the list of changes that have just been committed into a local
> 5.0 repository of sven. When sven does a push these changes will
> be propagated to the main repository and, within 24 hours after the
> push, to the public repository.
> For information on how to access the public repository
> see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html
>
> ChangeSet@stripped, 2007-09-14 12:39:11+02:00, sven@murkla.(none) +2 -0
>   BUG#30752 rpl_dual_pos_advance valgrind (jump depends on uninitialized LOG_INFO)
>   Problem: one thread could read uninitialized memory from (the stack of) another
> thread.
>   Fix: swapped order of initializing the memory and making it available to the other
> thread.
>   Fix: put lock around the statement that makes the memory available to the other
> thread.
>   Fix: all fields of the struct are now initialized in the constructor, to avoid
> future problems.
>
>   sql/sql_class.h@stripped, 2007-09-14 12:39:06+02:00, sven@murkla.(none) +7 -1
>     Initialize all members in constructor for more safe future code.
>
>   sql/sql_repl.cc@stripped, 2007-09-14 12:39:06+02:00, sven@murkla.(none) +8 -2
>     Swap order so that linfo is first initialized, then assigned, instead of the
> other way around.
>     Put a lock around the assignment.
>
> diff -Nrup a/sql/sql_class.h b/sql/sql_class.h
> --- a/sql/sql_class.h	2007-08-02 02:39:10 +02:00
> +++ b/sql/sql_class.h	2007-09-14 12:39:06 +02:00
> @@ -159,7 +159,13 @@ typedef struct st_log_info
>    my_off_t pos;
>    bool fatal; // if the purge happens to give us a negative offset
>    pthread_mutex_t lock;
> -  st_log_info():fatal(0) { pthread_mutex_init(&lock, MY_MUTEX_INIT_FAST);}
> +  st_log_info()
> +    : index_file_offset(0), index_file_start_offset(0),
> +      pos(0), fatal(0)
> +  {
> +    log_file_name[0] = '\0';
> +    pthread_mutex_init(&lock, MY_MUTEX_INIT_FAST);
> +  }
>    ~st_log_info() { pthread_mutex_destroy(&lock);}
>  } LOG_INFO;
>  
> diff -Nrup a/sql/sql_repl.cc b/sql/sql_repl.cc
> --- a/sql/sql_repl.cc	2007-07-05 10:09:54 +02:00
> +++ b/sql/sql_repl.cc	2007-09-14 12:39:06 +02:00
> @@ -364,7 +364,6 @@ void mysql_binlog_send(THD* thd, char* l
>      name=0;					// Find first log
>  
>    linfo.index_file_offset = 0;
> -  thd->current_linfo = &linfo;
>  
>    if (mysql_bin_log.find_log_pos(&linfo, name, 1))
>    {
> @@ -373,6 +372,10 @@ void mysql_binlog_send(THD* thd, char* l
>      goto err;
>    }
>  
> +  pthread_mutex_lock(&LOCK_thread_count);
> +  thd->current_linfo = &linfo;
> +  pthread_mutex_unlock(&LOCK_thread_count);
> +
>    if ((file=open_binlog(&log, log_file_name, &errmsg)) < 0)
>    {
>      my_errno= ER_MASTER_FATAL_ERROR_READING_BINLOG;
> @@ -1338,13 +1341,16 @@ bool mysql_show_binlog_events(THD* thd)
>        name=0;					// Find first log
>  
>      linfo.index_file_offset = 0;
> -    thd->current_linfo = &linfo;
>  
>      if (mysql_bin_log.find_log_pos(&linfo, name, 1))
>      {
>        errmsg = "Could not find target log";
>        goto err;
>      }
> +
> +    pthread_mutex_lock(&LOCK_thread_count);
> +    thd->current_linfo = &linfo;
> +    pthread_mutex_unlock(&LOCK_thread_count);
>  
>      if ((file=open_binlog(&log, linfo.log_file_name, &errmsg)) < 0)
>        goto err;
>   

It seems all other uses of the object stored under thd->current_linfo is 
protected by this mutex, so there is no risk of reading data that has 
been removed. It is also the case that the structure is auto, but every 
path out of the function resets the value of current_linfo to 0, so 
there is no risk of passing out a pointer that refers to stack data that 
does not exist any more.

-- 
Mats Kindahl
Lead Software Developer
Replication Team
MySQL AB, www.mysql.com