From: Date: July 23 2008 2:46pm Subject: bzr commit into mysql-6.0-backup branch (ingo.struewing:2672) Bug#38108 List-Archive: http://lists.mysql.com/commits/50309 X-Bug: 38108 Message-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit #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);