maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Oleksandr!

On Nov 06, Oleksandr Byelkin wrote:
> revision-id: 4de59fa0f92 (mariadb-10.2.27-129-g4de59fa0f92)
> parent(s): 4b21495343d
> author: Oleksandr Byelkin <sanja@xxxxxxxxxxx>
> committer: Oleksandr Byelkin <sanja@xxxxxxxxxxx>
> 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@xxxxxxxxxxx