MDEV-32694: ASAN errors in Binary_string::alloced_length / reset_stmt_params

Anonymous block is represented internally by the class sp_head,
so every statement inside an anonymous block is a SP instruction.
On the other hand, the anonymous block specified in the FROM clause of
the PREPARE statement is treated as a single statement. In result,
all parameter markers (represented by the character ?) are parts of
the anonymous block specified in the prepared statement and at the same
time parameter are markers, internally represented by instances of
the class Item_param and distributed among SP instructions representing
SQL statements (every SQL statement is represented by an instance of
the class sp_instr_stmt)

In case table metadata changed on running an anonymous block in prepared
statement mode, only SP instruction's statement is re-parsed. Before
re-parsing a SP's statement, all items are cleaned up including
instances of the class Item_param that represent positional parameters.

Unfortunately, this leads to presence of a dangling pointer in
Prepared_statement::param_array that references to the deleted
Item_param while invoking reset_stmt_params happening on every execution
of a prepared statement.

To fix the issue, no instances of Item_param created on re-parsings
a statement for failed SP instruction, rather instances of Item_param
left from first time parsing are re-used. As a consequence, all pointers
to instances of the class Item_param stored in the array
Prepared_statememt::param_array and possibly spread along the code base
  (e.g. select_lex->limit_params.select_limit)
still point to valid Items.

Author: dmitryshulga

mysql-test/main/ps.result

@@ -6014,4 +6014,66 @@ INSERT INTO t1 VALUES (1,11);
 CREATE TRIGGER tr BEFORE INSERT ON t1 FOR EACH ROW SET @x = NULL;
 EXECUTE IMMEDIATE "UPDATE t1 SET a = ?" USING DEFAULT;
 DROP TABLE t1;
+#
+# MDEV-32694: ASAN errors in Binary_string::alloced_length / reset_stmt_params
+#
+CREATE TABLE t1 (a INT);
+INSERT INTO t1 VALUES (1), (2), (3), (4), (5), (6), (7);
+PREPARE stmt FROM 'BEGIN NOT ATOMIC SELECT * FROM t1 LIMIT ?; SELECT * FROM t1 LIMIT ?; END';
+# Expected output is row (1) produced by the first query
+# the rows (1), (2), (3) produced by the second one
+EXECUTE stmt USING 1, 3;
+a
+1
+a
+1
+2
+3
+ALTER TABLE t1 ADD COLUMN f INT;
+# Because metadata of the table t1 has been changed,
+# the second execution of the same prepared statement should result in
+# re-compilation of the first statement enclosed in the BEGIN / END block
+# and since different actual values are provided to positional parameters
+# on the second execution, the exepected output is the row (1), (2), (3)
+# produced by the first query and the rows (1), (2), (3), (4), (5)
+# produced by the second one
+EXECUTE stmt USING 3, 5;
+a	f
+1	NULL
+2	NULL
+3	NULL
+a	f
+1	NULL
+2	NULL
+3	NULL
+4	NULL
+5	NULL
+DEALLOCATE PREPARE stmt;
+DROP TABLE t1;
+# Check that the order of parameters preserved after re-compilation of a
+# failed statement inside anonymous BEGIN / END block.
+CREATE TABLE t1 (a INT);
+INSERT INTO t1 VALUES (1);
+PREPARE stmt FROM 'BEGIN NOT ATOMIC SELECT a, ?, ? FROM t1; END';
+# Expected output is the row (1, 10, 20)
+EXECUTE stmt USING 10, 20;
+a	?	?
+1	10	20
+ALTER TABLE t1 ADD COLUMN b INT;
+# Expected output is the row (1, 300, 400)
+EXECUTE stmt USING 300, 400;
+a	?	?
+1	300	400
+ALTER TABLE t1 DROP COLUMN b;
+# Expected output is the row (1, 500, 700)
+EXECUTE stmt USING 500, 700;
+a	?	?
+1	500	700
+ALTER TABLE t1 ADD COLUMN b INT;
+# Expected output is the row (1, 700, 900)
+EXECUTE stmt USING 700, 900;
+a	?	?
+1	700	900
+DEALLOCATE PREPARE stmt;
+DROP TABLE t1;
 # End of 11.4 tests

mysql-test/main/ps.test

@@ -5479,4 +5479,51 @@ EXECUTE IMMEDIATE "UPDATE t1 SET a = ?" USING DEFAULT;
 # Cleanup
 DROP TABLE t1;
 
+--echo #
+--echo # MDEV-32694: ASAN errors in Binary_string::alloced_length / reset_stmt_params
+--echo #
+
+CREATE TABLE t1 (a INT);
+INSERT INTO t1 VALUES (1), (2), (3), (4), (5), (6), (7);
+
+PREPARE stmt FROM 'BEGIN NOT ATOMIC SELECT * FROM t1 LIMIT ?; SELECT * FROM t1 LIMIT ?; END';
+--echo # Expected output is row (1) produced by the first query
+--echo # the rows (1), (2), (3) produced by the second one
+EXECUTE stmt USING 1, 3;
+ALTER TABLE t1 ADD COLUMN f INT;
+
+--echo # Because metadata of the table t1 has been changed,
+--echo # the second execution of the same prepared statement should result in
+--echo # re-compilation of the first statement enclosed in the BEGIN / END block
+--echo # and since different actual values are provided to positional parameters
+--echo # on the second execution, the exepected output is the row (1), (2), (3)
+--echo # produced by the first query and the rows (1), (2), (3), (4), (5)
+--echo # produced by the second one
+EXECUTE stmt USING 3, 5;
+
+# Cleanup
+DEALLOCATE PREPARE stmt;
+DROP TABLE t1;
+
+--echo # Check that the order of parameters preserved after re-compilation of a
+--echo # failed statement inside anonymous BEGIN / END block.
+CREATE TABLE t1 (a INT);
+INSERT INTO t1 VALUES (1);
+PREPARE stmt FROM 'BEGIN NOT ATOMIC SELECT a, ?, ? FROM t1; END';
+--echo # Expected output is the row (1, 10, 20)
+EXECUTE stmt USING 10, 20;
+ALTER TABLE t1 ADD COLUMN b INT;
+--echo # Expected output is the row (1, 300, 400)
+EXECUTE stmt USING 300, 400;
+ALTER TABLE t1 DROP COLUMN b;
+--echo # Expected output is the row (1, 500, 700)
+EXECUTE stmt USING 500, 700;
+ALTER TABLE t1 ADD COLUMN b INT;
+--echo # Expected output is the row (1, 700, 900)
+EXECUTE stmt USING 700, 900;
+
+# Cleanup
+DEALLOCATE PREPARE stmt;
+DROP TABLE t1;
+
 --echo # End of 11.4 tests

sql/sp_instr.cc

@@ -24,6 +24,42 @@ static int cmp_rqp_locations(const void *a_, const void *b_)
 }
 
 
+/**
+  Traverse the list of Item_param instances created on the fist parsing of
+  SP instruction's statement and put them back into sp_inst_lex->free list
+  for releasing them on deallocating statement's resources to avoid
+  memory leaks.
+*/
+
+void
+sp_lex_instr::put_back_item_params(THD *thd, LEX *lex,
+                                   const List<Item_param>& param_values)
+{
+  /*
+    Instance of Item_param must be ignored on re-parsing a statement
+    of failed SP instruction, therefore lex->param_list must be empty.
+    Instance of the class Item_param created on first (initial) parsing of
+    Prepared Statement is used for whole its life.
+  */
+  DBUG_ASSERT(lex->param_list.is_empty());
+
+  for (auto it= param_values.begin();
+       it != param_values.end(); ++it)
+  {
+    /*
+      Put retained instances of Item_param back into sp_lex_inst::free_list
+      to avoid leaking them. Original ordering of Item_param objects
+      are preserved since param_values contains items in reverse order.
+    */
+    Item_param *param_for_adding_to_free_list= it.operator->();
+
+    Item *prev_head= free_list;
+    free_list= param_for_adding_to_free_list;
+    param_for_adding_to_free_list->next= prev_head;
+  }
+}
+
+
 /*
   StoredRoutinesBinlogging
   This paragraph applies only to statement-based binlogging. Row-based
@@ -592,14 +628,30 @@ void sp_lex_instr::get_query(String *sql_query) const
 }
 
 
-void sp_lex_instr::cleanup_before_parsing(enum_sp_type sp_type)
+List<Item_param>
+sp_lex_instr::cleanup_before_parsing(enum_sp_type sp_type)
 {
   Item *current= free_list;
+  List<Item_param> param_values{};
 
   while (current)
   {
     Item *next= current->next;
-    current->delete_self();
+
+    if (current->is_stored_routine_parameter())
+      /*
+        `current` points to an instance of the class Item_param.
+        Place an instance of the class Item_param into the list `param_values`
+        and skip the item in free_list (don't invoke the method delete_self()
+        on it). Since the `free_list` stores items in reverse order of creation
+        (that is the last created item is the one pointed by the `free_list`),
+        place items in the list `param_values` using push_front to save
+        original ordering of items
+      */
+      param_values.push_front((Item_param*)current);
+    else
+      current->delete_self();
+
     current= next;
   }
 
@@ -612,6 +664,8 @@ void sp_lex_instr::cleanup_before_parsing(enum_sp_type sp_type)
       dangling references.
     */
     m_cur_trigger_stmt_items.empty();
+
+  return param_values;
 }
 
 
@@ -770,10 +824,13 @@ LEX* sp_lex_instr::parse_expr(THD *thd, sp_head *sp, LEX *sp_instr_lex)
       m_cur_trigger_stmt_items.first->next_trig_field_list;
 
   /*
-    Clean up items owned by this SP instruction.
+    Clean up items owned by this SP instruction except instances of Item_param.
+    `sp_statement_param_values` stores instances of the class Item_param
+    associated with the SP instruction's statement before the statement
+    has been re-parsed.
   */
-  cleanup_before_parsing(sp->m_handler->type());
-
+  List<Item_param> sp_statement_param_values=
+    cleanup_before_parsing(sp->m_handler->type());
   DBUG_ASSERT(mem_root != thd->mem_root);
   /*
     Back up the current free_list pointer and reset it to nullptr.
@@ -812,18 +869,29 @@ LEX* sp_lex_instr::parse_expr(THD *thd, sp_head *sp, LEX *sp_instr_lex)
   if (parser_state.init(thd, sql_query.c_ptr(), sql_query.length()))
     return nullptr;
 
+  /*
+    Direct the parser to handle the '?' symbol in special way, that is as
+    a positional parameter inside a prepared statement.
+  */
+  parser_state.m_lip.stmt_prepare_mode= true;
+
   // Create a new LEX and initialize it.
 
   LEX *lex_saved= thd->lex;
   Item **cursor_free_list= nullptr;
+  st_lex_local *lex_local= nullptr;
 
   /*
     sp_instr_lex != nullptr for cursor relating SP instructions (sp_instr_cpush,
     sp_instr_cursor_copy_struct) and in some cases for sp_instr_set.
   */
   if (sp_instr_lex == nullptr)
   {
-    thd->lex= new (thd->mem_root) st_lex_local;
+    lex_local= new (thd->mem_root) st_lex_local;
+    thd->lex= lex_local;
+
+    lex_local->sp_statement_param_values= std::move(sp_statement_param_values);
+    lex_local->param_values_it= lex_local->sp_statement_param_values.begin();
     lex_start(thd);
     if (sp->m_handler->type() == SP_TYPE_TRIGGER)
     {
@@ -892,7 +960,17 @@ LEX* sp_lex_instr::parse_expr(THD *thd, sp_head *sp, LEX *sp_instr_lex)
   const char *m_tmp_query_bak= sp->m_tmp_query;
   sp->m_tmp_query= sql_query.c_ptr();
 
+  /*
+    Hint the parser that re-parsing of a failed SP instruction is in progress
+    and instances of the class Item_param associated with SP instruction
+    should be handled carefully (re-used on re-parsing the instruction's
+    statement).
+    @sa param_push_or_clone
+    @sa LEX::add_placeholder
+  */
+  thd->reparsing_sp_stmt= true;
   bool parsing_failed= parse_sql(thd, &parser_state, nullptr);
+  thd->reparsing_sp_stmt= false;
 
   sp->m_tmp_query= m_tmp_query_bak;
   thd->m_digest= parent_digest;
@@ -915,12 +993,17 @@ LEX* sp_lex_instr::parse_expr(THD *thd, sp_head *sp, LEX *sp_instr_lex)
       */
       *cursor_free_list= thd->free_list;
     else
+    {
       /*
         Assign the list of items created on re-parsing the statement to
         the current stored routine's instruction.
       */
       free_list= thd->free_list;
 
+      put_back_item_params(thd, thd->lex,
+                           lex_local->sp_statement_param_values);
+    }
+
     thd->free_list= nullptr;
   }
 

sql/sp_instr.h

@@ -500,8 +500,12 @@ class sp_lex_instr : public sp_instr
 
   /**
     Clean up items previously created on behalf of the current instruction.
+
+    @return a list of Item_param instances representing position parameters
+            specified for the instruction that is a part of a prepared
+            statement
   */
-  void cleanup_before_parsing(enum_sp_type sp_type);
+  List<Item_param> cleanup_before_parsing(enum_sp_type sp_type);
 
 
   /**
@@ -524,6 +528,10 @@ class sp_lex_instr : public sp_instr
 
   bool setup_memroot_for_reparsing(sp_head *sphead,
                                    bool *new_memroot_allocated);
+
+  void put_back_item_params(THD *thd, LEX *lex,
+                            const List<Item_param>& param_values);
+
 };
 
 

sql/sql_class.h

@@ -5972,6 +5972,8 @@ class THD: public THD_count, /* this must be first */
     return (lex->sphead != 0 &&
             !(in_sub_stmt & (SUB_STMT_FUNCTION | SUB_STMT_TRIGGER)));
   }
+
+  bool reparsing_sp_stmt= {false};
 };
 
 

sql/sql_lex.cc

@@ -8176,9 +8176,19 @@ Item *LEX::make_item_sysvar(THD *thd,
 
 static bool param_push_or_clone(THD *thd, LEX *lex, Item_param *item)
 {
-  return !lex->clone_spec_offset ?
-         lex->param_list.push_back(item, thd->mem_root) :
-         item->add_as_clone(thd);
+  if (lex->clone_spec_offset)
+    return item->add_as_clone(thd);
+  else
+  {
+    if (thd->reparsing_sp_stmt)
+      /*
+        Don't put an instance of Item_param in case a SP statement
+        being re-parsed.
+      */
+      return false;
+
+    return lex->param_list.push_back(item, thd->mem_root);
+  }
 }
 
 
@@ -8196,8 +8206,40 @@ Item_param *LEX::add_placeholder(THD *thd, const LEX_CSTRING *name,
     return NULL;
   }
   Query_fragment pos(thd, sphead, start, end);
-  Item_param *item= new (thd->mem_root) Item_param(thd, name,
-                                                   pos.pos(), pos.length());
+  Item_param *item;
+  /*
+    Check whether re-parsing of a failed SP instruction is in progress.
+    In context of the method LEX::add_placeholder, the failed instruction
+    being re-parsed is a part of compound statement enclosed into
+    the BEGIN/END clauses.
+  */
+  if (thd->reparsing_sp_stmt)
+  {
+    /*
+      Get a saved Item_param and reuse it instead of creating a new one.
+      st_lex_local stores instances of the class Item_param that were saved
+      before cleaning up SP instruction's free_list. So, the same instance of
+      Item_param will be used on every re-parsing of failed SP instruction
+      for each specific positional parameter.
+    */
+    st_lex_local *lex= (st_lex_local*)this;
+    DBUG_ASSERT(lex->param_values_it != lex->sp_statement_param_values.end());
+    /*
+      For release build emit internal error in case the assert condition
+      fails
+    */
+    if (lex->param_values_it == lex->sp_statement_param_values.end())
+    {
+      my_error(ER_INTERNAL_ERROR, MYF(0), "no more Item_param for re-bind");
+      return nullptr;
+    }
+
+    item= lex->param_values_it.operator ->();
+    lex->param_values_it++;
+  }
+  else
+    item= new (thd->mem_root) Item_param(thd, name,
+                                         pos.pos(), pos.length());
   if (unlikely(!item) || unlikely(param_push_or_clone(thd, this, item)))
   {
     my_error(ER_OUT_OF_RESOURCES, MYF(0));