MDEV-36850 SIGSEGV in Item_sp_variable::save_in_field | fill_record

Thanks to Sergei Golubchik for the idea and a working prototype of this patch.

Problem:

Inside these methods:
- Item_splocal_assoc_array_element::append_for_log()
- Item_splocal_assoc_array_element_field::append_for_log()

an expression like this:

  first_names(nick || CONVERT(' ' USING ucs2)

was converted to:

  first_names(nick || CONVERT(CONVERT(' ' USING ucs2) USING latin1)

i.e. an automatic CONVERT(... USING latin1) was added, as expected.

In the end of append_for_log() the destructor of
Item_change_list_savepoint_raii restored the Item changes, so
the automatically added CONVERT(..USING latin1) was removed from
the tree and the tree changed back to:

   first_names(nick || CONVERT(' ' USING ucs2)

But all Item_splocal_assoc_array_element* Items were left in the fixed state.

Later, duing the INSERT, a concatenation of the SP variable `nick`
and the space character in UCS2 evaluated 'Michael\x00\x20' instead
of the expected 'Michael\x20', so the assoc array
element with the given key was not found.

Note:

Item_change_list_savepoint_raii was needed to make this DBUG_ASSERT in
sp_lex_keeper::reset_lex_and_exec_core() happy:
  DBUG_ASSERT(thd->Item_change_list::is_empty());

The fix:

- Removing Item_change_list_savepoint_raii from the implementations of
  Item_splocal_assoc_array_element*::append_for_log()
  Removing the class Item_change_list_savepoint_raii as it's not needed
  any more.

- Relaxing the DBUG_ASSERT() in sp_lex_keeper::reset_lex_and_exec_core() to:
  DBUG_ASSERT(dbug_rqp_are_fixed(instr) || thd->Item_change_list::is_empty());

  where dbug_rqp_are_fixed() is a new debug function to check that
  all Rewritable_query_parameter's in instr::free_list are fixed.

plugin/type_assoc_array/mysql-test/type_assoc_array/sp-assoc-array-ucs2.result

@@ -0,0 +1,58 @@
+SET sql_mode=ORACLE;
+#
+# MDEV-36850 SIGSEGV in Item_sp_variable::save_in_field | fill_record
+#
+CREATE TABLE t1 (a VARCHAR(64));
+SET character_set_database=sjis;
+SET collation_connection=ucs2_general_ci;
+DECLARE
+TYPE first_names_t IS TABLE OF VARCHAR2(64) INDEX BY VARCHAR2(20);
+first_names first_names_t;
+nick VARCHAR(64):= 'Monty';
+BEGIN
+first_names('Monty') := 'Michael';
+INSERT INTO t1 VALUES (first_names(nick));
+INSERT INTO t1 VALUES (first_names(TRIM(nick || ' ')));
+END;
+$$
+SELECT * FROM t1;
+a
+Michael
+Michael
+DROP TABLE t1;
+SET character_set_database=DEFAULT;
+SET collation_connection=DEFAULT;
+CREATE PROCEDURE p1 AS
+TYPE first_names_t IS TABLE OF VARCHAR2(64) INDEX BY VARCHAR2(20) CHARACTER SET latin1;
+first_names first_names_t;
+nick VARCHAR(64) CHARACTER SET latin1:= 'Monty';
+BEGIN
+CREATE OR REPLACE TABLE t1 (a VARCHAR(64));
+first_names('Monty') := 'Michael';
+INSERT INTO t1 VALUES (first_names(nick || CONVERT(' ' USING ucs2)));
+SELECT * FROM t1;
+DROP TABLE t1;
+END;
+$$
+CALL p1;
+a
+Michael
+DROP PROCEDURE p1;
+CREATE PROCEDURE p1 AS
+TYPE person_t IS RECORD (first_name VARCHAR(64), last_name VARCHAR(64));
+TYPE persons_t IS TABLE OF person_t INDEX BY VARCHAR2(20) CHARACTER SET latin1;
+persons persons_t;
+nick VARCHAR(64) CHARACTER SET latin1:= 'Monty';
+person person_t := ('Michael','Widenius');
+BEGIN
+CREATE OR REPLACE TABLE t1 (a VARCHAR(64));
+persons(nick) := person;
+INSERT INTO t1 VALUES (persons(nick || CONVERT(' ' USING ucs2)).first_name);
+SELECT * FROM t1;
+DROP TABLE t1;
+END;
+$$
+CALL p1;
+a
+Michael
+DROP PROCEDURE p1;

plugin/type_assoc_array/mysql-test/type_assoc_array/sp-assoc-array-ucs2.test

@@ -0,0 +1,66 @@
+--source include/have_ucs2.inc
+--source include/have_sjis.inc
+
+SET sql_mode=ORACLE;
+
+--echo #
+--echo # MDEV-36850 SIGSEGV in Item_sp_variable::save_in_field | fill_record
+--echo #
+
+CREATE TABLE t1 (a VARCHAR(64));
+SET character_set_database=sjis;
+SET collation_connection=ucs2_general_ci;
+delimiter $$;
+DECLARE
+  TYPE first_names_t IS TABLE OF VARCHAR2(64) INDEX BY VARCHAR2(20);
+  first_names first_names_t;
+  nick VARCHAR(64):= 'Monty';
+BEGIN
+  first_names('Monty') := 'Michael';
+  INSERT INTO t1 VALUES (first_names(nick));
+  INSERT INTO t1 VALUES (first_names(TRIM(nick || ' ')));
+END;
+$$
+delimiter ;$$
+SELECT * FROM t1;
+DROP TABLE t1;
+SET character_set_database=DEFAULT;
+SET collation_connection=DEFAULT;
+
+
+delimiter $$;
+CREATE PROCEDURE p1 AS
+  TYPE first_names_t IS TABLE OF VARCHAR2(64) INDEX BY VARCHAR2(20) CHARACTER SET latin1;
+  first_names first_names_t;
+  nick VARCHAR(64) CHARACTER SET latin1:= 'Monty';
+BEGIN
+  CREATE OR REPLACE TABLE t1 (a VARCHAR(64));
+  first_names('Monty') := 'Michael';
+  INSERT INTO t1 VALUES (first_names(nick || CONVERT(' ' USING ucs2)));
+  SELECT * FROM t1;
+  DROP TABLE t1;
+END;
+$$
+delimiter ;$$
+CALL p1;
+DROP PROCEDURE p1;
+
+
+delimiter $$;
+CREATE PROCEDURE p1 AS
+  TYPE person_t IS RECORD (first_name VARCHAR(64), last_name VARCHAR(64));
+  TYPE persons_t IS TABLE OF person_t INDEX BY VARCHAR2(20) CHARACTER SET latin1;
+  persons persons_t;
+  nick VARCHAR(64) CHARACTER SET latin1:= 'Monty';
+  person person_t := ('Michael','Widenius');
+BEGIN
+  CREATE OR REPLACE TABLE t1 (a VARCHAR(64));
+  persons(nick) := person;
+  INSERT INTO t1 VALUES (persons(nick || CONVERT(' ' USING ucs2)).first_name);
+  SELECT * FROM t1;
+  DROP TABLE t1;
+END;
+$$
+delimiter ;$$
+CALL p1;
+DROP PROCEDURE p1;

plugin/type_assoc_array/sql_type_assoc_array.cc

@@ -41,32 +41,6 @@ public:
   using StringBuffer::StringBuffer;
 };
 
-/*
-  Support class to rollback the change list when append_to_log is called.
-  The following query for example:
-    INSERT INTO t1 VALUES (first_names(TRIM(nick || ' ')));
-
-  will create a new item during fix_fields.
-
-  The raii in the class suffix denotes that this class will
-  automatically rollback the change list upon destruction.
-*/
-class Item_change_list_savepoint_raii : public Item_change_list_savepoint
-{
-private:
-  THD *m_thd;
-public:
-  Item_change_list_savepoint_raii(THD *thd)
-    : Item_change_list_savepoint(thd)
-    , m_thd(thd)
-  { }
-  ~Item_change_list_savepoint_raii()
-  {
-    rollback(m_thd);
-  }
-};
-
-
 class Item_field_packable
 {
 protected:
@@ -536,7 +510,6 @@ public:
 
   bool append_for_log(THD *thd, String *str) override
   {
-    Item_change_list_savepoint_raii savepoint(thd);
     if (T::fix_fields_if_needed(thd, NULL))
       return true;
 
@@ -2196,8 +2169,6 @@ void Item_splocal_assoc_array_element_field::print(String *str,
 
 bool Item_splocal_assoc_array_element::append_for_log(THD *thd, String *str)
 {
-  Item_change_list_savepoint_raii savepoint(thd);
-
   if (fix_fields_if_needed(thd, NULL))
     return true;
 
@@ -2218,8 +2189,6 @@ bool Item_splocal_assoc_array_element::append_for_log(THD *thd, String *str)
 bool Item_splocal_assoc_array_element_field::append_for_log(THD *thd,
                                                             String *str)
 {
-  Item_change_list_savepoint_raii savepoint(thd);
-
   if (fix_fields_if_needed(thd, NULL))
     return true;
 

sql/sp_instr.cc

@@ -240,6 +240,25 @@ subst_spvars(THD *thd, sp_instr *instr, LEX_STRING *query_str)
   DBUG_RETURN(false);
 }
 
+
+#ifndef DBUG_OFF
+/*
+  Check if all rewrittable query params in an instruction are fixed.
+  They can be fixed e.g. if append_for_log() already happened.
+*/
+bool dbug_rqp_are_fixed(sp_instr *instr)
+{
+  for (Item *item= instr->free_list; item; item= item->next)
+  {
+    Rewritable_query_parameter *rqp= item->get_rewritable_query_parameter();
+    if (rqp && rqp->pos_in_query && !item->fixed())
+      return false;
+  }
+  return true;
+}
+#endif
+
+
 /**
   Prepare LEX and thread for execution of instruction, if requested open
   and lock LEX's tables, execute instruction's core function, perform
@@ -286,7 +305,14 @@ sp_lex_keeper::reset_lex_and_exec_core(THD *thd, uint *nextp,
   thd->transaction->stmt.m_unsafe_rollback_flags= 0;
 
   DBUG_ASSERT(!thd->derived_tables);
-  DBUG_ASSERT(thd->Item_change_list::is_empty());
+  /*
+    Item*::append_for_log() called from subst_spvars (which already happened
+    at this point) can create new Items in some cases. For example:
+      INSERT INTO t1 VALUES
+       (assoc_array(spvar_latin1 || CONVERT(' ' USING ucs2)));
+    wraps CONVERT into Item_func_conv_charset.
+  */
+  DBUG_ASSERT(dbug_rqp_are_fixed(instr) || thd->Item_change_list::is_empty());
   /*
     Use our own lex.
     We should not save old value since it is saved/restored in