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 <sa...@mariadb.com>
> committer: Oleksandr Byelkin <sa...@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
_______________________________________________
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
