List:Commits« Previous MessageNext Message »
From:Dmitry Lenev Date:December 11 2012 6:39pm
Subject:bzr push into mysql-5.5 branch (Dmitry.Lenev:4106 to 4107) Bug#15954872
View as plain text  
 4107 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.5-only version of patch.
      
      Changed code of MDL_key::mdl_key_init() to take into account
      size of buffer for the key.
      
      Introduced new version of create_table_def_key() helper function
      which constructs TDC key without risk of result buffer overrun.
      Places in code that construct TDC keys were changed to use this
      function.
      
      Also changed rm_temporary_table() and open_new_frm() functions
      to avoid use of "unsafe" strmov() and strxmov() functions and
      use safer strnxmov() instead.

    modified:
      sql/mdl.h
      sql/sql_base.cc
      sql/sql_base.h
      sql/sql_cache.cc
 4106 sayantan.dutta@stripped	2012-12-11 [merge]
      upmerge 14737171 5.1 => 5.5

    modified:
      mysql-test/mysql-test-run.pl
=== modified file 'sql/mdl.h'
--- a/sql/mdl.h	2012-12-05 16:41:29 +0000
+++ b/sql/mdl.h	2012-12-11 18:04:30 +0000
@@ -241,8 +241,14 @@ 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.
+    */
+    m_db_name_length= (uint16) (strmake(m_ptr + 1, db, NAME_LEN) - m_ptr - 1);
+    m_length= (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-09-22 12:37:04 +0000
+++ b/sql/sql_base.cc	2012-12-11 18:04:30 +0000
@@ -316,8 +316,9 @@ uint create_table_def_key(THD *thd, char
                           const TABLE_LIST *table_list,
                           bool tmp_table)
 {
-  uint key_length= (uint) (strmov(strmov(key, table_list->db)+1,
-                                  table_list->table_name)-key)+1;
+  uint key_length= create_table_def_key(key, table_list->db,
+                                        table_list->table_name);
+
   if (tmp_table)
   {
     int4store(key + key_length, thd->server_id);
@@ -815,13 +816,10 @@ void release_table_share(TABLE_SHARE *sh
 TABLE_SHARE *get_cached_table_share(const char *db, const char *table_name)
 {
   char key[NAME_LEN*2+2];
-  TABLE_LIST table_list;
   uint key_length;
   mysql_mutex_assert_owner(&LOCK_open);
 
-  table_list.db= (char*) db;
-  table_list.table_name= (char*) table_name;
-  key_length= create_table_def_key((THD*) 0, key, &table_list, 0);
+  key_length= create_table_def_key(key, db, table_name);
   return (TABLE_SHARE*) my_hash_search(&table_def_cache,
                                        (uchar*) key, key_length);
 }  
@@ -3168,7 +3166,7 @@ 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(key, db, table_name);
 
   for (TABLE *table= list; table ; table=table->next)
   {
@@ -5994,17 +5992,27 @@ TABLE *open_table_uncached(THD *thd, con
 }
 
 
-bool rm_temporary_table(handlerton *base, char *path)
+/**
+  Delete a temporary table.
+
+  @param base  Handlerton for table to be deleted.
+  @param path  Path to the table to be deleted (i.e. path
+               to its .frm without an extension).
+
+  @retval false - success.
+  @retval true  - failure.
+*/
+
+bool rm_temporary_table(handlerton *base, const char *path)
 {
   bool error=0;
   handler *file;
-  char *ext;
+  char frm_path[FN_REFLEN + 1];
   DBUG_ENTER("rm_temporary_table");
 
-  strmov(ext= strend(path), reg_ext);
-  if (mysql_file_delete(key_file_frm, path, MYF(0)))
+  strxnmov(frm_path, sizeof(frm_path) - 1, path, reg_ext, NullS);
+  if (mysql_file_delete(key_file_frm, frm_path, MYF(0)))
     error=1; /* purecov: inspected */
-  *ext= 0;				// remove extension
   file= get_new_handler((TABLE_SHARE*) 0, current_thd->mem_root, base);
   if (file && file->ha_delete_table(path))
   {
@@ -8943,7 +8951,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(key, db, table_name);
 
   if ((share= (TABLE_SHARE*) my_hash_search(&table_def_cache,(uchar*) key,
                                             key_length)))
@@ -9056,12 +9064,14 @@ open_new_frm(THD *thd, TABLE_SHARE *shar
 {
   LEX_STRING pathstr;
   File_parser *parser;
-  char path[FN_REFLEN];
+  char path[FN_REFLEN+1];
   DBUG_ENTER("open_new_frm");
 
   /* Create path with extension */
-  pathstr.length= (uint) (strxmov(path, share->normalized_path.str, reg_ext,
-                                  NullS)- path);
+  pathstr.length= (uint) (strxnmov(path, sizeof(path) - 1,
+                                   share->normalized_path.str,
+                                   reg_ext,
+                                   NullS) - path);
   pathstr.str=    path;
 
   if ((parser= sql_parse_prepare(&pathstr, mem_root, 1)))

=== modified file 'sql/sql_base.h'
--- a/sql/sql_base.h	2011-03-07 09:08:10 +0000
+++ b/sql/sql_base.h	2012-12-11 18:04:30 +0000
@@ -81,6 +81,31 @@ uint cached_table_definitions(void);
 uint create_table_def_key(THD *thd, char *key,
                           const TABLE_LIST *table_list,
                           bool tmp_table);
+
+/**
+  Create a table cache key for non-temporary table.
+
+  @param key         Buffer for key (must be at least NAME_LEN*2+2 bytes).
+  @param db          Database name.
+  @param table_name  Table name.
+
+  @return Length of key.
+
+  @sa create_table_def_key(thd, char *, table_list, bool)
+*/
+
+inline uint
+create_table_def_key(char *key, const char *db, const char *table_name)
+{
+  /*
+    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.
+  */
+  return (uint)(strmake(strmake(key, db, NAME_LEN) + 1, table_name,
+                        NAME_LEN) - key + 1);
+}
+
 TABLE_SHARE *get_table_share(THD *thd, TABLE_LIST *table_list, char *key,
                              uint key_length, uint db_flags, int *error,
                              my_hash_value_type hash_value);
@@ -157,7 +182,7 @@ thr_lock_type read_lock_type_for_table(T
                                        TABLE_LIST *table_list);
 
 my_bool mysql_rm_tmp_tables(void);
-bool rm_temporary_table(handlerton *base, char *path);
+bool rm_temporary_table(handlerton *base, const char *path);
 void close_tables_for_reopen(THD *thd, TABLE_LIST **tables,
                              const MDL_savepoint &start_of_statement_svp);
 TABLE_LIST *find_table_in_list(TABLE_LIST *table,

=== modified file 'sql/sql_cache.cc'
--- a/sql/sql_cache.cc	2012-02-16 09:48:16 +0000
+++ b/sql/sql_cache.cc	2012-12-11 18:04:30 +0000
@@ -2786,8 +2786,8 @@ void Query_cache::invalidate_table(THD *
     char key[MAX_DBKEY_LENGTH];
     uint key_length;
 
-    key_length=(uint) (strmov(strmov(key,table_list->db)+1,
-			      table_list->table_name) -key)+ 1;
+    key_length= create_table_def_key(key, table_list->db,
+                                     table_list->table_name);
 
     // We don't store temporary tables => no key_length+=4 ...
     invalidate_table(thd, (uchar *)key, key_length);
@@ -2904,8 +2904,8 @@ Query_cache::register_tables_from_list(T
       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= create_table_def_key(key, tables_used->view_db.str,
+                                       tables_used->view_name.str);
       /*
         There are not callback function for for VIEWs
       */
@@ -4137,8 +4137,9 @@ 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((uint) (strmake(strmake(key, dbname,
+                                      min(*db_length, NAME_LEN)) + 1,
+                              filename, NAME_LEN) - key) + 1);
 }
 
 /****************************************************************************

No bundle (reason: useless for push emails).
Thread
bzr push into mysql-5.5 branch (Dmitry.Lenev:4106 to 4107) Bug#15954872Dmitry Lenev12 Dec