maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Oleksandr!

On Oct 09, Oleksandr Byelkin wrote:
> revision-id: 62c35fe93e1 (mariadb-10.2.31-487-g62c35fe93e1)
> parent(s): f584d567dbb
> author: Oleksandr Byelkin <sanja@xxxxxxxxxxx>
> committer: Oleksandr Byelkin <sanja@xxxxxxxxxxx>
> timestamp: 2020-10-07 13:43:39 +0200
> message:
> 
> MDEV-15703 Crash in EXECUTE IMMEDIATE 'CREATE OR REPLACE TABLE t1 (a INT DEFAULT ?)' USING DEFAULT
> 
> part 2:
> 
> - check that expressions are evaluable for
>   making empty row and assigning PS variable
> 
> - correctly handling writing to a temporary tabe during multi-update
>   by setting associated field
> 
> - Item::raise_error_not_evaluable() bakported from 10.4 and made vitrual
> 
> - Item::is_evaluable_expression() bakported from 10.4
> 
> - Item::check_is_evaluable_expression_or_error() bakported from 10.4

I still don't quite understand all details of your patch. But here're
more questions and comments below:

> diff --git a/sql/field.cc b/sql/field.cc
> index bdaaecc2026..8ce412a207d 100644
> --- a/sql/field.cc
> +++ b/sql/field.cc
> @@ -11183,6 +11205,16 @@ bool Field::save_in_field_default_value(bool view_error_processing)
>  {
>    THD *thd= table->in_use;
>  
> +  if ( // table is not opened properly
> +      table == NULL || table->pos_in_table_list == NULL ||
> +      table->pos_in_table_list->select_lex == NULL ||
> +      // we are in subquery/derived/CTE
> +      !table->pos_in_table_list->top_table()->select_lex->is_top_level_select())
> +  {
> +    DBUG_ASSERT(0); // shoud not happened
> +    my_error(ER_INVALID_DEFAULT_PARAM, MYF(0));
> +    return false;
> +  }

I'd still think it'd be better to do a series of asserts.
a complex condition to check for something that cannot ever happen?

>    if (flags & NO_DEFAULT_VALUE_FLAG &&
>        real_type() != MYSQL_TYPE_ENUM)
>    {
> @@ -11221,13 +11253,30 @@ bool Field::save_in_field_default_value(bool view_error_processing)
>  
>  bool Field::save_in_field_ignore_value(bool view_error_processing)
>  {
> -  enum_sql_command com= table->in_use->lex->sql_command;
> -  // All insert-like commands
> -  if (com == SQLCOM_INSERT || com == SQLCOM_REPLACE ||
> -      com == SQLCOM_INSERT_SELECT || com == SQLCOM_REPLACE_SELECT ||
> -      com == SQLCOM_LOAD)
> -    return save_in_field_default_value(view_error_processing);
> -  return 0; // ignore
> +  if ( // table is not opened properly
> +      table == NULL || table->pos_in_table_list == NULL ||
> +      table->pos_in_table_list->select_lex == NULL ||
> +      // we are in subquery/derived/CTE
> +      !table->pos_in_table_list->top_table()->select_lex->is_top_level_select())
> +  {
> +    DBUG_ASSERT(0); // shoud not happened
> +    my_error(ER_INVALID_DEFAULT_PARAM, MYF(0));
> +    return false;
> +  }

ditto

> +  switch (find_ignore_reaction(table->in_use))
> +  {
> +    case IGNORE_MEANS_DEFAULT:
> +      return save_in_field_default_value(view_error_processing);
> +    case IGNORE_MEANS_FIELD_VALUE:
> +      return 0; // ignore
> +    default:
> +      ; // fall through to error
> +  }
> +
> +  // unexpected command
> +  DBUG_ASSERT(0); // shoud not happened
> +  my_error(ER_INVALID_DEFAULT_PARAM, MYF(0));
> +  return false;
>  }
>  
>  
> diff --git a/sql/item.cc b/sql/item.cc
> index 994d45a9dc3..401ee5d6be2 100644
> --- a/sql/item.cc
> +++ b/sql/item.cc
> @@ -84,6 +85,17 @@ void item_init(void)
>  }
>  
>  
> +void Item::raise_error_not_evaluable()
> +{
> +  String tmp;
> +  this->print(&tmp, QT_ORDINARY);
> +  // TODO-10.5: add an error message to errmsg-utf8.txt
> +  my_printf_error(ER_UNKNOWN_ERROR,
> +                  "'%s' is not allowed in this context", MYF(0), tmp.ptr());
> +  DBUG_ASSERT(0); // cannot be happen before 10.5
> +}
> +
> +

better remove it, this dead code won't simplify merges. In 10.3
it'll be added with a merge normally.

having check_is_evaluable_expression_or_error() makes sense though,
it is used and copying it from 10.3 will help to merge indeed

or, may be, just backport the patch from 10.3 to 10.2?

>  void Item::push_note_converted_to_negative_complement(THD *thd)
>  {
>    push_warning(thd, Sql_condition::WARN_LEVEL_NOTE, ER_UNKNOWN_ERROR,
> @@ -8968,18 +9084,26 @@ bool Item_default_value::fix_fields(THD *thd, Item **items)
>      def_field->move_field_offset((my_ptrdiff_t)
>                                   (def_field->table->s->default_values -
>                                    def_field->table->record[0]));
> -  set_field(def_field);
> -  return FALSE;
> +  return def_field;
> +}
>  
> -error:
> -  context->process_error(thd);
> -  return TRUE;
> +bool Item_default_value::associate_with_target_field(THD *thd,
> +                                                     Item_field *field)
> +{
> +  associated= true;
> +  arg= field;

iirc, `arg` used to tell the difference between DEFAULT and DEFAULT(x).
how are you doing it now?

> +  return tie_field(thd);
>  }
>  
>  void Item_default_value::cleanup()
>  {
>    delete cached_field;                        // Free cached blob data
>    cached_field= 0;
> +  if (associated)
> +  {
> +    arg= NULL;
> +    associated= false;
> +  }
>    Item_field::cleanup();
>  }
>  
> @@ -9102,8 +9226,54 @@ void Item_ignore_value::print(String *str, enum_query_type query_type)
>      str->append(STRING_WITH_LEN("ignore"));
>  }
>  
> +
> +bool Item_ignore_value::associate_with_target_field(THD *thd,
> +                                                    Item_field *field)
> +{
> +  bitmap_set_bit(field->field->table->read_set, field->field->field_index);
> +  return Item_default_value::associate_with_target_field(thd, field);
> +}
> +
> +
>  int Item_ignore_value::save_in_field(Field *field_arg, bool no_conversions)
>  {
> +  if (arg)

how is IGNORE with arg!=0 differs from IGNORE with arg==0?
when happens what?

> +  {
> +    switch (find_ignore_reaction(field->table->in_use))
> +    {
> +      case IGNORE_MEANS_DEFAULT:
> +        DBUG_ASSERT(0); // impossible now, but fully working code if needed
> +        /*
> +          save default value
> +          Note: (((Field*)(arg()->real_item()) is Item_field* (checked in
> +          tie_field().
> +        */
> +        if ((((Item_field*)(arg->real_item()))->field->flags &
> +             NO_DEFAULT_VALUE_FLAG))
> +        {
> +          my_error(ER_NO_DEFAULT_FOR_FIELD, MYF(0),
> +                   ((Item_field*)(arg->real_item()))->field->field_name);
> +          return true;
> +        }
> +        calculate();
> +        return Item_field::save_in_field(field_arg, no_conversions);
> +      case IGNORE_MEANS_FIELD_VALUE:
> +        // checked on tie_field
> +        DBUG_ASSERT(arg->real_item()->type() == FIELD_ITEM);
> +        /*
> +          save original field (Item::save_in_field is not applicable because
> +          result_field for the referenced field set to this temporary table).
> +        */
> +        arg->save_val(field_arg);
> +        return false;
> +      default:
> +         ; // fall through to error
> +    }
> +    DBUG_ASSERT(0); //impossible
> +    my_error(ER_INVALID_DEFAULT_PARAM, MYF(0));
> +    return true;
> +  }
> +
>    return field_arg->save_in_field_ignore_value(context->error_processor ==
>                                                 &view_error_processor);
>  }
> diff --git a/sql/sp_head.cc b/sql/sp_head.cc
> index 6a650183fb8..41a27a3aaaf 100644
> --- a/sql/sp_head.cc
> +++ b/sql/sp_head.cc
> @@ -418,6 +418,9 @@ sp_eval_expr(THD *thd, Field *result_field, Item **expr_item_ptr)
>    if (!(expr_item= sp_prepare_func_item(thd, expr_item_ptr)))
>      goto error;
>  
> +  if (expr_item->check_is_evaluable_expression_or_error())
> +    goto error;

for CALL(?) ?

> +
>    /*
>      Set THD flags to emit warnings/errors in case of overflow/type errors
>      during saving the item into the field.
> diff --git a/sql/sql_base.cc b/sql/sql_base.cc
> index cc77b58cb3e..b52d6f96a84 100644
> --- a/sql/sql_base.cc
> +++ b/sql/sql_base.cc
> @@ -8345,6 +8346,9 @@ fill_record(THD *thd, TABLE *table, Field **ptr, List<Item> &values,
>                            field->field_name, table->s->table_name.str);
>      }
>  
> +    if(evaluable && value->check_is_evaluable_expression_or_error())
> +      goto err;

Is it a good idea to do this check constantly for every user and for every
row in selects, insert/update, etc?

Could it be done inside IGNORE/DEFAULT? For example, by checking
if field->table->tmp_table == INTERNAL_TMP_TABLE.

> +
>      if (use_value)
>        value->save_val(field);
>      else
> diff --git a/sql/unireg.cc b/sql/unireg.cc
> index 083960523c1..91f9073c7ef 100644
> --- a/sql/unireg.cc
> +++ b/sql/unireg.cc
> @@ -1002,6 +1002,12 @@ static bool make_empty_rec(THD *thd, uchar *buff, uint table_options,
>  
>        int res= !expr->fixed && // may be already fixed if ALTER TABLE
>                  expr->fix_fields(thd, &expr);
> +      if (!res && expr->check_is_evaluable_expression_or_error())

is this for CREATE TABLE case?

> +      {
> +        error= 1;
> +        delete regfield; //To avoid memory leak
> +        goto err;
> +      }
>        if (!res)
>          res= expr->save_in_field(regfield, 1);
>        if (!res && (field->flags & BLOB_FLAG))
 
Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx