MDEV-12761 Error return from external_lock make the server crash

vuvova · vuvova · commit f9264280d68d · 2017-05-15T22:23:10.000+02:00
bunch of bugs when external_lock() fails on unlock:
* mi_lock_database() used mi_mark_crashed() under share-&gt;intern_lock,
  but mi_mark_crashed() itself locks this mutex.
* handler::close() required table to be unlocked, but failed
  external_lock didn't count as unlock
* mysql_unlock_tables() ignored all unlock errors, but they still set
  the error status in stmt_da.
diff --git a/mysql-test/r/myisam_debug.result b/mysql-test/r/myisam_debug.result
@@ -27,3 +27,15 @@ CHECK TABLE t1;
 Table	Op	Msg_type	Msg_text
 test.t1	check	status	OK
 DROP TABLE t1,t2;
+call mtr.add_suppression('Incorrect key file for table');
+create table t1 (a int, index(a));
+lock tables t1 write;
+insert t1 values (1),(2),(1);
+set @old_dbug=@@debug_dbug;
+set debug_dbug='+d,mi_lock_database_failure';
+unlock tables;
+Warnings:
+Error	126	Incorrect key file for table './test/t1.MYI'; try to repair it
+Error	1015	Can't lock file (errno: 22 "Invalid argument")
+set debug_dbug=@old_dbug;
+drop table t1;
diff --git a/mysql-test/t/myisam_debug.test b/mysql-test/t/myisam_debug.test
@@ -59,3 +59,16 @@ KILL QUERY @thread_id;
 CHECK TABLE t1; 
 DROP TABLE t1,t2;
 DISCONNECT insertConn;
+
+#
+# MDEV-12761 Error return from external_lock make the server crash
+#
+call mtr.add_suppression('Incorrect key file for table');
+create table t1 (a int, index(a));
+lock tables t1 write;
+insert t1 values (1),(2),(1);
+set @old_dbug=@@debug_dbug;
+set debug_dbug='+d,mi_lock_database_failure';
+unlock tables;
+set debug_dbug=@old_dbug;
+drop table t1;
diff --git a/sql/handler.cc b/sql/handler.cc
@@ -5944,7 +5944,7 @@ int handler::ha_external_lock(THD *thd, int lock_type)
   MYSQL_TABLE_LOCK_WAIT(m_psi, PSI_TABLE_EXTERNAL_LOCK, lock_type,
     { error= external_lock(thd, lock_type); })
 
-  if (error == 0)
+  if (error == 0 || lock_type == F_UNLCK)
   {
     m_lock_type= lock_type;
     cached_table_flags= table_flags();
diff --git a/sql/lock.cc b/sql/lock.cc
@@ -380,12 +380,15 @@ static int lock_external(THD *thd, TABLE **tables, uint count)
 void mysql_unlock_tables(THD *thd, MYSQL_LOCK *sql_lock, bool free_lock)
 {
   DBUG_ENTER("mysql_unlock_tables");
+  bool errors= thd->is_error();
   if (sql_lock->table_count)
     unlock_external(thd, sql_lock->table, sql_lock->table_count);
   if (sql_lock->lock_count)
     thr_multi_unlock(sql_lock->locks, sql_lock->lock_count, 0);
   if (free_lock)
     my_free(sql_lock);
+  if (!errors)
+    thd->clear_error();
   DBUG_VOID_RETURN;
 }
 
diff --git a/storage/myisam/mi_locking.c b/storage/myisam/mi_locking.c
@@ -29,7 +29,7 @@ static void mi_update_status_with_lock(MI_INFO *info);
 
 int mi_lock_database(MI_INFO *info, int lock_type)
 {
-  int error;
+  int error, mark_crashed= 0;
   uint count;
   MYISAM_SHARE *share=info->s;
   DBUG_ENTER("mi_lock_database");
@@ -52,6 +52,7 @@ int mi_lock_database(MI_INFO *info, int lock_type)
   }
 
   error= 0;
+  DBUG_EXECUTE_IF ("mi_lock_database_failure", error= EINVAL;);
   mysql_mutex_lock(&share->intern_lock);
   if (share->kfile >= 0)		/* May only be false on windows */
   {
@@ -75,17 +76,15 @@ int mi_lock_database(MI_INFO *info, int lock_type)
                                                       &share->dirty_part_map,
                                                       FLUSH_KEEP))
       {
-	error=my_errno;
+	mark_crashed= error=my_errno;
         mi_print_error(info->s, HA_ERR_CRASHED);
-	mi_mark_crashed(info);		/* Mark that table must be checked */
       }
       if (info->opt_flag & (READ_CACHE_USED | WRITE_CACHE_USED))
       {
 	if (end_io_cache(&info->rec_cache))
 	{
-	  error=my_errno;
+	  mark_crashed= error=my_errno;
           mi_print_error(info->s, HA_ERR_CRASHED);
-	  mi_mark_crashed(info);
 	}
       }
       if (!count)
@@ -110,22 +109,19 @@ int mi_lock_database(MI_INFO *info, int lock_type)
 	  share->state.unique=   info->last_unique=  info->this_unique;
 	  share->state.update_count= info->last_loop= ++info->this_loop;
           if (mi_state_info_write(share->kfile, &share->state, 1))
-	    error=my_errno;
+	    mark_crashed= error=my_errno;
 	  share->changed=0;
 	  if (myisam_flush)
 	  {
             if (mysql_file_sync(share->kfile, MYF(0)))
-	      error= my_errno;
+	      mark_crashed= error= my_errno;
             if (mysql_file_sync(info->dfile, MYF(0)))
-	      error= my_errno;
+	      mark_crashed= error= my_errno;
 	  }
 	  else
 	    share->not_flushed=1;
 	  if (error)
-          {
             mi_print_error(info->s, HA_ERR_CRASHED);
-	    mi_mark_crashed(info);
-          }
 	}
 	if (info->lock_type != F_EXTRA_LCK)
 	{
@@ -260,6 +256,8 @@ int mi_lock_database(MI_INFO *info, int lock_type)
   }
 #endif
   mysql_mutex_unlock(&share->intern_lock);
+  if (mark_crashed)
+    mi_mark_crashed(info);
   DBUG_RETURN(error);
 } /* mi_lock_database */
 

Original file line number	Diff line number	Diff line change
`@@ -5944,7 +5944,7 @@ int handler::ha_external_lock(THD *thd, int lock_type)`
`5944`	`5944`	`MYSQL_TABLE_LOCK_WAIT(m_psi, PSI_TABLE_EXTERNAL_LOCK, lock_type,`
`5945`	`5945`	`{ error= external_lock(thd, lock_type); })`
`5946`	`5946`
`5947`		`- if (error == 0)`
	`5947`	`+ if (error == 0 \|\| lock_type == F_UNLCK)`
`5948`	`5948`	`{`
`5949`	`5949`	`m_lock_type= lock_type;`
`5950`	`5950`	`cached_table_flags= table_flags();`
Original file line number	Diff line number	Diff line change
`@@ -29,7 +29,7 @@ static void mi_update_status_with_lock(MI_INFO *info);`
`29`	`29`
`30`	`30`	`int mi_lock_database(MI_INFO *info, int lock_type)`
`31`	`31`	`{`
`32`		`- int error;`
	`32`	`+ int error, mark_crashed= 0;`
`33`	`33`	`uint count;`
`34`	`34`	`MYISAM_SHARE *share=info->s;`
`35`	`35`	`DBUG_ENTER("mi_lock_database");`
`@@ -52,6 +52,7 @@ int mi_lock_database(MI_INFO *info, int lock_type)`
`52`	`52`	`}`
`53`	`53`
`54`	`54`	`error= 0;`
	`55`	`+ DBUG_EXECUTE_IF ("mi_lock_database_failure", error= EINVAL;);`
`55`	`56`	`mysql_mutex_lock(&share->intern_lock);`
`56`	`57`	`if (share->kfile >= 0) /* May only be false on windows */`
`57`	`58`	`{`
`@@ -75,17 +76,15 @@ int mi_lock_database(MI_INFO *info, int lock_type)`
`75`	`76`	`&share->dirty_part_map,`
`76`	`77`	`FLUSH_KEEP))`
`77`	`78`	`{`
`78`		`- error=my_errno;`
	`79`	`+ mark_crashed= error=my_errno;`
`79`	`80`	`mi_print_error(info->s, HA_ERR_CRASHED);`
`80`		`- mi_mark_crashed(info); /* Mark that table must be checked */`
`81`	`81`	`}`
`82`	`82`	`if (info->opt_flag & (READ_CACHE_USED \| WRITE_CACHE_USED))`
`83`	`83`	`{`
`84`	`84`	`if (end_io_cache(&info->rec_cache))`
`85`	`85`	`{`
`86`		`- error=my_errno;`
	`86`	`+ mark_crashed= error=my_errno;`
`87`	`87`	`mi_print_error(info->s, HA_ERR_CRASHED);`
`88`		`- mi_mark_crashed(info);`
`89`	`88`	`}`
`90`	`89`	`}`
`91`	`90`	`if (!count)`
`@@ -110,22 +109,19 @@ int mi_lock_database(MI_INFO *info, int lock_type)`
`110`	`109`	`share->state.unique= info->last_unique= info->this_unique;`
`111`	`110`	`share->state.update_count= info->last_loop= ++info->this_loop;`
`112`	`111`	`if (mi_state_info_write(share->kfile, &share->state, 1))`
`113`		`- error=my_errno;`
	`112`	`+ mark_crashed= error=my_errno;`
`114`	`113`	`share->changed=0;`
`115`	`114`	`if (myisam_flush)`
`116`	`115`	`{`
`117`	`116`	`if (mysql_file_sync(share->kfile, MYF(0)))`
`118`		`- error= my_errno;`
	`117`	`+ mark_crashed= error= my_errno;`
`119`	`118`	`if (mysql_file_sync(info->dfile, MYF(0)))`
`120`		`- error= my_errno;`
	`119`	`+ mark_crashed= error= my_errno;`
`121`	`120`	`}`
`122`	`121`	`else`
`123`	`122`	`share->not_flushed=1;`
`124`	`123`	`if (error)`
`125`		`- {`
`126`	`124`	`mi_print_error(info->s, HA_ERR_CRASHED);`
`127`		`- mi_mark_crashed(info);`
`128`		`- }`
`129`	`125`	`}`
`130`	`126`	`if (info->lock_type != F_EXTRA_LCK)`
`131`	`127`	`{`
`@@ -260,6 +256,8 @@ int mi_lock_database(MI_INFO *info, int lock_type)`
`260`	`256`	`}`
`261`	`257`	`#endif`
`262`	`258`	`mysql_mutex_unlock(&share->intern_lock);`
	`259`	`+ if (mark_crashed)`
	`260`	`+ mi_mark_crashed(info);`
`263`	`261`	`DBUG_RETURN(error);`
`264`	`262`	`} /* mi_lock_database */`
`265`	`263`