maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Oleksandr!

On Jul 29, Oleksandr Byelkin wrote:
> revision-id: bc73b455ba2 (mariadb-10.2.31-318-gbc73b455ba2)
> parent(s): 7311586ca25
> author: Oleksandr Byelkin <sanja@xxxxxxxxxxx>
> committer: Oleksandr Byelkin <sanja@xxxxxxxxxxx>
> timestamp: 2020-07-17 13:47:26 +0200
> message:
> 
> MDEV-15703 Crash in EXECUTE IMMEDIATE 'CREATE OR REPLACE TABLE t1 (a INT DEFAULT ?)' USING DEFAULT
> 
> part 2:
...
> diff --git a/sql/field.h b/sql/field.h
> index d5e2cb25788..2755f5f687c 100644
> --- a/sql/field.h
> +++ b/sql/field.h
> @@ -51,6 +51,16 @@ enum enum_check_fields
>    CHECK_FIELD_ERROR_FOR_NULL
>  };
>  
> +
> +enum ignore_value_reaction
> +{
> +  IGNORE_MEANS_ERROR,
> +  IGNORE_MEANS_DEFAULT,
> +  IGNORE_MEANS_FIELD_VALUE

I thought you'll have IGNORE_MEANS_IGNORE :)

> +};
> +
> +ignore_value_reaction find_ignore_reaction(THD *thd);
> +
>  /*
>    Common declarations for Field and Item
>  */
> diff --git a/sql/item.h b/sql/item.h
> index 7338c8be47b..dabf237712b 100644
> --- a/sql/item.h
> +++ b/sql/item.h
> @@ -1404,6 +1424,16 @@ class Item: public Value_source,
>                       LOWEST_PRECEDENCE);
>    }
>    virtual void print(String *str, enum_query_type query_type);
> +
> +  class Print: public String
> +  {
> +  public:
> +    Print(Item *item, enum_query_type type)
> +    {
> +      item->print(this, type);
> +    }

no, I don't think this should be part of the bugfix.

> +  };
> +
>    void print_item_w_name(String *str, enum_query_type query_type);
>    void print_value(String *str);
>  
> @@ -2077,6 +2107,15 @@ class Item: public Value_source,
>    {
>      marker &= ~EXTRACTION_MASK;
>    }
> +
> +  virtual bool associate_with_target_field(THD *thd __attribute__((unused)),
> +                                           Item_field *field
> +                                             __attribute__((unused)))
> +  {
> +    DBUG_ASSERT(fixed);
> +    DBUG_ASSERT(fixed);

you want to be double-sure? :)

> +    return false;
> +  }
>  };
>  
>  
> diff --git a/sql/item.cc b/sql/item.cc
> index 382eb3ac47a..d11017f3ca6 100644
> --- a/sql/item.cc
> +++ b/sql/item.cc
> @@ -84,6 +85,15 @@ void item_init(void)
>  }
>  
>  
> +void Item::raise_error_not_evaluable()
> +{
> +  Item::Print tmp(this, 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());

I don't see any tests for this error

> +}
> +
> +
>  void Item::push_note_converted_to_negative_complement(THD *thd)
>  {
>    push_warning(thd, Sql_condition::WARN_LEVEL_NOTE, ER_UNKNOWN_ERROR,
> @@ -8945,6 +9022,14 @@ bool Item_default_value::fix_fields(THD *thd, Item **items)
>      return FALSE;
>    }
>  
> +  return tie_field(thd);
> +}
> +
> +

comment? what does tie_field do?

> +bool Item_default_value::tie_field(THD *thd)
> +{
> +  Item *real_arg;
> +  Item_field *field_arg;
>    /*
>      DEFAULT() do not need table field so should not ask handler to bring
>      field value (mark column for read)
> @@ -9131,8 +9241,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);

may be, remove it from the write_set instead?

> +  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)
> +  {
> +    switch (find_ignore_reaction(field->table->in_use))
> +    {
> +      case IGNORE_MEANS_DEFAULT:
> +        DBUG_ASSERT(0); // impossible now, but fully working code if needed

Impossible? One cannot use IGNORE in INSERT?
Not even in EXECUTE IMMEDIATE "INSERT ... " USING IGNORE?

> +        /*
> +          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/sql_base.cc b/sql/sql_base.cc
> index 436f753557e..4dbda30708a 100644
> --- a/sql/sql_base.cc
> +++ b/sql/sql_base.cc
> @@ -8340,6 +8341,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;

this looks like the wrong way of doing it.
whether an item is "evaluable" or not is metadata. It should be checked once
before the statement execution

> +
>      if (use_value)
>        value->save_val(field);
>      else
> diff --git a/sql/sql_update.cc b/sql/sql_update.cc
> index 65a828147ae..26ee427e3e7 100644
> --- a/sql/sql_update.cc
> +++ b/sql/sql_update.cc
> @@ -1796,6 +1796,9 @@ int multi_update::prepare(List<Item> &not_used_values,
>    while ((item= (Item_field *) field_it++))
>    {
>      Item *value= value_it++;
> +    if (value->associate_with_target_field(thd, item))
> +      DBUG_RETURN(1);

what happens for single table updates?

> +
>      uint offset= item->field->table->pos_in_table_list->shared;
>      fields_for_table[offset]->push_back(item, thd->mem_root);
>      values_for_table[offset]->push_back(value, thd->mem_root);
> diff --git a/sql/unireg.cc b/sql/unireg.cc
> index 083960523c1..a3bfc678e07 100644
> --- a/sql/unireg.cc
> +++ b/sql/unireg.cc
> @@ -1000,8 +1000,14 @@ static bool make_empty_rec(THD *thd, uchar *buff, uint table_options,
>      {
>        Item *expr= field->default_value->expr;
>  
> -      int res= !expr->fixed && // may be already fixed if ALTER TABLE
> -                expr->fix_fields(thd, &expr);
> +      int res= MY_TEST(!expr->fixed && // may be already fixed if ALTER TABLE
> +                        expr->fix_fields(thd, &expr));

eh? what's the point of this change?

> +      if (!res && expr->check_is_evaluable_expression_or_error())
> +      {
> +        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