Hi, Alexander! On Dec 26, Alexander Barkov wrote:

Hi Sergei,

can you please review a patch for MDEV-14603?

I agree with the fix. But I don't like that there are many things to backup/restore (Statement, arena, free_list, and now change_list), they're all saved/restored in different places - it's easy to miss something when making changes. Would it be possible to move all that saving/restoring into dedicated helpers and use them in all three places (prepare, execute, execute immediate)?

diff --git a/mysql-test/r/ps.result b/mysql-test/r/ps.result index 6857ebc..a56920d 100644 --- a/mysql-test/r/ps.result +++ b/mysql-test/r/ps.result @@ -5074,3 +5074,45 @@ t1 CREATE TABLE `t1` ( ) ENGINE=MyISAM DEFAULT CHARSET=latin1 DROP TABLE t1; DROP PROCEDURE p1; +# +# MDEV-14603 signal 11 with short stacktrace +# +SET NAMES utf8; +CREATE TABLE t1(i INT); +CREATE PROCEDURE p1(tn VARCHAR(32)) +EXECUTE IMMEDIATE CONCAT('ANALYZE TABLE ',tn); +CALL p1('t1'); +Table Op Msg_type Msg_text +test.t1 analyze status Table is already up to date +DROP PROCEDURE p1; +DROP TABLE t1; +SET NAMES utf8; +CREATE PROCEDURE p1() +EXECUTE IMMEDIATE CONCAT('SELECT ',CONVERT(RAND() USING latin1)); +CALL p1(); +DROP PROCEDURE p1; +SET NAMES utf8; +CREATE PROCEDURE p1() +BEGIN +PREPARE stmt FROM CONCAT('SELECT ',CONVERT(RAND() USING latin1)); +EXECUTE stmt; +DEALLOCATE PREPARE stmt; +END; +$$ +CALL p1(); +DROP PROCEDURE p1; +SET NAMES utf8; +CREATE PROCEDURE p1(a VARCHAR(10) CHARACTER SET utf8) +EXECUTE IMMEDIATE 'SELECT ?' USING CONCAT(a, CONVERT(RAND() USING latin1)); +CALL p1('x'); +DROP PROCEDURE p1; +SET NAMES utf8; +CREATE PROCEDURE p1(a VARCHAR(10) CHARACTER SET utf8) +BEGIN +PREPARE stmt FROM 'SELECT ?'; +EXECUTE stmt USING CONCAT(a, CONVERT(RAND() USING latin1)); +DEALLOCATE PREPARE stmt; +END; +$$ +CALL p1('x'); +DROP PROCEDURE p1; diff --git a/mysql-test/t/ps.test b/mysql-test/t/ps.test index a7683b5..640f479 100644 --- a/mysql-test/t/ps.test +++ b/mysql-test/t/ps.test @@ -4529,3 +4529,62 @@ CREATE TABLE t1 AS SELECT @a AS a, @b AS b; SHOW CREATE TABLE t1; DROP TABLE t1; DROP PROCEDURE p1; + + +--echo # +--echo # MDEV-14603 signal 11 with short stacktrace +--echo # + +SET NAMES utf8; +CREATE TABLE t1(i INT); +CREATE PROCEDURE p1(tn VARCHAR(32)) + EXECUTE IMMEDIATE CONCAT('ANALYZE TABLE ',tn); +CALL p1('t1'); +DROP PROCEDURE p1; +DROP TABLE t1; + +SET NAMES utf8; +CREATE PROCEDURE p1() + EXECUTE IMMEDIATE CONCAT('SELECT ',CONVERT(RAND() USING latin1)); +--disable_result_log +CALL p1(); +--enable_result_log +DROP PROCEDURE p1; + +SET NAMES utf8; +DELIMITER $$; +CREATE PROCEDURE p1() +BEGIN + PREPARE stmt FROM CONCAT('SELECT ',CONVERT(RAND() USING latin1)); + EXECUTE stmt; + DEALLOCATE PREPARE stmt; +END; +$$ +DELIMITER ;$$ +--disable_result_log +CALL p1(); +--enable_result_log +DROP PROCEDURE p1; + +SET NAMES utf8; +CREATE PROCEDURE p1(a VARCHAR(10) CHARACTER SET utf8) + EXECUTE IMMEDIATE 'SELECT ?' USING CONCAT(a, CONVERT(RAND() USING latin1)); +--disable_result_log +CALL p1('x'); +--enable_result_log +DROP PROCEDURE p1; + +SET NAMES utf8; +DELIMITER $$; +CREATE PROCEDURE p1(a VARCHAR(10) CHARACTER SET utf8) +BEGIN + PREPARE stmt FROM 'SELECT ?'; + EXECUTE stmt USING CONCAT(a, CONVERT(RAND() USING latin1)); + DEALLOCATE PREPARE stmt; +END; +$$ +DELIMITER ;$$ +--disable_result_log +CALL p1('x'); +--enable_result_log +DROP PROCEDURE p1; diff --git a/sql/sql_prepare.cc b/sql/sql_prepare.cc index 19c714c..d63c42c 100644 --- a/sql/sql_prepare.cc +++ b/sql/sql_prepare.cc @@ -2830,6 +2830,25 @@ void mysql_sql_stmt_prepare(THD *thd) DBUG_VOID_RETURN; }

+ /* + Make sure we call Prepared_statement::prepare() with an empty + THD::change_list. It can be non-empty as the above + LEX::get_dynamic_sql_string() calls fix_fields() for the Item + containing the PS source, e.g. on character set conversion: + + SET NAMES utf8; + DELIMITER $$ + CREATE PROCEDURE p1() + BEGIN + PREPARE stmt FROM CONCAT('SELECT ',CONVERT(RAND() USING latin1)); + EXECUTE stmt; + END; + $$ + DELIMITER ; + CALL p1(); + */ + Item_change_list save_change_list; + thd->change_list.move_elements_to(&save_change_list); if (stmt->prepare(query.str, (uint) query.length)) { /* Statement map deletes the statement on erase */ @@ -2840,6 +2859,7 @@ void mysql_sql_stmt_prepare(THD *thd) SESSION_TRACKER_CHANGED(thd, SESSION_STATE_CHANGE_TRACKER, NULL); my_ok(thd, 0L, 0L, "Statement prepared"); } + save_change_list.move_elements_to(&thd->change_list);

DBUG_VOID_RETURN; } @@ -2871,7 +2891,30 @@ void mysql_sql_stmt_execute_immediate(THD *thd) // See comments on thd->free_list in mysql_sql_stmt_execute() Item *free_list_backup= thd->free_list; thd->free_list= NULL; + /* + Make sure that we're call Prepared_statement::execute_immediate() + with an empty THD::change_list. It can be non empty as the above + LEX::prepared_stmt_params_fix_fields() and LEX::get_dynamic_str_string() + call fix_fields() for the PS source and PS parameter Items and + can do Item tree changes, e.g. on character set conversion: + + - Example #1: Item tree changes in get_dynamic_str_string() + SET NAMES utf8; + CREATE PROCEDURE p1() + EXECUTE IMMEDIATE CONCAT('SELECT ',CONVERT(RAND() USING latin1)); + CALL p1(); + + - Example #2: Item tree changes in prepared_stmt_param_fix_fields(): + SET NAMES utf8; + CREATE PROCEDURE p1(a VARCHAR(10) CHARACTER SET utf8) + EXECUTE IMMEDIATE 'SELECT ?' USING CONCAT(a, CONVERT(RAND() USING latin1)); + CALL p1('x'); + */ + Item_change_list save_change_list; + thd->change_list.move_elements_to(&save_change_list); (void) stmt->execute_immediate(query.str, (uint) query.length); + thd->rollback_item_tree_changes(); + save_change_list.move_elements_to(&thd->change_list); thd->free_items(); thd->free_list= free_list_backup;

@@ -3269,7 +3312,29 @@ void mysql_sql_stmt_execute(THD *thd) */ Item *free_list_backup= thd->free_list; thd->free_list= NULL; // Hide the external (e.g. "SET STATEMENT") Items + /* + Make sure we call Prepared_statement::execute_loop() with an empty + THD::change_list. It can be non-empty because the above + LEX::prepared_stmt_params_fix_fields() calls fix_fields() for + the PS parameter Items and can do some Item tree changes, + e.g. on character set conversion: + + SET NAMES utf8; + DELIMITER $$ + CREATE PROCEDURE p1(a VARCHAR(10) CHARACTER SET utf8) + BEGIN + PREPARE stmt FROM 'SELECT ?'; + EXECUTE stmt USING CONCAT(a, CONVERT(RAND() USING latin1)); + END; + $$ + DELIMITER ; + CALL p1('x'); + */ + Item_change_list save_change_list; + thd->change_list.move_elements_to(&save_change_list); (void) stmt->execute_loop(&expanded_query, FALSE, NULL, NULL); + thd->rollback_item_tree_changes(); + save_change_list.move_elements_to(&thd->change_list); thd->free_items(); // Free items created by execute_loop() /* Now restore the "external" (e.g. "SET STATEMENT") Item list.

Regards, Sergei Chief Architect MariaDB and security@mariadb.org