bzr commit into mysql-6.0-backup branch (ingo.struewing:2672) Bug#38108

[Ingo Struewing]

#At file:///home2/mydev/bzrroot/mysql-6.0-bug38108/

 2672 Ingo Struewing	2008-07-23
      Bug#38108 - Race condition in MyISAM backup driver
      
      A race condition between two or more running restore operations could leave the
      global variable 'myisam_single_user' in an undefined state.
      
      To force reading of state info from the index file, we changed the value of the
      global variable 'myisam_single_user' temporarily. If multiple threads run this
      code with arbitrary scheduling, the variable value may be left back with
      undefined value. It could even happen that a thread undoes a value change
      before the changing thread used it. Then the reading from file would be
      suppressed and the table left crashed.
      
      Fixed by adding a 'force' parameter to mi_state_info_read_dsk(). This makes
      modification of the global variable unnecessary.
modified:
  storage/myisam/mi_dynrec.c
  storage/myisam/mi_examine_log.c
  storage/myisam/mi_locking.c
  storage/myisam/mi_open.c
  storage/myisam/myisam_backup_engine.cc
  storage/myisam/myisamdef.h

per-file messages:
  storage/myisam/mi_dynrec.c
    Bug#38108 - Race condition in MyISAM backup driver
    Added an argument for the 'force' parameter of mi_state_info_read_dsk().
  storage/myisam/mi_examine_log.c
    Bug#38108 - Race condition in MyISAM backup driver
    Added an argument for the 'force' parameter of mi_state_info_read_dsk().
    Removed tampering with the global variable 'myisam_single_user'.
  storage/myisam/mi_locking.c
    Bug#38108 - Race condition in MyISAM backup driver
    Added an argument for the 'force' parameter of mi_state_info_read_dsk().
  storage/myisam/mi_open.c
    Bug#38108 - Race condition in MyISAM backup driver
    Added parameter 'force' to mi_state_info_read_dsk().
  storage/myisam/myisam_backup_engine.cc
    Bug#38108 - Race condition in MyISAM backup driver
    Added an argument for the 'force' parameter of mi_state_info_read_dsk().
    Removed tampering with the global variable 'myisam_single_user'.
  storage/myisam/myisamdef.h
    Bug#38108 - Race condition in MyISAM backup driver
    Added 'force' parameter to mi_state_info_read_dsk().
=== modified file 'storage/myisam/mi_dynrec.c'
--- a/storage/myisam/mi_dynrec.c	2008-07-09 07:12:43 +0000
+++ b/storage/myisam/mi_dynrec.c	2008-07-23 12:46:33 +0000
@@ -1742,7 +1742,7 @@ int _mi_read_rnd_dynamic_record(MI_INFO 
       {						/* Check if changed */
 	info_read=1;
 	info->rec_cache.seek_not_done=1;
-	if (mi_state_info_read_dsk(share->kfile,&share->state,1))
+	if (mi_state_info_read_dsk(share->kfile, &share->state, 1, 0))
 	  goto panic;
       }
       if (filepos >= info->state->data_file_length)

=== modified file 'storage/myisam/mi_examine_log.c'
--- a/storage/myisam/mi_examine_log.c	2008-07-08 20:18:10 +0000
+++ b/storage/myisam/mi_examine_log.c	2008-07-23 12:46:33 +0000
@@ -860,13 +860,9 @@ static int mi_close_care_state(MI_INFO *
   if (!update_index_on_close)
   {
     MYISAM_SHARE *share;
-    my_bool      old_myisam_single_user;
 
     share= info->s;
-    old_myisam_single_user= myisam_single_user;
-    myisam_single_user= FALSE;
-    (void) mi_state_info_read_dsk(share->kfile, &share->state, 1);
-    myisam_single_user= old_myisam_single_user;
+    (void) mi_state_info_read_dsk(share->kfile, &share->state, 1, 1);
   }
   return mi_close(info);
 }

=== modified file 'storage/myisam/mi_locking.c'
--- a/storage/myisam/mi_locking.c	2008-07-09 07:12:43 +0000
+++ b/storage/myisam/mi_locking.c	2008-07-23 12:46:33 +0000
@@ -159,7 +159,7 @@ int mi_lock_database(MI_INFO *info, int 
 	  error=my_errno;
 	  break;
 	}
-	if (mi_state_info_read_dsk(share->kfile, &share->state, 1))
+	if (mi_state_info_read_dsk(share->kfile, &share->state, 1, 0))
 	{
 	  error=my_errno;
 	  (void) my_lock(share->kfile,F_UNLCK,0L,F_TO_EOF,MYF(MY_SEEK_NOT_DONE));
@@ -204,7 +204,7 @@ int mi_lock_database(MI_INFO *info, int 
 	  }
 	  if (!share->r_locks)
 	  {
-	    if (mi_state_info_read_dsk(share->kfile, &share->state, 1))
+	    if (mi_state_info_read_dsk(share->kfile, &share->state, 1, 0))
 	    {
 	      error=my_errno;
 	      (void) my_lock(share->kfile,F_UNLCK,0L,F_TO_EOF,
@@ -415,7 +415,7 @@ int _mi_readinfo(register MI_INFO *info,
       if (my_lock(share->kfile,lock_type,0L,F_TO_EOF,
 		  info->lock_wait | MY_SEEK_NOT_DONE))
 	DBUG_RETURN(1);
-      if (mi_state_info_read_dsk(share->kfile, &share->state, 1))
+      if (mi_state_info_read_dsk(share->kfile, &share->state, 1, 0))
       {
 	int error= my_errno ? my_errno : HA_ERR_FILE_TOO_SHORT;
 	(void) my_lock(share->kfile,F_UNLCK,0L,F_TO_EOF,

=== modified file 'storage/myisam/mi_open.c'
--- a/storage/myisam/mi_open.c	2008-07-09 07:12:43 +0000
+++ b/storage/myisam/mi_open.c	2008-07-23 12:46:33 +0000
@@ -1048,11 +1048,40 @@ uchar *mi_state_info_read(uchar *ptr, MI
 }
 
 
-uint mi_state_info_read_dsk(File file, MI_STATE_INFO *state, my_bool pRead)
+/**
+  Read state info from file.
+
+  @param[in]        file        index file descriptor
+  @param[in,out]    state       state info to update from file
+  @param[in]        pRead       if to use my_pread() instaed of my_read()
+  @param[in]        force       force read
+
+  @return           status
+    @retval         0           ok
+    @retval         1           error
+
+  Normally this function does not read the state info from file if
+  'myisam_single_user' is true. This means that mysqld is the only
+  program that works on the table files. No other program modifies the
+  files. Hence the in-memory state is expected to be current.
+
+  If there are other programs tampering with the files, mysqld must be
+  started with --external-locking. This makes 'myisam_single_user'
+  false. In this case this function does indeed read the state from
+  disk.
+
+  In cases like restore, we modify the table files directly,
+  bypassing the MyISAM interface. We do this inside of mysqld, so
+  --external-locking need not be specified. We support this case by the
+  'force' parameter.
+*/
+
+uint mi_state_info_read_dsk(File file, MI_STATE_INFO *state, my_bool pRead,
+                            my_bool force)
 {
   uchar	buff[MI_STATE_INFO_SIZE + MI_STATE_EXTRA_SIZE];
 
-  if (!myisam_single_user)
+  if (!myisam_single_user || force)
   {
     if (pRead)
     {

=== modified file 'storage/myisam/myisam_backup_engine.cc'
--- a/storage/myisam/myisam_backup_engine.cc	2008-07-14 15:33:03 +0000
+++ b/storage/myisam/myisam_backup_engine.cc	2008-07-23 12:46:33 +0000
@@ -1758,7 +1758,6 @@ result_t Table_restore::close()
   {
     MI_INFO      *mi_info;
     MYISAM_SHARE *share;
-    my_bool      old_myisam_single_user;
 
     mi_info= mi_open(file_name.ptr(), O_RDWR, HA_OPEN_FOR_REPAIR);
     if (mi_info == NULL)
@@ -1766,14 +1765,8 @@ result_t Table_restore::close()
     share= mi_info->s;
     DBUG_PRINT("myisam_backup", ("share data_file: %lu",
                                  (ulong) share->state.state.data_file_length));
-    old_myisam_single_user= myisam_single_user;
-    myisam_single_user= FALSE;
-    if (mi_state_info_read_dsk(share->kfile,&share->state,1))
-    {
-      myisam_single_user= old_myisam_single_user;
+    if (mi_state_info_read_dsk(share->kfile, &share->state, 1, 1))
       goto err;
-    }
-    myisam_single_user= old_myisam_single_user;
     DBUG_PRINT("myisam_backup", ("share data_file: %lu",
                                  (ulong) share->state.state.data_file_length));
     /*

=== modified file 'storage/myisam/myisamdef.h'
--- a/storage/myisam/myisamdef.h	2008-07-09 08:19:03 +0000
+++ b/storage/myisam/myisamdef.h	2008-07-23 12:46:33 +0000
@@ -799,7 +799,8 @@ uint mi_state_info_write(MYISAM_SHARE *s
                          MI_STATE_INFO *state, uint pWrite);
 uchar *mi_state_info_read(uchar *ptr, MI_STATE_INFO *state);
 int mi_remap_file_and_write_state_for_unlock(MI_INFO *info);
-uint mi_state_info_read_dsk(File file, MI_STATE_INFO *state, my_bool pRead);
+uint mi_state_info_read_dsk(File file, MI_STATE_INFO *state, my_bool pRead,
+                            my_bool force);
 uint mi_base_info_write(File file, MI_BASE_INFO *base);
 uchar *mi_n_base_info_read(uchar *ptr, MI_BASE_INFO *base);
 int mi_keyseg_write(File file, const HA_KEYSEG *keyseg);