bk commit into 5.1 tree (acurtis:1.2482) BUG#25679

[antony]

Below is the list of changes that have just been committed into a local
5.1 repository of antony. When antony 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-03-15 10:37:48-07:00, acurtis@stripped +9 -0
  Bug #25679 Denial-of-Service against entire server if FEDERATED engine is compiled
  
  Bug fixed:
  * Denial of service caused by LOCK_open mutex held during table creation operation
    when table creation requires network communication and remote server is timing out.
    This prevents all other sessions from opening any tables.
  
  This patch provides:
  * Name lock is acquired when creating a non-temporary table.
  * LOCK_open mutex is released when calling storage engine's ::create()/::open() method
  * Flag to indicate that storage engine still requires the LOCK_open mutex to be held.

  mysql-test/r/federated.result@stripped, 2007-03-15 10:37:42-07:00, acurtis@stripped +26 -0
    Bug #25679 Denial-of-Service against entire server if FEDERATED engine is compiled
    
    New results

  mysql-test/t/federated.test@stripped, 2007-03-15 10:37:42-07:00, acurtis@stripped +38 -0
    Bug #25679 Denial-of-Service against entire server if FEDERATED engine is compiled
    
    Test for bug by creating a federated table connection to same mysqld instance.
    Denial-of-service as reported by user is caused by LOCK_open mutex being held while
    attempting network connection to remote server.
    
    We should have a further test to test he ::open path but thats harder to arrange.

  sql/ha_ndbcluster.cc@stripped, 2007-03-15 10:37:42-07:00, acurtis@stripped +1 -0
    Bug #25679 Denial-of-Service against entire server if FEDERATED engine is compiled
    
    NDB requires LOCK_open mutex to be held during call to ::create()
    This is due to ndb binlog accessing mysqld internal table share structures.

  sql/handler.h@stripped, 2007-03-15 10:37:43-07:00, acurtis@stripped +1 -0
    Bug #25679 Denial-of-Service against entire server if FEDERATED engine is compiled
    
    New HTON flag to indicate that storage engine ::create() method needs LOCK_open mutex
    to be held when it is invoked.

  sql/sql_base.cc@stripped, 2007-03-15 10:37:43-07:00, acurtis@stripped +26 -0
    Bug #25679 Denial-of-Service against entire server if FEDERATED engine is compiled
    
    If there is a name-lock held which is for a table currently being created, the table
    logically does not exist yet. Report the non-existance of the table.
    
    When we are opening a table, we have a reference on the share so we can release the
    LOCK_open mutex but we reaquire it later.

  sql/sql_delete.cc@stripped, 2007-03-15 10:37:43-07:00, acurtis@stripped +11 -4
    Bug #25679 Denial-of-Service against entire server if FEDERATED engine is compiled
    
    Release the LOCK_open mutex when opening a table. A name lock is being held so it is
ok

  sql/sql_table.cc@stripped, 2007-03-15 10:37:43-07:00, acurtis@stripped +89 -23
    Bug #25679 Denial-of-Service against entire server if FEDERATED engine is compiled
    
    Acquire a name lock and release the LOCK_open mutex when opening a table

  sql/table.h@stripped, 2007-03-15 10:37:43-07:00, acurtis@stripped +1 -0
    Bug #25679 Denial-of-Service against entire server if FEDERATED engine is compiled
    
    New table flag to indicate that the name lock is for a table currently being created.

  sql/unireg.cc@stripped, 2007-03-15 10:37:43-07:00, acurtis@stripped +1 -1
    Bug #25679 Denial-of-Service against entire server if FEDERATED engine is compiled
    
    Style fix.

# This is a BitKeeper patch.  What follows are the unified diffs for the
# set of deltas contained in the patch.  The rest of the patch, the part
# that BitKeeper cares about, is below these diffs.
# User:	acurtis
# Host:	ltamd64.xiphis.org
# Root:	/home/antony/work2/p2-bug25679.2

--- 1.252/sql/handler.h	2007-03-15 10:38:01 -07:00
+++ 1.253/sql/handler.h	2007-03-15 10:38:01 -07:00
@@ -698,6 +698,7 @@
 #define HTON_TEMPORARY_NOT_SUPPORTED (1 << 6) //Having temporary tables not
supported
 #define HTON_SUPPORT_LOG_TABLES      (1 << 7) //Engine supports log tables
 #define HTON_NO_PARTITION            (1 << 8) //You can not partition these tables
+#define HTON_CREATE_NEEDS_OPEN_MUTEX (1 << 9) //::create() needs LOCK_open held
 
 typedef struct st_thd_trans
 {

--- 1.386/sql/sql_base.cc	2007-03-15 10:38:01 -07:00
+++ 1.387/sql/sql_base.cc	2007-03-15 10:38:01 -07:00
@@ -2177,6 +2177,17 @@
                                  &state))
   {
     /*
+      If the table is name locked by a CREATE TABLE operation,
+      we immediately report that the table does not exist.
+    */
+    if (table->s->create_in_progress && table->in_use != thd)
+    {
+      pthread_mutex_unlock(&LOCK_open);
+      my_error(ER_NO_SUCH_TABLE, MYF(0), table_list->db, table_list->alias);
+      DBUG_RETURN(0);
+    }
+	
+    /*
       Here we flush tables marked for flush. However we never flush log
       tables here. They are flushed only on FLUSH LOGS.
       Normally, table->s->version contains the value of
@@ -2887,6 +2898,10 @@
     release_table_share(share, RELEASE_NORMAL);
     DBUG_RETURN((flags & OPEN_VIEW_NO_PARSE)? -1 : 0);
   }
+  
+  /* We have a ref on the table share so we can unlock LOCK_open */
+  if (!(share->db_type->flags & HTON_CREATE_NEEDS_OPEN_MUTEX))
+    pthread_mutex_unlock(&LOCK_open);
 
   while ((error= open_table_from_share(thd, share, alias,
                                        (uint) (HA_OPEN_KEYFILE |
@@ -2897,6 +2912,9 @@
                                         EXTRA_RECORD),
                                        thd->open_options, entry, FALSE)))
   {
+    if (!(share->db_type->flags & HTON_CREATE_NEEDS_OPEN_MUTEX))
+      pthread_mutex_lock(&LOCK_open);
+
     if (error == 7)                             // Table def changed
     {
       share->version= 0;                        // Mark share as old
@@ -2977,12 +2995,16 @@
  
      if (error)
        goto err;
+     if (!(share->db_type->flags & HTON_CREATE_NEEDS_OPEN_MUTEX))
+       pthread_mutex_unlock(&LOCK_open);
      break;
    }
 
   if (Table_triggers_list::check_n_load(thd, share->db.str,
                                         share->table_name.str, entry, 0))
   {
+    if (!(share->db_type->flags & HTON_CREATE_NEEDS_OPEN_MUTEX))
+      pthread_mutex_lock(&LOCK_open);
     closefrm(entry, 0);
     goto err;
   }
@@ -3018,11 +3040,15 @@
                         "to write 'DELETE FROM `%s`.`%s`' to the binary log",
                         table_list->db, table_list->table_name);
         delete entry->triggers;
+        if (!(share->db_type->flags & HTON_CREATE_NEEDS_OPEN_MUTEX))
+          pthread_mutex_lock(&LOCK_open);
         closefrm(entry, 0);
         goto err;
       }
     }
   }
+  if (!(share->db_type->flags & HTON_CREATE_NEEDS_OPEN_MUTEX))
+    pthread_mutex_lock(&LOCK_open);
   DBUG_RETURN(0);
 
 err:

--- 1.210/sql/sql_delete.cc	2007-03-15 10:38:01 -07:00
+++ 1.211/sql/sql_delete.cc	2007-03-15 10:38:01 -07:00
@@ -865,6 +865,7 @@
   char path[FN_REFLEN];
   TABLE *table;
   bool error;
+  bool needs_open_mutex= false;
   uint closed_log_tables= 0, lock_logger= 0;
   uint path_length;
   uint log_type;
@@ -904,6 +905,7 @@
   if (!dont_send_ok)
   {
     enum legacy_db_type table_type;
+	handlerton *hton;
     mysql_frm_type(thd, path, &table_type);
     if (table_type == DB_TYPE_UNKNOWN)
     {
@@ -911,12 +913,15 @@
                table_list->db, table_list->table_name);
       DBUG_RETURN(TRUE);
     }
-    if (!ha_check_storage_engine_flag(ha_resolve_by_legacy_type(thd, table_type),
-                                      HTON_CAN_RECREATE))
+    hton= ha_resolve_by_legacy_type(thd, table_type);
+    if (!ha_check_storage_engine_flag(hton, HTON_CAN_RECREATE))
       goto trunc_by_del;
 
     if (lock_and_wait_for_table_name(thd, table_list))
       DBUG_RETURN(TRUE);
+
+    needs_open_mutex=
+      ha_check_storage_engine_flag(hton, HTON_CREATE_NEEDS_OPEN_MUTEX);
   }
 
   log_type= check_if_log_table(table_list->db_length, table_list->db,
@@ -935,10 +940,12 @@
   // crashes, replacement works.  *(path + path_length - reg_ext_length)=
   // '\0';
   path[path_length - reg_ext_length] = 0;
-  VOID(pthread_mutex_lock(&LOCK_open));
+  if (needs_open_mutex)
+    pthread_mutex_lock(&LOCK_open);
   error= ha_create_table(thd, path, table_list->db, table_list->table_name,
                          &create_info, 1);
-  VOID(pthread_mutex_unlock(&LOCK_open));
+  if (needs_open_mutex)
+    pthread_mutex_unlock(&LOCK_open);
   query_cache_invalidate3(thd, table_list, 0);
 
 end:

--- 1.393/sql/sql_table.cc	2007-03-15 10:38:01 -07:00
+++ 1.394/sql/sql_table.cc	2007-03-15 10:38:01 -07:00
@@ -3203,6 +3203,8 @@
   KEY		*key_info_buffer;
   HA_CREATE_INFO *create_info;
   handler	*file;
+  TABLE_LIST table_list[1];
+  bool release_lock_open;
   bool		error= TRUE;
   DBUG_ENTER("mysql_create_table_internal");
   DBUG_PRINT("enter", ("db: '%s'  table: '%s'  tmp: %d",
@@ -3499,10 +3501,53 @@
     create_info->data_file_name= create_info->index_file_name= 0;
   create_info->table_options=db_options;
 
+  if (!(create_info->options & HA_LEX_CREATE_TMP_TABLE))
+  {
+    int rc;
+    /* obtain a name lock */
+    bzero(&table_list, sizeof(TABLE_LIST));
+    table_list[0].db= (char *) db;
+    table_list[0].table_name= (char *) table_name;
+    if ((rc= lock_table_name(thd, table_list, TRUE)))
+    {
+      if (rc < 0)
+      {
+        my_printf_error(ER_UNKNOWN_ERROR,
+                        "Unable to acquire name lock on '%-.64s'", 
+                        MYF(0), table_name);
+	    goto unlock_and_end;
+      }
+      my_error(ER_TABLE_EXISTS_ERROR, MYF(0), table_name);
+      unlock_table_name(thd, table_list);
+      goto unlock_and_end;
+    }
+    /* Indicate that lock is a create name lock. */
+    table_list[0].table->s->create_in_progress= TRUE;
+  }
+
+  /*
+    Release the LOCK_open mutex, we are holding a name lock.
+    By releasing the LOCK_open mutex, we are permitting other queries
+    to be executed while table creation is in progress.
+  */
+  if ((release_lock_open= !(file->ht->flags & HTON_CREATE_NEEDS_OPEN_MUTEX)))
+    pthread_mutex_unlock(&LOCK_open);
+
   path[path_length - reg_ext_length]= '\0'; // Remove .frm extension
   if (rea_create_table(thd, path, db, table_name, create_info, fields,
                        key_count, key_info_buffer, file))
-    goto unlock_and_end;
+  {
+    if (!(create_info->options & HA_LEX_CREATE_TMP_TABLE))
+    {
+      if (release_lock_open)
+        pthread_mutex_lock(&LOCK_open);
+      unlock_table_name(thd, table_list);
+      goto unlock_and_end;
+	}
+    if (release_lock_open)
+	  goto err;
+	goto unlock_and_end;
+  }
 
   if (create_info->options & HA_LEX_CREATE_TMP_TABLE)
   {
@@ -3510,7 +3555,9 @@
     if (!(open_temporary_table(thd, path, db, table_name, 1)))
     {
       (void) rm_temporary_table(create_info->db_type, path);
-      goto unlock_and_end;
+      if (!release_lock_open)
+    	pthread_mutex_unlock(&LOCK_open);
+      goto err;
     }
     thd->tmp_table_used= 1;
   }
@@ -3528,8 +3575,17 @@
         !(create_info->options & HA_LEX_CREATE_TMP_TABLE))))
     write_bin_log(thd, TRUE, thd->query, thd->query_length);
   error= FALSE;
-unlock_and_end:
-  VOID(pthread_mutex_unlock(&LOCK_open));
+
+  if (!(create_info->options & HA_LEX_CREATE_TMP_TABLE))
+  {
+    if (release_lock_open)
+      pthread_mutex_lock(&LOCK_open);
+    unlock_table_name(thd, table_list);
+    goto unlock_and_end;
+  }
+  
+  if (!release_lock_open)
+    goto unlock_and_end;
 
 err:
   thd->proc_info="After create";
@@ -3542,7 +3598,10 @@
                       ER_TABLE_EXISTS_ERROR, ER(ER_TABLE_EXISTS_ERROR),
                       alias);
   create_info->table_existed= 1;		// Mark that table existed
-  goto unlock_and_end;
+
+unlock_and_end:
+  VOID(pthread_mutex_unlock(&LOCK_open));
+  goto err;
 }
 
 
@@ -4606,7 +4665,8 @@
   char *src_table= table_ident->table.str;
   int  err;
   bool res= TRUE, unlock_dst_table= FALSE;
-  enum legacy_db_type not_used;
+  bool needs_open_mutex= FALSE;
+  enum legacy_db_type table_type;
   HA_CREATE_INFO *create_info;
 #ifdef WITH_PARTITION_STORAGE_ENGINE
   char tmp_path[FN_REFLEN];
@@ -4657,7 +4717,7 @@
   /* 
      create like should be not allowed for Views, Triggers, ... 
   */
-  if (mysql_frm_type(thd, src_path, &not_used) != FRMTYPE_TABLE)
+  if (mysql_frm_type(thd, src_path, &table_type) != FRMTYPE_TABLE)
   {
     my_error(ER_WRONG_OBJECT, MYF(0), src_db, src_table, "BASE TABLE");
     goto err;
@@ -4710,8 +4770,25 @@
   {
     dst_path_length= build_table_filename(dst_path, sizeof(dst_path),
                                           db, table_name, reg_ext, 0);
+
+    bzero((gptr)&dst_tables_list, sizeof(dst_tables_list));
+    dst_tables_list.db= table->db;
+    dst_tables_list.table_name= table->table_name;
+
+    /*
+      lock destination table name
+    */
+    if (lock_and_wait_for_table_name(thd, &dst_tables_list))
+      goto err;
+    else
+      unlock_dst_table= TRUE;
+
     if (!access(dst_path, F_OK))
       goto table_exists;
+
+    needs_open_mutex=
+	  ha_check_storage_engine_flag(ha_resolve_by_legacy_type(thd, table_type),
+                                   HTON_CREATE_NEEDS_OPEN_MUTEX);
   }
 
   /*
@@ -4745,9 +4822,11 @@
   my_copy(src_path, dst_path, MYF(MY_DONT_OVERWRITE_FILE));
 #endif
   dst_path[dst_path_length - reg_ext_length]= '\0';  // Remove .frm
-  pthread_mutex_lock(&LOCK_open);  
+  if (needs_open_mutex)
+    pthread_mutex_lock(&LOCK_open);
   err= ha_create_table(thd, dst_path, db, table_name, create_info, 1);
-  pthread_mutex_unlock(&LOCK_open);
+  if (needs_open_mutex)
+    pthread_mutex_unlock(&LOCK_open);
   if (create_info->options & HA_LEX_CREATE_TMP_TABLE)
   {
     if (err || !open_temporary_table(thd, dst_path, db, table_name, 1))
@@ -4802,21 +4881,8 @@
           store_create_info() to work. The table will be closed
           by close_thread_tables() at the end of the statement.
         */
-        if (open_tables(thd, &table, &counter, 0))
+        if (open_tables(thd, &table, &counter, MYSQL_LOCK_IGNORE_FLUSH))
           goto err;
-
-        bzero((gptr)&dst_tables_list, sizeof(dst_tables_list));
-        dst_tables_list.db= table->db;
-        dst_tables_list.table_name= table->table_name;
-
-        /*
-          lock destination table name, to make sure that nobody
-          can drop/alter the table while we execute store_create_info()
-        */
-        if (lock_and_wait_for_table_name(thd, &dst_tables_list))
-          goto err;
-        else
-          unlock_dst_table= TRUE;
 
         IF_DBUG(int result=) store_create_info(thd, table, &query,
                                                create_info);

--- 1.166/sql/table.h	2007-03-15 10:38:01 -07:00
+++ 1.167/sql/table.h	2007-03-15 10:38:01 -07:00
@@ -209,6 +209,7 @@
   bool crashed;
   bool is_view;
   bool name_lock, replace_with_name_lock;
+  bool create_in_progress;
   bool waiting_on_cond;                 /* Protection against free */
   ulong table_map_id;                   /* for row-based replication */
   ulonglong table_map_version;

--- 1.98/sql/unireg.cc	2007-03-15 10:38:01 -07:00
+++ 1.99/sql/unireg.cc	2007-03-15 10:38:01 -07:00
@@ -344,9 +344,9 @@
                      List<create_field> &create_fields,
                      uint keys, KEY *key_info, handler *file)
 {
+  char frm_name[FN_REFLEN];
   DBUG_ENTER("rea_create_table");
 
-  char frm_name[FN_REFLEN];
   strxmov(frm_name, path, reg_ext, NullS);
   if (mysql_create_frm(thd, frm_name, db, table_name, create_info,
                        create_fields, keys, key_info, file))

--- 1.419/sql/ha_ndbcluster.cc	2007-03-15 10:38:01 -07:00
+++ 1.420/sql/ha_ndbcluster.cc	2007-03-15 10:38:01 -07:00
@@ -6764,6 +6764,7 @@
     ndbcluster_binlog_init_handlerton();
 #endif
     h->flags=            HTON_CAN_RECREATE | HTON_TEMPORARY_NOT_SUPPORTED;
+    h->flags|=           HTON_CREATE_NEEDS_OPEN_MUTEX;
     h->discover=         ndbcluster_discover;
     h->find_files= ndbcluster_find_files;
     h->table_exists_in_engine= ndbcluster_table_exists_in_engine;

--- 1.44/mysql-test/r/federated.result	2007-03-15 10:38:01 -07:00
+++ 1.45/mysql-test/r/federated.result	2007-03-15 10:38:01 -07:00
@@ -1843,6 +1843,32 @@
 D18DD184D184D0B5D0BAD182D0B8D0B2D0BDD183D18E
 drop table federated.t1;
 drop table federated.t1;
+create table federated.t1 (a int not null, b varchar(64), primary key (a))
+engine=MyISAM DEFAULT CHARSET=utf8;
+create table federated.t2 (
+a int not null, b varchar(64), primary key (a)
+) DEFAULT CHARSET=utf8 ENGINE=Federated
+connection='mysql://root@stripped:MASTER_PORT/federated/t1';
+insert into federated.t1 (a,b) values (1, "foo");
+insert into federated.t1 (a,b) values (2, "bar");
+insert into federated.t2 (a,b) values (3, "hello");
+flush tables;
+select * from federated.t2 where a = 1;
+a	b
+1	foo
+select * from federated.t2 where a = 2;
+a	b
+2	bar
+select * from federated.t1 where a = 3;
+a	b
+3	hello
+create table federated.t3 (
+a int not null, b varchar(64), primary key (a)
+) DEFAULT CHARSET=utf8 ENGINE=Federated
+connection='mysql://root@stripped:MASTER_PORT/federated/t3';
+ERROR HY000: Can't create federated table. Foreign data src error:  error: 1146  'Table
'federated.t3' doesn't exist'
+drop table federated.t2;
+drop table federated.t1;
 DROP TABLE IF EXISTS federated.t1;
 DROP DATABASE IF EXISTS federated;
 DROP TABLE IF EXISTS federated.t1;

--- 1.36/mysql-test/t/federated.test	2007-03-15 10:38:01 -07:00
+++ 1.37/mysql-test/t/federated.test	2007-03-15 10:38:01 -07:00
@@ -1629,5 +1629,43 @@
 connection slave;
 drop table federated.t1;
 
+#
+#  BUG#25679 Denial-of-Service against entire server if FEDERATED engine is compiled
+# Deadlock caused by LOCK_open mutex being held during ::create operation
+# which causes server to sleep until network timeout if remote server
+# is inaccessible. When connecting to self, it causes a deadlock.
+#
+connection master;
+create table federated.t1 (a int not null, b varchar(64), primary key (a))
+engine=MyISAM DEFAULT CHARSET=utf8;
+--replace_result $MASTER_MYPORT MASTER_PORT
+eval create table federated.t2 (
+  a int not null, b varchar(64), primary key (a)
+) DEFAULT CHARSET=utf8 ENGINE=Federated
+  connection='mysql://root@stripped:$MASTER_MYPORT/federated/t1';
+# check that we are looking at the same table
+insert into federated.t1 (a,b) values (1, "foo");
+insert into federated.t1 (a,b) values (2, "bar");
+insert into federated.t2 (a,b) values (3, "hello");
+flush tables;
+select * from federated.t2 where a = 1;
+select * from federated.t2 where a = 2;
+select * from federated.t1 where a = 3;
+
+# Check infinite recursion with CREATE TABLE
+# it will fail with 'remote' server saying table doesn't exist
+--replace_result $MASTER_MYPORT MASTER_PORT
+--error ER_CANT_CREATE_FEDERATED_TABLE
+eval create table federated.t3 (
+  a int not null, b varchar(64), primary key (a)
+) DEFAULT CHARSET=utf8 ENGINE=Federated
+  connection='mysql://root@stripped:$MASTER_MYPORT/federated/t3';
+
+# If the user manages to make a self-referencing federated table,
+# it will still cause a D.O.S. when mysqld creates a lot of
+# network connections to itself.
+drop table federated.t2;
+drop table federated.t1;
+
 
 source include/federated_cleanup.inc;