Re: [Maria-developers] d7b274fa257: MDEV-16932: ASAN heap-use-after-free in my_charlen_utf8 / my_well_formed_char_length_utf8 on 2nd execution of SP with ALTER trying to add bad CHECK

Hi, Oleksandr! A couple of minor comments, see below On Jul 15, Oleksandr Byelkin wrote:

revision-id: d7b274fa257 (mariadb-10.2.25-63-gd7b274fa257) parent(s): 64900e3d7c3 author: Oleksandr Byelkin committer: Oleksandr Byelkin timestamp: 2019-07-11 13:29:51 +0200 message:

MDEV-16932: ASAN heap-use-after-free in my_charlen_utf8 / my_well_formed_char_length_utf8 on 2nd execution of SP with ALTER trying to add bad CHECK

Make automatic name generation during execution (not prepare).

Check result of memory allocation operation.

diff --git a/sql/sql_table.cc b/sql/sql_table.cc index 043cfaaaaaa..636fb9f427c 100644 --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -6205,6 +6210,10 @@ handle_if_exists_options(THD *thd, TABLE *table, Alter_info *alter_info) Virtual_column_info *check; TABLE_SHARE *share= table->s; uint c; + + if (fix_constraints_names(thd, &alter_info->check_constraint_list)) + DBUG_RETURN(TRUE);

It's not very logical, I think, to generate constraint names in handle_if_exists_options(), I'd rather move it out and put directly in mysql_alter_table().

+ while ((check=it++)) { if (!(check->flags & Alter_info::CHECK_CONSTRAINT_IF_NOT_EXISTS) && @@ -6232,10 +6241,44 @@ handle_if_exists_options(THD *thd, TABLE *table, Alter_info *alter_info) } }

- DBUG_VOID_RETURN; + DBUG_RETURN(FALSE); }

+static bool fix_constraints_names(THD *thd, List + *check_constraint_list) +{ + List_iterator it((*check_constraint_list)); + Virtual_column_info *check; + uint nr= 1; + DBUG_ENTER("fix_constraints_names"); + if (!check_constraint_list) + DBUG_RETURN(FALSE); + // Prevent accessing freed memory during generating unique names + while ((check=it++)) + { + if (check->automatic_name) + { + check->name.str= NULL; + check->name.length= 0; + } + } + it.rewind(); + // Grnerate unique names if needed

typo

+ while ((check=it++)) + { + if (!check->name.length) + { + check->automatic_name= TRUE; + if (make_unique_constraint_name(thd, &check->name, + check_constraint_list, + &nr)) + DBUG_RETURN(TRUE); + } + } + DBUG_RETURN(FALSE);

I would remove the first while() at all and just do the second one till the end: bool ret= FALSE; while ((check=it++)) if (check->automatic_name) ret |= make_unique_constraint_name(...); return ret; Because thd->strmake() basically never fails, it doesn't make much sense to optimize for a case when it does. This code above assumes you set automatic_name=TRUE in mysql_prepare_create_table().

+} + /** Get Create_field object for newly created table by field index.

Regards, Sergei