Hi, Oleksandr! On Oct 09, Oleksandr Byelkin wrote:

revision-id: 62c35fe93e1 (mariadb-10.2.31-487-g62c35fe93e1) parent(s): f584d567dbb author: Oleksandr Byelkin <sanja@mariadb.com> committer: Oleksandr Byelkin <sanja@mariadb.com> 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@mariadb.org