List:Commits« Previous MessageNext Message »
From:Dmitry Lenev Date:December 11 2012 6:38pm
Subject:bzr push into mysql-5.6 branch (Dmitry.Lenev:4754 to 4755) Bug#15954872
View as plain text  
 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#15954872Dmitry Lenev12 Dec