From a0290cfbed7d10c3d5e27e942f60f925ed413f2b Mon Sep 17 00:00:00 2001 From: Alexander Barkov Date: Thu, 31 Jul 2025 09:12:56 +0400 Subject: [PATCH] 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. --- .../sp-assoc-array-ucs2.result | 58 ++++++++++++++++ .../type_assoc_array/sp-assoc-array-ucs2.test | 66 +++++++++++++++++++ .../type_assoc_array/sql_type_assoc_array.cc | 31 --------- sql/sp_instr.cc | 28 +++++++- 4 files changed, 151 insertions(+), 32 deletions(-) create mode 100644 plugin/type_assoc_array/mysql-test/type_assoc_array/sp-assoc-array-ucs2.result create mode 100644 plugin/type_assoc_array/mysql-test/type_assoc_array/sp-assoc-array-ucs2.test diff --git a/plugin/type_assoc_array/mysql-test/type_assoc_array/sp-assoc-array-ucs2.result b/plugin/type_assoc_array/mysql-test/type_assoc_array/sp-assoc-array-ucs2.result new file mode 100644 index 00000000000..1d6f3ac8de9 --- /dev/null +++ b/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; diff --git a/plugin/type_assoc_array/mysql-test/type_assoc_array/sp-assoc-array-ucs2.test b/plugin/type_assoc_array/mysql-test/type_assoc_array/sp-assoc-array-ucs2.test new file mode 100644 index 00000000000..1ab17a80d2f --- /dev/null +++ b/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; diff --git a/plugin/type_assoc_array/sql_type_assoc_array.cc b/plugin/type_assoc_array/sql_type_assoc_array.cc index 7c971a2d906..8884fd25588 100644 --- a/plugin/type_assoc_array/sql_type_assoc_array.cc +++ b/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; diff --git a/sql/sp_instr.cc b/sql/sp_instr.cc index dd4e23bb5ce..29b57091e49 100644 --- a/sql/sp_instr.cc +++ b/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