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<uint16>(strmake(m_ptr + 1, db, NAME_LEN) -
+ m_ptr - 1);
+ m_length= static_cast<uint16>(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<uint>(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<uint>(strmake(strmake(key, dbname,
+ min<uint32>(*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).
| Thread |
|---|
| • bzr push into mysql-5.6 branch (Dmitry.Lenev:4754 to 4755) Bug#15954872 | Dmitry Lenev | 12 Dec |
