From: Dmitry Lenev Date: December 11 2012 6:38pm Subject: bzr push into mysql-5.6 branch (Dmitry.Lenev:4754 to 4755) Bug#15954872 List-Archive: http://lists.mysql.com/commits/145485 X-Bug: 15954872 Message-Id: <20121211183803.9044.19346.4755@jubjub> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit 4755 Dmitry Lenev 2012-12-11 [merge] Bug #15954872 "MAKE MDL SUBSYSTEM AND TABLE DEFINITION CACHE ROBUST AGAINST BUGS IN CALLERS". Both MDL subsystems and Table Definition Cache code assume that callers ensure that names of objects passed to them are not longer than NAME_LEN bytes. Unfortunately due to bugs in callers this assumption might be broken in some cases. As result we get nasty bugs causing buffer overruns when we construct MDL key or TDC key from object names. This patch makes MDL and TDC code more robust against such bugs by ensuring that we always checking size of result buffer when constructing MDL and TDC keys. This doesn't free its callers from ensuring that both db and table names are shorter than NAME_LEN bytes. But at least these steps prevents buffer overruns in case of bug in caller, replacing them with less harmful behavior. This is 5.6+ version of patch. Changed code of MDL_key::mdl_key_init() to take into account size of buffer for the key. Did the same thing for create_table_def_key() function. Added asserts ensuring that callers won't pass arguments longer than NAME_LEN to them. Adjusted code constructing TDC keys directly to either use create_table_def_key() or get_table_def_key(). To support this change changed assert in get_table_def_key() in such way that it can be for table list elements which were used for opening of a view. modified: sql/mdl.h sql/sql_base.cc sql/sql_cache.cc sql/sql_cache.h sql/table.h unittest/gunit/mdl-t.cc 4754 Gleb Shchepa 2012-12-11 Bug #15892875: EXPLAIN UPDATE OUTPUT DOESN'T MATCH QUERY EXECUTION EXPLAIN single-table UPDATE/DELETE outputs the "ALL" value (full-table scan access method) in the "type" column even if the optimizer choose the table scan by an index access method: the "ALL" value should be replaced with "index" in this case. The Explain_table::explain_join_type() function has been modified to take into account the index scan access. modified: mysql-test/r/innodb_explain_json_non_select_all.result mysql-test/r/innodb_explain_json_non_select_none.result mysql-test/r/innodb_explain_non_select_all.result mysql-test/r/innodb_explain_non_select_none.result mysql-test/r/myisam_explain_json_non_select_all.result mysql-test/r/myisam_explain_json_non_select_none.result mysql-test/r/myisam_explain_non_select_all.result mysql-test/r/myisam_explain_non_select_none.result mysql-test/r/partition_explicit_prune.result mysql-test/r/partition_locking.result sql/opt_explain.cc === modified file 'sql/mdl.h' --- a/sql/mdl.h 2012-12-05 17:04:12 +0000 +++ b/sql/mdl.h 2012-12-11 18:07:34 +0000 @@ -335,8 +335,16 @@ public: const char *db, const char *name) { m_ptr[0]= (char) mdl_namespace; - m_db_name_length= (uint16) (strmov(m_ptr + 1, db) - m_ptr - 1); - m_length= (uint16) (strmov(m_ptr + m_db_name_length + 2, name) - m_ptr + 1); + /* + It is responsibility of caller to ensure that db and object names + are not longer than NAME_LEN. Still we play safe and try to avoid + buffer overruns. + */ + DBUG_ASSERT(strlen(db) <= NAME_LEN && strlen(name) <= NAME_LEN); + m_db_name_length= static_cast(strmake(m_ptr + 1, db, NAME_LEN) - + m_ptr - 1); + m_length= static_cast(strmake(m_ptr + m_db_name_length + 2, name, + NAME_LEN) - m_ptr + 1); } void mdl_key_init(const MDL_key *rhs) { === modified file 'sql/sql_base.cc' --- a/sql/sql_base.cc 2012-11-30 15:07:51 +0000 +++ b/sql/sql_base.cc 2012-12-11 18:07:34 +0000 @@ -272,8 +272,16 @@ static uint create_table_def_key(THD *th const char *db_name, const char *table_name, bool tmp_table) { - uint key_length= (uint) (strmov(strmov(key, db_name) + 1, table_name) - - key) + 1; + /* + In theory caller should ensure that both db and table_name are + not longer than NAME_LEN bytes. In practice we play safe to avoid + buffer overruns. + */ + DBUG_ASSERT(strlen(db_name) <= NAME_LEN && strlen(table_name) <= NAME_LEN); + uint key_length= static_cast(strmake(strmake(key, db_name, NAME_LEN) + + 1, table_name, NAME_LEN) - key + + 1); + if (tmp_table) { int4store(key + key_length, thd->server_id); @@ -311,8 +319,10 @@ uint get_table_def_key(const TABLE_LIST is properly initialized, so table definition cache can be produced from key used by MDL subsystem. */ - DBUG_ASSERT(!strcmp(table_list->db, table_list->mdl_request.key.db_name()) && - !strcmp(table_list->table_name, table_list->mdl_request.key.name())); + DBUG_ASSERT(!strcmp(table_list->get_db_name(), + table_list->mdl_request.key.db_name()) && + !strcmp(table_list->get_table_name(), + table_list->mdl_request.key.name())); *key= (const char*)table_list->mdl_request.key.ptr() + 1; return table_list->mdl_request.key.length() - 1; @@ -3129,7 +3139,8 @@ err_unlock: TABLE *find_locked_table(TABLE *list, const char *db, const char *table_name) { char key[MAX_DBKEY_LENGTH]; - uint key_length=(uint) (strmov(strmov(key,db)+1,table_name)-key)+1; + uint key_length= create_table_def_key((THD*)NULL, key, db, table_name, + false); for (TABLE *table= list; table ; table=table->next) { @@ -9167,7 +9178,7 @@ void tdc_remove_table(THD *thd, enum_tdc thd->mdl_context.is_lock_owner(MDL_key::TABLE, db, table_name, MDL_EXCLUSIVE)); - key_length=(uint) (strmov(strmov(key,db)+1,table_name)-key)+1; + key_length= create_table_def_key(thd, key, db, table_name, false); if ((share= (TABLE_SHARE*) my_hash_search(&table_def_cache,(uchar*) key, key_length))) === modified file 'sql/sql_cache.cc' --- a/sql/sql_cache.cc 2012-10-31 06:26:56 +0000 +++ b/sql/sql_cache.cc 2012-12-11 18:07:34 +0000 @@ -2824,11 +2824,9 @@ void Query_cache::invalidate_table(THD * invalidate_table(thd, table_list->table); // Table is open else { - char key[MAX_DBKEY_LENGTH]; + const char *key; uint key_length; - - key_length=(uint) (strmov(strmov(key,table_list->db)+1, - table_list->table_name) -key)+ 1; + key_length= get_table_def_key(table_list, &key); // We don't store temporary tables => no key_length+=4 ... invalidate_table(thd, (uchar *)key, key_length); @@ -2940,13 +2938,12 @@ Query_cache::register_tables_from_list(T block_table->n= n; if (tables_used->view) { - char key[MAX_DBKEY_LENGTH]; + const char *key; uint key_length; DBUG_PRINT("qcache", ("view: %s db: %s", tables_used->view_name.str, tables_used->view_db.str)); - key_length= (uint) (strmov(strmov(key, tables_used->view_db.str) + 1, - tables_used->view_name.str) - key) + 1; + key_length= get_table_def_key(tables_used, &key); /* There are not callback function for for VIEWs */ @@ -3056,7 +3053,7 @@ my_bool Query_cache::register_all_tables */ my_bool -Query_cache::insert_table(uint key_len, char *key, +Query_cache::insert_table(uint key_len, const char *key, Query_cache_block_table *node, uint32 db_length, uint8 cache_type, qc_engine_callback callback, @@ -4205,8 +4202,10 @@ uint Query_cache::filename_2_table_key ( *db_length= (filename - dbname) - 1; DBUG_PRINT("qcache", ("table '%-.*s.%s'", *db_length, dbname, filename)); - DBUG_RETURN((uint) (strmov(strmake(key, dbname, *db_length) + 1, - filename) -key) + 1); + DBUG_RETURN(static_cast(strmake(strmake(key, dbname, + min(*db_length, + NAME_LEN)) + 1, + filename, NAME_LEN) - key) + 1); } /**************************************************************************** === modified file 'sql/sql_cache.h' --- a/sql/sql_cache.h 2012-05-31 23:23:37 +0000 +++ b/sql/sql_cache.h 2012-12-11 18:07:34 +0000 @@ -370,7 +370,7 @@ protected: my_bool register_all_tables(Query_cache_block *block, TABLE_LIST *tables_used, TABLE_COUNTER_TYPE tables); - my_bool insert_table(uint key_len, char *key, + my_bool insert_table(uint key_len, const char *key, Query_cache_block_table *node, uint32 db_length, uint8 cache_type, qc_engine_callback callback, === modified file 'sql/table.h' --- a/sql/table.h 2012-11-29 14:50:08 +0000 +++ b/sql/table.h 2012-12-11 18:07:34 +0000 @@ -1966,7 +1966,7 @@ public: @brief Returns the name of the database that the referenced table belongs to. */ - char *get_db_name() { return view != NULL ? view_db.str : db; } + char *get_db_name() const { return view != NULL ? view_db.str : db; } /** @brief Returns the name of the table that this TABLE_LIST represents. @@ -1974,7 +1974,7 @@ public: @details The unqualified table name or view name for a table or view, respectively. */ - char *get_table_name() { return view != NULL ? view_name.str : table_name; } + char *get_table_name() const { return view != NULL ? view_name.str : table_name; } int fetch_number_of_rows(); bool update_derived_keys(Field*, Item**, uint); bool generate_keys(); === modified file 'unittest/gunit/mdl-t.cc' --- a/unittest/gunit/mdl-t.cc 2012-12-04 12:48:34 +0000 +++ b/unittest/gunit/mdl-t.cc 2012-12-11 18:07:34 +0000 @@ -1612,4 +1612,83 @@ TEST_F(MDLTest, HogLockTest5) max_write_lock_count= org_max_write_lock_count; } + +/** Test class for MDL_key class testing. Doesn't require MDL initialization. */ + +class MDLKeyTest : public ::testing::Test +{ +protected: + MDLKeyTest() + { } +private: + GTEST_DISALLOW_COPY_AND_ASSIGN_(MDLKeyTest); +}; + + +// Google Test recommends DeathTest suffix for classes use in death tests. +typedef MDLKeyTest MDLKeyDeathTest; + + +/* + Verifies that debug build dies with a DBUG_ASSERT if we try to construct + MDL_key with too long database or object names. +*/ + +#if GTEST_HAS_DEATH_TEST && !defined(DBUG_OFF) +TEST_F(MDLKeyDeathTest, DieWhenNamesAreTooLong) +{ + ::testing::FLAGS_gtest_death_test_style = "threadsafe"; + + /* We need a name which is longer than NAME_LEN = 64*3 = 192.*/ + const char *too_long_name= + "0123456789012345678901234567890123456789012345678901234567890123" + "0123456789012345678901234567890123456789012345678901234567890123" + "0123456789012345678901234567890123456789012345678901234567890123" + "0123456789"; + + EXPECT_DEATH(MDL_key key0(MDL_key::TABLE, too_long_name, ""), + ".*Assertion.*strlen.*failed.*"); + EXPECT_DEATH(MDL_key key1(MDL_key::TABLE, "", too_long_name), + ".*Assertion.*strlen.*failed.*"); + + MDL_key key2; + + EXPECT_DEATH(key2.mdl_key_init(MDL_key::TABLE, too_long_name, ""), + ".*Assertion.*strlen.*failed.*"); + EXPECT_DEATH(key2.mdl_key_init(MDL_key::TABLE, "", too_long_name), + ".*Assertion.*strlen.*failed.*"); + +} +#endif // GTEST_HAS_DEATH_TEST && !defined(DBUG_OFF) + + +/* + Verifies that for production build we allow construction of + MDL_key with too long database or object names, but they are + truncated. +*/ + +#if defined(DBUG_OFF) +TEST_F(MDLKeyTest, TruncateTooLongNames) +{ + /* We need a name which is longer than NAME_LEN = 64*3 = 192.*/ + const char *too_long_name= + "0123456789012345678901234567890123456789012345678901234567890123" + "0123456789012345678901234567890123456789012345678901234567890123" + "0123456789012345678901234567890123456789012345678901234567890123" + "0123456789"; + + MDL_key key(MDL_key::TABLE, too_long_name, too_long_name); + + const char *db_name= key.db_name(); + const char *name= key.name(); + + EXPECT_LE(strlen(db_name), (uint)NAME_LEN); + EXPECT_TRUE(strncmp(db_name, too_long_name, NAME_LEN) == 0); + EXPECT_LE(strlen(name), (uint)NAME_LEN); + EXPECT_TRUE(strncmp(name, too_long_name, NAME_LEN) == 0); +} +#endif // defined(DBUG_OFF) + + } // namespace No bundle (reason: useless for push emails).