maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

Hi Sanja,

Please review a patch for MDEV-18195.


Thanks.

commit 1afccf570dcf6a0f274aab375380efd26a706cdf
Author: Alexander Barkov <bar@xxxxxxxxxxx>
Date:   Thu Jan 17 16:37:57 2019 +0400

    MDEV-18195 ASAN use-after-poison in my_strcasecmp_utf8 / Item::eq upon prepared statement with ORDER BY NAME_CONST
    
    ASAN noticed a freed memory access during EXECUTE in this script:
      PREPARE stmt FROM "SELECT 'x' ORDER BY NAME_CONST( 'f', 'foo' )";
      EXECUTE stmt;
    
    In case of a PREPARE statement, all Items, including Item_name_const,
    are created on Prepared_statement::main_mem_root.
    Item_name_const::fix_fields() did not take this into account
    and could allocate the value of Item::name on a wrong memory root,
    in this code:
    
      if (is_autogenerated_name)
      {
        set_name(thd, item_name->c_ptr(), (uint) item_name->length(),
                 system_charset_info);
      }
    
    When fix_fields() is called in the reported SQL script, THD's arena already
    points to THD::main_mem_root rather than to Prepared_statement::main_mem_root,
    so Item::name was allocated on THD::main_mem_root.
    Then, at the end of the dispatch_command() for the PREPARE statement,
    THD::main_mem_root got cleared. So during EXECUTE, Item::name
    pointed to an already freed memory.
    
    This patch changes the code to set the implicit name for Item_name_const
    at the constructor time rather than at fix_fields time. This guarantees
    that Item_name_const and its Item::name always reside on the same memory root.
    
    Note, this change makes the code for Item_name_const symmetric with other
    constant-alike items that set their default implicit names at the constructor
    call time rather than at fix_fields() time:
    - Item_string
    - Item_int
    - Item_real
    - Item_decimal
    - Item_null
    - Item_param

diff --git a/mysql-test/r/func_misc.result b/mysql-test/r/func_misc.result
index 1f9aa21..287a70f 100644
--- a/mysql-test/r/func_misc.result
+++ b/mysql-test/r/func_misc.result
@@ -1471,3 +1471,11 @@ CALL p1();
 ip_full_addr
 2000::
 DROP PROCEDURE p1;
+#
+# MDEV-18195 ASAN use-after-poison in my_strcasecmp_utf8 / Item::eq upon prepared statement with ORDER BY NAME_CONST
+#
+PREPARE stmt FROM "SELECT 'x' ORDER BY NAME_CONST( 'f', 'foo' )";
+EXECUTE stmt;
+x
+x
+DEALLOCATE PREPARE stmt;
diff --git a/mysql-test/t/func_misc.test b/mysql-test/t/func_misc.test
index 2509f55..a8da906 100644
--- a/mysql-test/t/func_misc.test
+++ b/mysql-test/t/func_misc.test
@@ -1145,3 +1145,12 @@ $$
 DELIMITER ;$$
 CALL p1();
 DROP PROCEDURE p1;
+
+
+--echo #
+--echo # MDEV-18195 ASAN use-after-poison in my_strcasecmp_utf8 / Item::eq upon prepared statement with ORDER BY NAME_CONST
+--echo #
+
+PREPARE stmt FROM "SELECT 'x' ORDER BY NAME_CONST( 'f', 'foo' )";
+EXECUTE stmt;
+DEALLOCATE PREPARE stmt;
diff --git a/sql/item.cc b/sql/item.cc
index 1a2c4eb..d8f27bd 100644
--- a/sql/item.cc
+++ b/sql/item.cc
@@ -1672,10 +1672,14 @@ bool Item_name_const::is_null()
 Item_name_const::Item_name_const(THD *thd, Item *name_arg, Item *val):
   Item(thd), value_item(val), name_item(name_arg)
 {
+  StringBuffer<128> name_buffer;
+  String *name_str;
   Item::maybe_null= TRUE;
   valid_args= true;
-  if (!name_item->basic_const_item())
+  if (!name_item->basic_const_item() ||
+      !(name_str= name_item->val_str(&name_buffer))) // Can't have a NULL name
     goto err;
+  set_name(name_str->ptr(), name_str->length(), name_str->charset());
 
   if (value_item->basic_const_item())
     return; // ok
@@ -1737,24 +1741,14 @@ Item::Type Item_name_const::type() const
 
 bool Item_name_const::fix_fields(THD *thd, Item **ref)
 {
-  char buf[128];
-  String *item_name;
-  String s(buf, sizeof(buf), &my_charset_bin);
-  s.length(0);
-
   if (value_item->fix_fields(thd, &value_item) ||
       name_item->fix_fields(thd, &name_item) ||
       !value_item->const_item() ||
-      !name_item->const_item() ||
-      !(item_name= name_item->val_str(&s))) // Can't have a NULL name 
+      !name_item->const_item())
   {
     my_error(ER_RESERVED_SYNTAX, MYF(0), "NAME_CONST");
     return TRUE;
   }
-  if (is_autogenerated_name)
-  {
-    set_name(item_name->ptr(), (uint) item_name->length(), system_charset_info);
-  }
   if (value_item->collation.derivation == DERIVATION_NUMERIC)
     collation.set_numeric();
   else