Re: [Maria-developers] bc73b455ba2: MDEV-15703 Crash in EXECUTE IMMEDIATE 'CREATE OR REPLACE TABLE t1 (a INT DEFAULT ?)' USING DEFAULT

Hi, Oleksandr! On Jul 29, Oleksandr Byelkin wrote:

revision-id: bc73b455ba2 (mariadb-10.2.31-318-gbc73b455ba2) parent(s): 7311586ca25 author: Oleksandr Byelkin committer: Oleksandr Byelkin 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> ¬_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@mariadb.org