Re: bk commit into 5.0 tree (davi:1.2578) BUG#32890

[Konstantin Osipov]

* Davi Arnaut <davi@stripped> [08/02/16 20:42]:

> ChangeSet@stripped, 2008-02-15 20:36:24-02:00, davi@stripped +5 -0
>   Bug#32890 Crash after repeated create and drop of tables and views
>   
>   The problem is that CREATE VIEW statements inside prepared statements
>   weren't being expanded during the prepare phase, which leads to objects
>   not being allocated in the appropriate memory arenas.
>   
>   The solution is to perform the validation of CREATE VIEW statements
>   during the prepare phase of a prepared statement. The validation
>   during the prepare phase assures that transformations of the parsed
>   tree will use the permanent arena of the prepared statement.

Good work! A nice changeset comment too. Some comments below.
I will also run your patch in the debugger after writing this
mail, and might have additional comments after that.

> +#
> +# Bug#32890 Crash after repeated create and drop of tables and views
> +#
> +
> +--disable_warnings
> +drop view if exists v1;
> +drop table if exists t1;
> +--enable_warnings
> +
> +create table t1 (a int, b int);
> +insert into t1 values (1,1), (2,2), (3,3);
> +insert into t1 values (3,1), (1,2), (2,3);
> +prepare stmt from "create view v1 as select * from t1";
> +execute stmt;
> +drop table t1;
> +create table t1 (a int, b int);
> +drop view v1;
> +execute stmt;
> +show create view v1;
> +drop view v1;

OK, even though these tests don't crash the server without your
patch, they generate a lot of valgrind warnings.
Your patch fixes the issue.

Please execute every prepared statement twice in this test, and
re-check the results. The second execution must produce the same
results as the first.
Additionally, please add tests for:
CREATE VIEW that refers to a temporary table
CREATE VIEW that refers to a table that doesn't exist
CREATE VIEW where list of explicit column names doesn't
match the number of columns
CREATE VIEW with duplicate explicit column names
CREATE VIEW when auto-generated column names must be used (CREATE
VIEW v1 as SELECT 1, 1)

In other words, please add white-box test coverage for the
contents of mysql_test_create view().

For every case when PREPARE succeeds, please run EXECUTE twice.

>  
> +/**
> +  @brief Validate and prepare for execution CREATE VIEW statement
> +
> +  @param stmt prepared statement
> +
> +  @note This function handles create view commands.
> +
> +  @retval FALSE Operation was a success.
> +  @retval TRUE An error occured.
> +*/
> +
> +static bool mysql_test_create_view(Prepared_statement *stmt)
> +{
> +  DBUG_ENTER("mysql_test_create_view");
> +  THD *thd= stmt->thd;
> +  LEX *lex= stmt->lex;
> +  SELECT_LEX *select_lex= &lex->select_lex;
> +  bool res= TRUE;
> +  /* Skip first table, which is the view we are creating */
> +  bool link_to_local;
> +  TABLE_LIST *view= lex->unlink_first_table(&link_to_local);
> +  TABLE_LIST *tables= lex->query_tables;
> +
> +  if (create_view_precheck(thd, tables, view, lex->create_view_mode))
> +    goto err;
> +
> +  if (open_and_lock_tables(stmt->thd, tables))
> +    goto err;

Please just call open_tables(): we normally don't lock tables at
prepare.

> +
> +  lex->view_prepare_mode= 1;
> +  res= select_like_stmt_test(stmt, 0, 0);

OK.
>  /*
>    Validate and prepare for execution a multi update statement.
>  
> @@ -1730,6 +1769,7 @@ static bool check_prepared_statement(Pre
>        my_message(ER_UNSUPPORTED_PS, ER(ER_UNSUPPORTED_PS), MYF(0));
>        goto error;
>      }
> +    res= mysql_test_create_view(stmt);

OK. Good.

> +#ifndef NO_EMBEDDED_ACCESS_CHECKS
>  
> -/**
> -  @brief Creating/altering VIEW procedure
> -
> -  @param thd thread handler
> -  @param views views to create
> -  @param mode VIEW_CREATE_NEW, VIEW_ALTER, VIEW_CREATE_OR_REPLACE
> -
> -  @note This function handles both create and alter view commands.
> -
> -  @retval FALSE Operation was a success.
> -  @retval TRUE An error occured.
> -*/
> -
> -bool mysql_create_view(THD *thd, TABLE_LIST *views, 
> -                       enum_view_create_mode mode)

Please add a doxygen comment.

> +bool create_view_precheck(THD *thd, TABLE_LIST *tables, TABLE_LIST *view,
> +                          enum_view_create_mode mode)
>  {
>    LEX *lex= thd->lex;
> -  bool link_to_local;
>    /* first table in list is target VIEW name => cut off it */
> -  TABLE_LIST *view= lex->unlink_first_table(&link_to_local);
> -  TABLE_LIST *tables= lex->query_tables;
>    TABLE_LIST *tbl;
>    SELECT_LEX *select_lex= &lex->select_lex;
> -#ifndef NO_EMBEDDED_ACCESS_CHECKS
>    SELECT_LEX *sl;
> -#endif
> -  SELECT_LEX_UNIT *unit= &lex->unit;
> -  bool res= FALSE;
> -  DBUG_ENTER("mysql_create_view");
> -
> -  /* This is ensured in the parser. */
> -  DBUG_ASSERT(!lex->proc_list.first && !lex->result &&
> -              !lex->param_list.elements && !lex->derived_tables);
> -
> -  if (mode != VIEW_CREATE_NEW)
> -  {
> -    if (mode == VIEW_ALTER &&
> -        fill_defined_view_parts(thd, view))
> -    {
> -      res= TRUE;
> -      goto err;
> -    }
> -    sp_cache_invalidate();
> -  }
> -
> -  if (!lex->definer)
> -  {
> -    /*
> -      DEFINER-clause is missing; we have to create default definer in
> -      persistent arena to be PS/SP friendly.
> -      If this is an ALTER VIEW then the current user should be set as
> -      the definer.
> -    */
> -    Query_arena original_arena;
> -    Query_arena *ps_arena =
> thd->activate_stmt_arena_if_needed(&original_arena);
> -
> -    if (!(lex->definer= create_default_definer(thd)))
> -      res= TRUE;
> +  bool res= TRUE;
> +  DBUG_ENTER("create_view_precheck");
>  
> -    if (ps_arena)
> -      thd->restore_active_arena(ps_arena, &original_arena);
> -
> -    if (res)
> -      goto err;
> -  }
> -
> -#ifndef NO_EMBEDDED_ACCESS_CHECKS
> -  /*
> -    check definer of view:
> -      - same as current user
> -      - current user has SUPER_ACL
> -  */
> -  if (lex->definer &&
> -      (strcmp(lex->definer->user.str, thd->security_ctx->priv_user) != 0
> ||
> -       my_strcasecmp(system_charset_info,
> -                     lex->definer->host.str,
> -                     thd->security_ctx->priv_host) != 0))
> -  {
> -    if (!(thd->security_ctx->master_access & SUPER_ACL))
> -    {
> -      my_error(ER_SPECIFIC_ACCESS_DENIED_ERROR, MYF(0), "SUPER");
> -      res= TRUE;
> -      goto err;
> -    }
> -    else
> -    {
> -      if (!is_acl_user(lex->definer->host.str,
> -                       lex->definer->user.str))
> -      {
> -        push_warning_printf(thd, MYSQL_ERROR::WARN_LEVEL_NOTE,
> -                            ER_NO_SUCH_USER,
> -                            ER(ER_NO_SUCH_USER),
> -                            lex->definer->user.str,
> -                            lex->definer->host.str);
> -      }
> -    }
> -  }
>    /*
>      Privilege check for view creation:
>      - user has CREATE VIEW privilege on view table
> @@ -323,10 +238,8 @@ bool mysql_create_view(THD *thd, TABLE_L
>         (check_access(thd, DROP_ACL, view->db, &view->grant.privilege,
>                       0, 0, is_schema_db(view->db)) ||
>          grant_option && check_grant(thd, DROP_ACL, view, 0, 1, 0))))
> -  {
> -    res= TRUE;
>      goto err;
> -  }
> +
>    for (sl= select_lex; sl; sl= sl->next_select())
>    {
>      for (tbl= sl->get_table_list(); tbl; tbl= tbl->next_local)
> @@ -340,7 +253,6 @@ bool mysql_create_view(THD *thd, TABLE_L
>          my_error(ER_TABLEACCESS_DENIED_ERROR, MYF(0),
>                   "ANY", thd->security_ctx->priv_user,
>                   thd->security_ctx->priv_host, tbl->table_name);
> -        res= TRUE;
>          goto err;
>        }
>        /*
> @@ -376,10 +288,7 @@ bool mysql_create_view(THD *thd, TABLE_L
>          if (check_access(thd, SELECT_ACL, tbl->db,
>                           &tbl->grant.privilege, 0, 0,
> test(tbl->schema_table)) ||
>              grant_option && check_grant(thd, SELECT_ACL, tbl, 0, 1, 0))
> -        {
> -          res= TRUE;
>            goto err;
> -        }
>        }
>      }
>    }
> @@ -403,7 +312,125 @@ bool mysql_create_view(THD *thd, TABLE_L
>        }
>      }
>    }
> +
> +  res= FALSE;
> +
> +err:
> +  DBUG_RETURN(res || thd->net.report_error);
> +}

Even though we do some things twice now, at prepare and at
execute, it should be OK. Good.


> +#else
> +
> +bool create_view_precheck(THD *thd, TABLE_LIST *tables, TABLE_LIST *view,
> +                          enum_view_create_mode mode)
> +{
> +  return FALSE;
> +}

OK.

> +
> +#endif
> +
> +
> +/**
> +  @brief Creating/altering VIEW procedure
> +
> +  @param thd thread handler
> +  @param views views to create
> +  @param mode VIEW_CREATE_NEW, VIEW_ALTER, VIEW_CREATE_OR_REPLACE
> +
> +  @note This function handles both create and alter view commands.
> +
> +  @retval FALSE Operation was a success.
> +  @retval TRUE An error occured.
> +*/
> +
> +bool mysql_create_view(THD *thd, TABLE_LIST *views,
> +                       enum_view_create_mode mode)
> +{
> +  LEX *lex= thd->lex;
> +  bool link_to_local;
> +  /* first table in list is target VIEW name => cut off it */
> +  TABLE_LIST *view= lex->unlink_first_table(&link_to_local);
> +  TABLE_LIST *tables= lex->query_tables;
> +  TABLE_LIST *tbl;
> +  SELECT_LEX *select_lex= &lex->select_lex;
> +#ifndef NO_EMBEDDED_ACCESS_CHECKS
> +  SELECT_LEX *sl;
> +#endif
> +  SELECT_LEX_UNIT *unit= &lex->unit;
> +  bool res= FALSE;
> +  DBUG_ENTER("mysql_create_view");
> +
> +  /* This is ensured in the parser. */
> +  DBUG_ASSERT(!lex->proc_list.first && !lex->result &&
> +              !lex->param_list.elements && !lex->derived_tables);
> +
> +  if (mode != VIEW_CREATE_NEW)
> +  {
> +    if (mode == VIEW_ALTER &&
> +        fill_defined_view_parts(thd, view))
> +    {
> +      res= TRUE;
> +      goto err;
> +    }
> +    sp_cache_invalidate();
> +  }
> +
> +  if (!lex->definer)
> +  {
> +    /*
> +      DEFINER-clause is missing; we have to create default definer in
> +      persistent arena to be PS/SP friendly.
> +      If this is an ALTER VIEW then the current user should be set as
> +      the definer.
> +    */
> +    Query_arena original_arena;
> +    Query_arena *ps_arena =
> thd->activate_stmt_arena_if_needed(&original_arena);
> +
> +    if (!(lex->definer= create_default_definer(thd)))
> +      res= TRUE;
> +
> +    if (ps_arena)
> +      thd->restore_active_arena(ps_arena, &original_arena);
> +
> +    if (res)
> +      goto err;
> +  }
> +
> +#ifndef NO_EMBEDDED_ACCESS_CHECKS
> +  /*
> +    check definer of view:
> +      - same as current user
> +      - current user has SUPER_ACL
> +  */
> +  if (lex->definer &&
> +      (strcmp(lex->definer->user.str, thd->security_ctx->priv_user) != 0
> ||
> +       my_strcasecmp(system_charset_info,
> +                     lex->definer->host.str,
> +                     thd->security_ctx->priv_host) != 0))
> +  {
> +    if (!(thd->security_ctx->master_access & SUPER_ACL))
> +    {
> +      my_error(ER_SPECIFIC_ACCESS_DENIED_ERROR, MYF(0), "SUPER");
> +      res= TRUE;
> +      goto err;
> +    }
> +    else
> +    {
> +      if (!is_acl_user(lex->definer->host.str,
> +                       lex->definer->user.str))
> +      {
> +        push_warning_printf(thd, MYSQL_ERROR::WARN_LEVEL_NOTE,
> +                            ER_NO_SUCH_USER,
> +                            ER(ER_NO_SUCH_USER),
> +                            lex->definer->user.str,
> +                            lex->definer->host.str);
> +      }
> +    }
> +  }
>  #endif
> +
> +  if ((res= create_view_precheck(thd, tables, view, mode)))
> +    goto err;

OK.

>    if (open_and_lock_tables(thd, tables))
>    {
> diff -Nrup a/sql/sql_view.h b/sql/sql_view.h
> --- a/sql/sql_view.h	2006-12-23 17:04:28 -02:00
> +++ b/sql/sql_view.h	2008-02-15 20:36:22 -02:00
> @@ -15,6 +15,9 @@
>     Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
>  */
>  
> +bool create_view_precheck(THD *thd, TABLE_LIST *tables, TABLE_LIST *view,
> +                          enum_view_create_mode mode);
> +
>  bool mysql_create_view(THD *thd, TABLE_LIST *view,
>                         enum_view_create_mode mode);

The patch is OK to push after addressing the
review comments.

Thank you for working on this,

-- 
-- Konstantin Osipov              Software Developer, Moscow, Russia
-- MySQL AB, www.mysql.com   The best DATABASE COMPANY in the GALAXY