List:Commits« Previous MessageNext Message »
From:Dmitry Lenev Date:December 11 2012 6:37pm
Subject:bzr push into mysql-trunk branch (Dmitry.Lenev:5215 to 5216) Bug#15954872
View as plain text  
 5216 Dmitry Lenev	2012-12-11 [merge]
      Merged fix for Bug #15954872 "MAKE MDL SUBSYSTEM AND TABLE
      DEFINITION CACHE ROBUST AGAINST BUGS IN CALLERS" into trunk.

    modified:
      sql/mdl.h
      sql/sql_base.cc
      sql/sql_cache.cc
      sql/sql_cache.h
      sql/table.h
      unittest/gunit/mdl-t.cc
 5215 Gleb Shchepa	2012-12-11 [merge]
      Auto up-merge (15892875)

    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:06:53 +0000
+++ b/sql/mdl.h	2012-12-11 18:12:13 +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-12-11 11:58:31 +0000
+++ b/sql/sql_base.cc	2012-12-11 18:12:13 +0000
@@ -271,8 +271,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);
@@ -310,8 +318,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;
@@ -3090,7 +3100,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)
   {
@@ -9337,7 +9348,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-11-21 12:44:48 +0000
+++ b/sql/sql_cache.cc	2012-12-11 18:12:13 +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-12-11 11:58:31 +0000
+++ b/sql/table.h	2012-12-11 18:12:13 +0000
@@ -1992,7 +1992,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.
@@ -2000,7 +2000,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:57:18 +0000
+++ b/unittest/gunit/mdl-t.cc	2012-12-11 18:12:13 +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-trunk branch (Dmitry.Lenev:5215 to 5216) Bug#15954872Dmitry Lenev12 Dec