maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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@xxxxxxxxxxx>
> committer: Oleksandr Byelkin <sanja@xxxxxxxxxxx>
> 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