From: Date: April 3 2007 6:50am Subject: bk commit into 5.1 tree (acurtis:1.2551) BUG#25679 List-Archive: http://lists.mysql.com/commits/23618 X-Bug: 25679 Message-Id: <200704030550.l335oG97021167@mail.mysql.com> 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-04-02 21:49:53-07:00, acurtis@stripped +10 -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 mysql-test/r/federated.result@stripped, 2007-04-02 21:49:46-07:00, acurtis@stripped +26 -0 Bug25679 Denial-of-Service against entire server if FEDERATED engine is compiled Simple test for bug. mysql-test/t/federated.test@stripped, 2007-04-02 21:49:47-07:00, acurtis@stripped +38 -0 Bug25679 Denial-of-Service against entire server if FEDERATED engine is compiled Simple test for bug. sql/ha_ndbcluster.cc@stripped, 2007-04-02 21:49:47-07:00, acurtis@stripped +2 -1 Bug25679 Denial-of-Service against entire server if FEDERATED engine is compiled LOCK_open is not held when calling ::create() sql/ha_ndbcluster_binlog.cc@stripped, 2007-04-02 21:49:47-07:00, acurtis@stripped +19 -2 Bug25679 Denial-of-Service against entire server if FEDERATED engine is compiled ndbcluster_binlog_open_table() only needs LOCK_open when calling assign_new_table_id() ndb_handle_schema_change() should acquire name lock before rewriting frm sql/handler.cc@stripped, 2007-04-02 21:49:47-07:00, acurtis@stripped +49 -7 Bug25679 Denial-of-Service against entire server if FEDERATED engine is compiled Acquire a creation name lock before attempting to find a table by discovery. sql/sql_base.cc@stripped, 2007-04-02 21:49:47-07:00, acurtis@stripped +20 -0 Bug25679 Denial-of-Service against entire server if FEDERATED engine is compiled When a name-lock has already been obtained and it is for a table creation, then the table does not exist yet - no need to wait, just report error to user. sql/sql_delete.cc@stripped, 2007-04-02 21:49:47-07:00, acurtis@stripped +0 -2 Bug25679 Denial-of-Service against entire server if FEDERATED engine is compiled no need for LOCK_open when calling ha_create_table() sql/sql_table.cc@stripped, 2007-04-02 21:49:48-07:00, acurtis@stripped +69 -23 Bug25679 Denial-of-Service against entire server if FEDERATED engine is compiled Name lock tables before creating them so that LOCK_open can be released earlier. Use a 'creation' name lock so that other threads attempting to use the as-yet non- existant table will abort with table not found. sql/table.h@stripped, 2007-04-02 21:49:48-07:00, acurtis@stripped +1 -0 Bug25679 Denial-of-Service against entire server if FEDERATED engine is compiled new table share flag to indicate that a table creation operation is in progress. sql/unireg.cc@stripped, 2007-04-02 21:49:48-07:00, acurtis@stripped +1 -1 Bug25679 Denial-of-Service against entire server if FEDERATED engine is compiled style fix - declare variables before DBUG_ENTER() # 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.3 --- 1.301/sql/handler.cc 2007-04-02 21:50:07 -07:00 +++ 1.302/sql/handler.cc 2007-04-02 21:50:07 -07:00 @@ -2614,14 +2614,49 @@ HA_CREATE_INFO create_info; TABLE table; TABLE_SHARE share; + TABLE_LIST table_list[1]; DBUG_ENTER("ha_create_table_from_engine"); DBUG_PRINT("enter", ("name '%s'.'%s'", db, name)); bzero((char*) &create_info,sizeof(create_info)); + bzero((char*) &table_list, sizeof(table_list)); + + /* obtain a name lock */ + table_list[0].db= (char*) db; + table_list[0].table_name= (char*) name; + if ((error= lock_table_name(thd, table_list, TRUE))) + { + if (error < 0) + { + my_printf_error(ER_UNKNOWN_ERROR, + "Unable to acquire name lock on '%-.64s'", + MYF(0), name); + DBUG_RETURN(-1); + } + /* Someone else has this table open or locked */ + my_error(ER_TABLE_EXISTS_ERROR, MYF(0), name); + unlock_table_name(thd, table_list); + DBUG_RETURN(-1); + } + + /* + Indicate that lock is a create name lock, if we haven't already + acquired a name lock. + */ + if (table_list[0].table) + 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. + */ + pthread_mutex_unlock(&LOCK_open); + if ((error= ha_discover(thd, db, name, &frmblob, &frmlen))) { /* Table could not be discovered and thus not created */ - DBUG_RETURN(error); + goto end; } /* @@ -2634,17 +2669,19 @@ error= writefrm(path, frmblob, frmlen); my_free((char*) frmblob, MYF(0)); if (error) - DBUG_RETURN(2); + { + error= 2; + goto end; + } + error= 3; init_tmp_table_share(&share, db, 0, name, path); if (open_table_def(thd, &share, 0)) - { - DBUG_RETURN(3); - } + goto end; if (open_table_from_share(thd, &share, "" ,0, 0, 0, &table, FALSE)) { free_table_share(&share); - DBUG_RETURN(3); + goto end; } update_create_info_from_table(&create_info, &table); @@ -2658,8 +2695,13 @@ } error=table.file->create(path,&table,&create_info); VOID(closefrm(&table, 1)); + error= (error != 0); + +end: + pthread_mutex_lock(&LOCK_open); + unlock_table_name(thd, table_list); - DBUG_RETURN(error != 0); + DBUG_RETURN(error); } void st_ha_check_opt::init() --- 1.396/sql/sql_base.cc 2007-04-02 21:50:07 -07:00 +++ 1.397/sql/sql_base.cc 2007-04-02 21:50:07 -07:00 @@ -2186,6 +2186,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 @@ -2895,6 +2906,9 @@ 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 */ + pthread_mutex_unlock(&LOCK_open); while ((error= open_table_from_share(thd, share, alias, (uint) (HA_OPEN_KEYFILE | @@ -2905,6 +2919,8 @@ EXTRA_RECORD), thd->open_options, entry, FALSE))) { + pthread_mutex_lock(&LOCK_open); + if (error == 7) // Table def changed { share->version= 0; // Mark share as old @@ -2985,12 +3001,14 @@ if (error) goto err; + pthread_mutex_unlock(&LOCK_open); break; } if (Table_triggers_list::check_n_load(thd, share->db.str, share->table_name.str, entry, 0)) { + pthread_mutex_lock(&LOCK_open); closefrm(entry, 0); goto err; } @@ -3026,11 +3044,13 @@ "to write 'DELETE FROM `%s`.`%s`' to the binary log", table_list->db, table_list->table_name); delete entry->triggers; + pthread_mutex_lock(&LOCK_open); closefrm(entry, 0); goto err; } } } + pthread_mutex_lock(&LOCK_open); DBUG_RETURN(0); err: --- 1.212/sql/sql_delete.cc 2007-04-02 21:50:07 -07:00 +++ 1.213/sql/sql_delete.cc 2007-04-02 21:50:07 -07:00 @@ -940,10 +940,8 @@ // crashes, replacement works. *(path + path_length - reg_ext_length)= // '\0'; path[path_length - reg_ext_length] = 0; - VOID(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)); query_cache_invalidate3(thd, table_list, 0); end: --- 1.400/sql/sql_table.cc 2007-04-02 21:50:07 -07:00 +++ 1.401/sql/sql_table.cc 2007-04-02 21:50:07 -07:00 @@ -3210,6 +3210,7 @@ KEY *key_info_buffer; HA_CREATE_INFO *create_info; handler *file; + TABLE_LIST table_list[1]; bool error= TRUE; DBUG_ENTER("mysql_create_table_internal"); DBUG_PRINT("enter", ("db: '%s' table: '%s' tmp: %d", @@ -3506,10 +3507,49 @@ 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. + */ + 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)) + { + pthread_mutex_lock(&LOCK_open); + unlock_table_name(thd, table_list); + goto unlock_and_end; + } + goto err; + } if (create_info->options & HA_LEX_CREATE_TMP_TABLE) { @@ -3517,7 +3557,7 @@ if (!(open_temporary_table(thd, path, db, table_name, 1))) { (void) rm_temporary_table(create_info->db_type, path); - goto unlock_and_end; + goto err; } thd->tmp_table_used= 1; } @@ -3535,9 +3575,14 @@ !(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)) + { + pthread_mutex_lock(&LOCK_open); + unlock_table_name(thd, table_list); + goto unlock_and_end; + } + err: thd->proc_info="After create"; delete file; @@ -3549,7 +3594,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; } @@ -4613,7 +4661,7 @@ char *src_table= table_ident->table.str; int err; bool res= TRUE, unlock_dst_table= FALSE; - enum legacy_db_type not_used; + enum legacy_db_type table_type; HA_CREATE_INFO *create_info; #ifdef WITH_PARTITION_STORAGE_ENGINE char tmp_path[FN_REFLEN]; @@ -4664,7 +4712,7 @@ /* create like should be not allowed for Views, Triggers, ... */ - if (mysql_frm_type(thd, src_path, ¬_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; @@ -4717,6 +4765,19 @@ { 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; } @@ -4752,9 +4813,7 @@ 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); err= ha_create_table(thd, dst_path, db, table_name, create_info, 1); - 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)) @@ -4809,21 +4868,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-04-02 21:50:07 -07:00 +++ 1.167/sql/table.h 2007-04-02 21:50:07 -07:00 @@ -210,6 +210,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-04-02 21:50:07 -07:00 +++ 1.99/sql/unireg.cc 2007-04-02 21:50:07 -07:00 @@ -344,9 +344,9 @@ List &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.425/sql/ha_ndbcluster.cc 2007-04-02 21:50:07 -07:00 +++ 1.426/sql/ha_ndbcluster.cc 2007-04-02 21:50:07 -07:00 @@ -5063,6 +5063,7 @@ */ if (share && !do_event_op) share->flags|= NSF_NO_BINLOG; + safe_mutex_assert_not_owner(&LOCK_open); ndbcluster_log_schema_op(thd, share, thd->query, thd->query_length, share->db, share->table_name, @@ -5070,7 +5071,7 @@ m_table->getObjectVersion(), (is_truncate) ? SOT_TRUNCATE_TABLE : SOT_CREATE_TABLE, - 0, 0, 1); + 0, 0, 0); break; } } --- 1.44/mysql-test/r/federated.result 2007-04-02 21:50:07 -07:00 +++ 1.45/mysql-test/r/federated.result 2007-04-02 21:50:07 -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-04-02 21:50:07 -07:00 +++ 1.37/mysql-test/t/federated.test 2007-04-02 21:50:07 -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; --- 1.106/sql/ha_ndbcluster_binlog.cc 2007-04-02 21:50:07 -07:00 +++ 1.107/sql/ha_ndbcluster_binlog.cc 2007-04-02 21:50:07 -07:00 @@ -294,7 +294,7 @@ int error; DBUG_ENTER("ndbcluster_binlog_open_table"); - safe_mutex_assert_owner(&LOCK_open); + safe_mutex_assert_not_owner(&LOCK_open); init_tmp_table_share(table_share, share->db, 0, share->table_name, share->key); if ((error= open_table_def(thd, table_share, 0))) @@ -314,7 +314,9 @@ free_table_share(table_share); DBUG_RETURN(error); } + pthread_mutex_lock(&LOCK_open); assign_new_table_id(table_share); + pthread_mutex_unlock(&LOCK_open); if (!reopen) { @@ -1595,9 +1597,22 @@ packfrm(data, length, &pack_data, &pack_length) == 0 && cmp_frm(altered_table, pack_data, pack_length)) { + TABLE_LIST name_lock_list[1]; DBUG_DUMP("frm", (char*)altered_table->getFrmData(), altered_table->getFrmLength()); - pthread_mutex_lock(&LOCK_open); + bzero((byte*) name_lock_list, sizeof(name_lock_list)); + + /* Acquire a name lock on the table */ + name_lock_list[0].db= (char*) dbname; + name_lock_list[0].table_name= (char*) tabname; + + if (lock_and_wait_for_table_name(thd, name_lock_list) < 0) + { + name_lock_list[0].table= NULL; + sql_print_information("NDB: Unable to acquire name lock for %s.%s", + dbname, tabname); + } + Ndb_table_guard ndbtab_g(dict, tabname); const NDBTAB *old= ndbtab_g.get_table(); if (!old && @@ -1634,6 +1649,8 @@ dbname= table_share->db.str; tabname= table_share->table_name.str; + pthread_mutex_lock(&LOCK_open); + unlock_table_name(thd, name_lock_list); pthread_mutex_unlock(&LOCK_open); } my_free((char*)data, MYF(MY_ALLOW_ZERO_PTR));