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 <sanja@mariadb.com> committer: Oleksandr Byelkin <sanja@mariadb.com> 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<Virtual_column_info> + *check_constraint_list) +{ + List_iterator<Virtual_column_info> 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
Am 15.07.19 um 19:21 schrieb Sergei Golubchik:
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 <sanja@mariadb.com> committer: Oleksandr Byelkin <sanja@mariadb.com> 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().
Absolutely agree. I tough to move but decided that it is too radical change, but if you think so also I will do.
+ 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<Virtual_column_info> + *check_constraint_list) +{ + List_iterator<Virtual_column_info> 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 ok
+ 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. it is not optimisation it is just bug in error reporting, just several month ago was fixing bug with the code like this, in any case it is better to report error than coredump (it is especially important because we do not have regular testing with low memory).
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
_______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
participants (2)
-
Oleksandr Byelkin -
Sergei Golubchik