Hi, Oleksandr!
On Nov 06, Oleksandr Byelkin wrote:
> revision-id: 4de59fa0f92 (mariadb-10.2.27-129-g4de59fa0f92)
> parent(s): 4b21495343d
> author: Oleksandr Byelkin <sanja(a)mariadb.com>
> committer: Oleksandr Byelkin <sanja(a)mariadb.com>
> timestamp: 2019-10-18 15:14:20 +0200
> message:
>
> MENT-360: ASAN heap-use-after-free in strmake_root / Query_arena::strmake
>
> Do not allow thd->query point to freed memory, use safe method to set query.
>
> ---
> mysql-test/r/processlist.result | 32 +++++++++++++++++++++++
> mysql-test/t/processlist.test | 58 +++++++++++++++++++++++++++++++++++++++++
> sql/sql_prepare.cc | 7 ++++-
> 3 files changed, 96 insertions(+), 1 deletion(-)
>
> diff --git a/mysql-test/r/processlist.result b/mysql-test/r/processlist.result
> index ab518d961ef..066154feaf1 100644
> --- a/mysql-test/r/processlist.result
> +++ b/mysql-test/r/processlist.result
> @@ -43,3 +43,35 @@ Message Incorrect string value: '\xF0\x9F\x98\x8Eyy...' for column `information_
> #
> # End of 10.1 tests
> #
> +#
> +# MENT-360: ASAN heap-use-after-free in strmake_root /
> +# Query_arena::strmake
> +#
> +CREATE PROCEDURE pr1()
> +BEGIN
> +DECLARE CONTINUE HANDLER FOR 1146 SET @a = 1;
> +DECLARE CONTINUE HANDLER FOR 1243 SET @a = 1;
> +LOOP
> +PREPARE stmt FROM "CREATE TEMPORARY TABLE ps AS SELECT * FROM non_existing_table";
> +EXECUTE stmt;
> +END LOOP;
> +END $
> +CREATE PROCEDURE pr2()
> +BEGIN
> +DECLARE CONTINUE HANDLER FOR 1094 SET @a = 1;
> +LOOP
> +SHOW EXPLAIN FOR 1;
> +END LOOP;
> +END $
> +connect con1,localhost,root,,;
> +CALL pr1();
> +connect con2,localhost,root,,;
> +CALL pr2();
> +connection default;
> +KILL 6;
> +KILL 7;
^^^ you forgot replace_result
also, mysqltest was designed for deterministic test cases, it'd be
better to rewrite this test to be deterministic, if possible.
> +DROP PROCEDURE pr1;
> +DROP PROCEDURE pr2;
> +#
> +# end of Enterprise tests
> +#
> diff --git a/sql/sql_prepare.cc b/sql/sql_prepare.cc
> index 525f09d611b..d0043979135 100644
> --- a/sql/sql_prepare.cc
> +++ b/sql/sql_prepare.cc
> @@ -2885,6 +2885,11 @@ void mysql_sql_stmt_prepare(THD *thd)
>
> if (stmt->prepare(query.str, (uint) query.length))
> {
> + /*
> + stmt->prepare() sets thd->query_string, so we have to reset it beck,
> + before it will point to uninitialised memory
> + */
> + thd->set_query(orig_query);
> /* Statement map deletes the statement on erase */
> thd->stmt_map.erase(stmt);
> }
> @@ -2902,7 +2907,7 @@ void mysql_sql_stmt_prepare(THD *thd)
> But here we should restore the original query so it's mentioned in
> logs properly.
> */
> - thd->set_query_inner(orig_query);
> + thd->set_query(orig_query);
> DBUG_VOID_RETURN;
> }
may be better to have set_query() only once, like in
bool res=stmt->prepare(query.str, (uint) query.length);
thd->set_query(orig_query);
if (res) ...
Regards,
Sergei
VP of MariaDB Server Engineering
and security(a)mariadb.org
