maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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@xxxxxxxxxxx