← Back to team overview

maria-developers team mailing list archive

  1. maria-developers team
  2. Mailing list archive
  3. Message #11909

Re: 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

  Thread Previous • Date Previous • • Thread Next 
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@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().
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@xxxxxxxxxxxxxxxxxxx
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

References

  Thread Previous • Date Previous • • Thread Next