MDEV-11146 SP variables of the SET data type erroneously allow values with comma

There was a duplicate code to create TYPELIB from List<String>:
- In typelib() and mysql_prepare_create_table(), which was used to initialize
  table fields.
- create_typelib() and sp_prepare_create_field(), which was used to initialize
  SP variables.
create_typelib() was incomplete and didn't check for wrong SET values.

Fix:
- Moving the code from create_typelib() and mysql_prepare_create_field()
  to news methods Column_definition::create_interval_from_interval_list()
  and Column_definition::prepare_interval_field().
- Moving the code from calculate_interval_lengths() in sql_table.cc
  to a new method Column_definition::calculate_interval_lengths(), as it's now
  needed only in Column_definition::create_interval_from_interval_list()
- Reusing the new method Column_definition::prepare_interval_field() in both
  mysql_prepare_create_table() and sp_prepare_create_field(), instead of the
  old duplicate code pieces
- Removing global functions typelib() and create_typelib()

This patch also fixes:
MDEV-11155 Bad error message when creating a SET column with comma and non-ASCII characters
The problem was that ErrCongString() was called with a wrong "charset" parameter.

mysql-test/r/ctype_utf8.result

@@ -11142,3 +11142,15 @@ SET STORAGE_ENGINE=Default;
 #
 # End of 10.2 tests
 #
+#
+# Start of 10.3 tests
+#
+#
+# MDEV-11155 Bad error message when creating a SET column with comma and non-ASCII characters
+#
+SET NAMES utf8;
+CREATE TABLE t1 (a SET('a,bü'));
+ERROR 22007: Illegal set 'a,bü' value found during parsing
+#
+# End of 10.3 tests
+#

mysql-test/r/sp-bugs.result

@@ -281,3 +281,14 @@ end|
 start transaction;
 call sp();
 drop procedure sp;
+#
+# MDEV-11146 SP variables of the SET data type erroneously allow values with comma
+#
+CREATE PROCEDURE p1()
+BEGIN
+DECLARE a SET('a','b','c','a,b');
+SET a='a,b';
+SELECT a, a+0;
+END;
+$$
+ERROR 22007: Illegal set 'a,b' value found during parsing

mysql-test/t/ctype_utf8.test

@@ -2074,3 +2074,20 @@ let $coll_pad='utf8_bin';
 --echo #
 --echo # End of 10.2 tests
 --echo #
+
+--echo #
+--echo # Start of 10.3 tests
+--echo #
+
+
+--echo #
+--echo # MDEV-11155 Bad error message when creating a SET column with comma and non-ASCII characters
+--echo #
+
+SET NAMES utf8;
+--error ER_ILLEGAL_VALUE_FOR_TYPE
+CREATE TABLE t1 (a SET('a,bü'));
+
+--echo #
+--echo # End of 10.3 tests
+--echo #

mysql-test/t/sp-bugs.test

@@ -307,3 +307,17 @@ start transaction;
 call sp();
 drop procedure sp;
 
+--echo #
+--echo # MDEV-11146 SP variables of the SET data type erroneously allow values with comma
+--echo #
+
+DELIMITER $$;
+--error ER_ILLEGAL_VALUE_FOR_TYPE
+CREATE PROCEDURE p1()
+BEGIN
+  DECLARE a SET('a','b','c','a,b');
+  SET a='a,b';
+  SELECT a, a+0;
+END;
+$$
+DELIMITER ;$$

sql/field.cc

@@ -9721,6 +9721,86 @@ void Field_bit_as_char::sql_type(String &res) const
   Handling of field and Create_field
 *****************************************************************************/
 
+bool Column_definition::create_interval_from_interval_list(MEM_ROOT *mem_root,
+                                               bool reuse_interval_list_values)
+{
+  DBUG_ENTER("Column_definition::create_interval_from_interval_list");
+  DBUG_ASSERT(!interval);
+  if (!(interval= (TYPELIB*) alloc_root(mem_root, sizeof(TYPELIB))))
+    DBUG_RETURN(true); // EOM
+
+  List_iterator<String> it(interval_list);
+  StringBuffer<64> conv;
+  char comma_buf[5]; /* 5 bytes for 'filename' charset */
+  DBUG_ASSERT(sizeof(comma_buf) >= charset->mbmaxlen);
+  int comma_length= charset->cset->wc_mb(charset, ',',
+                                         (uchar*) comma_buf,
+                                         (uchar*) comma_buf +
+                                         sizeof(comma_buf));
+  DBUG_ASSERT(comma_length >= 0 && comma_length <= (int) sizeof(comma_buf));
+
+  if (!multi_alloc_root(mem_root,
+                        &interval->type_names,
+                        sizeof(char*) * (interval_list.elements + 1),
+                        &interval->type_lengths,
+                        sizeof(uint) * (interval_list.elements + 1),
+                        NullS))
+    goto err; // EOM
+
+  interval->name= "";
+  interval->count= interval_list.elements;
+
+  for (uint i= 0; i < interval->count; i++)
+  {
+    uint32 dummy;
+    String *tmp= it++;
+    LEX_CSTRING value;
+    if (String::needs_conversion(tmp->length(), tmp->charset(),
+                                 charset, &dummy))
+    {
+      uint cnv_errs;
+      conv.copy(tmp->ptr(), tmp->length(), tmp->charset(), charset, &cnv_errs);
+      value.str= strmake_root(mem_root, conv.ptr(), conv.length());
+      value.length= conv.length();
+    }
+    else
+    {
+      value.str= reuse_interval_list_values ? tmp->ptr() :
+                                              strmake_root(mem_root,
+                                                           tmp->ptr(),
+                                                           tmp->length());
+      value.length= tmp->length();
+    }
+    if (!value.str)
+      goto err; // EOM
+
+    // Strip trailing spaces.
+    value.length= charset->cset->lengthsp(charset, value.str, value.length);
+    ((char*) value.str)[value.length]= '\0';
+
+    if (sql_type == MYSQL_TYPE_SET)
+    {
+      if (charset->coll->instr(charset, value.str, value.length,
+                               comma_buf, comma_length, NULL, 0))
+      {
+        ErrConvString err(tmp);
+        my_error(ER_ILLEGAL_VALUE_FOR_TYPE, MYF(0), "set", err.ptr());
+        goto err;
+      }
+    }
+    interval->type_names[i]= value.str;
+    interval->type_lengths[i]= value.length;
+  }
+  interval->type_names[interval->count]= 0;    // End marker
+  interval->type_lengths[interval->count]= 0;
+  interval_list.empty();  // Don't need interval_list anymore
+  DBUG_RETURN(false);
+err:
+  interval= NULL;  // Avoid having both non-empty interval_list and interval
+  DBUG_RETURN(true);
+}
+
+
 /**
   Convert create_field::length from number of characters to number of bytes.
 */
@@ -10533,6 +10613,9 @@ Column_definition::Column_definition(THD *thd, Field *old_field,
     interval= ((Field_enum*) old_field)->typelib;
   else
     interval=0;
+
+  interval_list.empty(); // prepare_interval_field() needs this
+
   char_length= length;
 
   /*

sql/field.h

@@ -3726,6 +3726,44 @@ Field *make_field(TABLE_SHARE *share, MEM_ROOT *mem_root,
 */
 class Column_definition: public Sql_alloc
 {
+  /**
+    Create "interval" from "interval_list".
+    @param mem_root                   - memory root to create the TYPELIB
+                                        instance and its values on
+    @param reuse_interval_list_values - determines if TYPELIB can reuse strings
+                                        from interval_list, or should always
+                                        allocate a copy on mem_root, even if
+                                        character set conversion is not needed
+    @retval false on success
+    @retval true  on error (bad values, or EOM)
+  */
+  bool create_interval_from_interval_list(MEM_ROOT *mem_root,
+                                          bool reuse_interval_list_values);
+
+  /*
+    Calculate TYPELIB (set or enum) max and total lengths
+
+    @param  cs            charset+collation pair of the interval
+    @param  max_length    length of the longest item
+    @param  tot_length    sum of the item lengths
+
+    After this method call:
+    - ENUM uses max_length
+    - SET uses tot_length.
+  */
+  void calculate_interval_lengths(uint32 *max_length, uint32 *tot_length)
+  {
+    const char **pos;
+    uint *len;
+    *max_length= *tot_length= 0;
+    for (pos= interval->type_names, len= interval->type_lengths;
+         *pos ; pos++, len++)
+    {
+      size_t length= charset->cset->numchars(charset, *pos, *pos + *len);
+      *tot_length+= length;
+      set_if_bigger(*max_length, (uint32)length);
+    }
+  }
 public:
   const char *field_name;
   LEX_STRING comment;			// Comment for field
@@ -3775,7 +3813,65 @@ class Column_definition: public Sql_alloc
 
   Column_definition(THD *thd, Field *field, Field *orig_field);
   void create_length_to_internal_length(void);
+  /**
+    Prepare a SET/ENUM field.
+    Create "interval" from "interval_list" if needed, and adjust "length".
+    @param mem_root                   - Memory root to allocate TYPELIB and
+                                        its values on
+    @param reuse_interval_list_values - determines if TYPELIB can reuse value
+                                        buffers from interval_list, or should
+                                        always allocate a copy on mem_root,
+                                        even if character set conversion
+                                        is not needed
+  */
+  bool prepare_interval_field(MEM_ROOT *mem_root,
+                              bool reuse_interval_list_values)
+  {
+    DBUG_ENTER("Column_definition::prepare_interval_field");
+    DBUG_ASSERT(sql_type == MYSQL_TYPE_ENUM || sql_type == MYSQL_TYPE_SET);
+    /*
+      Interval values are either in "interval" or in "interval_list",
+      but not in both at the same time, and are not empty at the same time.
+      - Values are in "interval_list" when we're coming from the parser
+        in CREATE TABLE or in CREATE {FUNCTION|PROCEDURE}.
+      - Values are in "interval" when we're in ALTER TABLE.
+
+      In a corner case with an empty set like SET(''):
+      - after the parser we have interval_list.elements==1
+      - in ALTER TABLE we have a non-NULL interval with interval->count==1,
+        with interval->type_names[0]=="" and interval->type_lengths[0]==0.
+      So the assert is still valid for this corner case.
+
+      ENUM and SET with no values at all (e.g. ENUM(), SET()) are not possible,
+      as the parser requires at least one element, so for a ENUM or SET field it
+      should never happen that both internal_list.elements and interval are 0.
+    */
+    DBUG_ASSERT((interval == NULL) == (interval_list.elements > 0));
+
+    /*
+      Create typelib from interval_list, and if necessary
+      convert strings from client character set to the
+      column character set.
+    */
+    if (interval_list.elements &&
+        create_interval_from_interval_list(mem_root,
+                                           reuse_interval_list_values))
+      DBUG_RETURN(true);
 
+    uint32 field_length, dummy;
+    if (sql_type == MYSQL_TYPE_SET)
+    {
+      calculate_interval_lengths(&dummy, &field_length);
+      length= field_length + (interval->count - 1);
+    }
+    else /* MYSQL_TYPE_ENUM */
+    {
+      calculate_interval_lengths(&field_length, &dummy);
+      length= field_length;
+    }
+    set_if_smaller(length, MAX_FIELD_WIDTH - 1);
+    DBUG_RETURN(false);
+  }
   bool check(THD *thd);
 
   bool stored_in_db() const { return !vcol_info || vcol_info->stored_in_db; }

sql/sp_head.cc

@@ -747,58 +747,6 @@ sp_head::set_stmt_end(THD *thd)
 }
 
 
-static TYPELIB *
-create_typelib(MEM_ROOT *mem_root, Column_definition *field_def, List<String> *src)
-{
-  TYPELIB *result= NULL;
-  CHARSET_INFO *cs= field_def->charset;
-  DBUG_ENTER("create_typelib");
-
-  if (src->elements)
-  {
-    result= (TYPELIB*) alloc_root(mem_root, sizeof(TYPELIB));
-    result->count= src->elements;
-    result->name= "";
-    if (!(result->type_names=(const char **)
-          alloc_root(mem_root,(sizeof(char *)+sizeof(int))*(result->count+1))))
-      DBUG_RETURN(0);
-    result->type_lengths= (uint*)(result->type_names + result->count+1);
-    List_iterator<String> it(*src);
-    String conv;
-    for (uint i=0; i < result->count; i++)
-    {
-      uint32 dummy;
-      uint length;
-      String *tmp= it++;
-
-      if (String::needs_conversion(tmp->length(), tmp->charset(),
-                                   cs, &dummy))
-      {
-        uint cnv_errs;
-        conv.copy(tmp->ptr(), tmp->length(), tmp->charset(), cs, &cnv_errs);
-
-        length= conv.length();
-        result->type_names[i]= (char*) strmake_root(mem_root, conv.ptr(),
-                                                    length);
-      }
-      else
-      {
-        length= tmp->length();
-        result->type_names[i]= strmake_root(mem_root, tmp->ptr(), length);
-      }
-
-      // Strip trailing spaces.
-      length= cs->cset->lengthsp(cs, result->type_names[i], length);
-      result->type_lengths[i]= length;
-      ((uchar *)result->type_names[i])[length]= '\0';
-    }
-    result->type_names[result->count]= 0;
-    result->type_lengths[result->count]= 0;
-  }
-  DBUG_RETURN(result);
-}
-
-
 sp_head::~sp_head()
 {
   LEX *lex;
@@ -2362,11 +2310,8 @@ sp_head::fill_field_definition(THD *thd, LEX *lex,
   if (field_def->check(thd))
     return TRUE;
 
-  if (field_def->interval_list.elements)
-    field_def->interval= create_typelib(mem_root, field_def,
-                                        &field_def->interval_list);
-
-  sp_prepare_create_field(thd, field_def);
+  if (sp_prepare_create_field(thd, mem_root, field_def))
+    return true;
 
   if (prepare_create_field(field_def, &unused1, HA_CAN_GEOMETRY))
   {

sql/sql_table.cc

@@ -2821,40 +2821,6 @@ bool check_duplicates_in_interval(const char *set_or_name,
 }
 
 
-/*
-  Check TYPELIB (set or enum) max and total lengths
-
-  SYNOPSIS
-    calculate_interval_lengths()
-    cs            charset+collation pair of the interval
-    typelib       list of values for the column
-    max_length    length of the longest item
-    tot_length    sum of the item lengths
-
-  DESCRIPTION
-    After this function call:
-    - ENUM uses max_length
-    - SET uses tot_length.
-
-  RETURN VALUES
-    void
-*/
-void calculate_interval_lengths(CHARSET_INFO *cs, TYPELIB *interval,
-                                uint32 *max_length, uint32 *tot_length)
-{
-  const char **pos;
-  uint *len;
-  *max_length= *tot_length= 0;
-  for (pos= interval->type_names, len= interval->type_lengths;
-       *pos ; pos++, len++)
-  {
-    size_t length= cs->cset->numchars(cs, *pos, *pos + *len);
-    *tot_length+= length;
-    set_if_bigger(*max_length, (uint32)length);
-  }
-}
-
-
 /*
   Prepare a create_table instance for packing
 
@@ -3251,79 +3217,16 @@ mysql_prepare_create_table(THD *thd, HA_CREATE_INFO *create_info,
     if (sql_field->sql_type == MYSQL_TYPE_SET ||
         sql_field->sql_type == MYSQL_TYPE_ENUM)
     {
-      uint32 dummy;
-      CHARSET_INFO *cs= sql_field->charset;
-      TYPELIB *interval= sql_field->interval;
-
       /*
-        Create typelib from interval_list, and if necessary
-        convert strings from client character set to the
-        column character set.
+        Create the typelib in runtime memory - we will free the
+        occupied memory at the same time when we free this
+        sql_field -- at the end of execution.
+        Pass "true" as the last argument to reuse "interval_list"
+        values in "interval" in cases when no character conversion is needed,
+        to avoid extra copying.
       */
-      if (!interval)
-      {
-        /*
-          Create the typelib in runtime memory - we will free the
-          occupied memory at the same time when we free this
-          sql_field -- at the end of execution.
-        */
-        interval= sql_field->interval= typelib(thd->mem_root,
-                                               sql_field->interval_list);
-        List_iterator<String> int_it(sql_field->interval_list);
-        String conv, *tmp;
-        char comma_buf[5]; /* 5 bytes for 'filename' charset */
-        DBUG_ASSERT(sizeof(comma_buf) >= cs->mbmaxlen);
-        int comma_length= cs->cset->wc_mb(cs, ',', (uchar*) comma_buf,
-                                          (uchar*) comma_buf +
-                                          sizeof(comma_buf));
-        DBUG_ASSERT(comma_length > 0);
-        for (uint i= 0; (tmp= int_it++); i++)
-        {
-          size_t lengthsp;
-          if (String::needs_conversion(tmp->length(), tmp->charset(),
-                                       cs, &dummy))
-          {
-            uint cnv_errs;
-            conv.copy(tmp->ptr(), tmp->length(), tmp->charset(), cs, &cnv_errs);
-            interval->type_names[i]= strmake_root(thd->mem_root, conv.ptr(),
-                                                  conv.length());
-            interval->type_lengths[i]= conv.length();
-          }
-
-          // Strip trailing spaces.
-          lengthsp= cs->cset->lengthsp(cs, interval->type_names[i],
-                                       interval->type_lengths[i]);
-          interval->type_lengths[i]= lengthsp;
-          ((uchar *)interval->type_names[i])[lengthsp]= '\0';
-          if (sql_field->sql_type == MYSQL_TYPE_SET)
-          {
-            if (cs->coll->instr(cs, interval->type_names[i], 
-                                interval->type_lengths[i], 
-                                comma_buf, comma_length, NULL, 0))
-            {
-              ErrConvString err(tmp->ptr(), tmp->length(), cs);
-              my_error(ER_ILLEGAL_VALUE_FOR_TYPE, MYF(0), "set", err.ptr());
-              DBUG_RETURN(TRUE);
-            }
-          }
-        }
-        sql_field->interval_list.empty(); // Don't need interval_list anymore
-      }
-
-      if (sql_field->sql_type == MYSQL_TYPE_SET)
-      {
-        uint32 field_length;
-        calculate_interval_lengths(cs, interval, &dummy, &field_length);
-        sql_field->length= field_length + (interval->count - 1);
-      }
-      else  /* MYSQL_TYPE_ENUM */
-      {
-        uint32 field_length;
-        DBUG_ASSERT(sql_field->sql_type == MYSQL_TYPE_ENUM);
-        calculate_interval_lengths(cs, interval, &field_length, &dummy);
-        sql_field->length= field_length;
-      }
-      set_if_smaller(sql_field->length, MAX_FIELD_WIDTH-1);
+      if (sql_field->prepare_interval_field(thd->mem_root, true))
+        DBUG_RETURN(true); // E.g. wrong values with commas: SET('a,b')
     }
 
     if (sql_field->sql_type == MYSQL_TYPE_BIT)
@@ -4349,28 +4252,18 @@ static bool prepare_blob_field(THD *thd, Column_definition *sql_field)
 
 */
 
-void sp_prepare_create_field(THD *thd, Column_definition *sql_field)
+bool sp_prepare_create_field(THD *thd, MEM_ROOT *mem_root,
+                             Column_definition *sql_field)
 {
   if (sql_field->sql_type == MYSQL_TYPE_SET ||
       sql_field->sql_type == MYSQL_TYPE_ENUM)
   {
-    uint32 field_length, dummy;
-    if (sql_field->sql_type == MYSQL_TYPE_SET)
-    {
-      calculate_interval_lengths(sql_field->charset,
-                                 sql_field->interval, &dummy, 
-                                 &field_length);
-      sql_field->length= field_length + 
-                         (sql_field->interval->count - 1);
-    }
-    else /* MYSQL_TYPE_ENUM */
-    {
-      calculate_interval_lengths(sql_field->charset,
-                                 sql_field->interval,
-                                 &field_length, &dummy);
-      sql_field->length= field_length;
-    }
-    set_if_smaller(sql_field->length, MAX_FIELD_WIDTH-1);
+    /*
+      Pass "false" as the last argument to allocate TYPELIB values on mem_root,
+      even if no character set conversion is needed.
+    */
+    if (sql_field->prepare_interval_field(mem_root, false))
+      return true; // E.g. wrong values with commas: SET('a,b')
   }
 
   if (sql_field->sql_type == MYSQL_TYPE_BIT)
@@ -4380,8 +4273,12 @@ void sp_prepare_create_field(THD *thd, Column_definition *sql_field)
   }
   sql_field->create_length_to_internal_length();
   DBUG_ASSERT(sql_field->default_value == 0);
-  /* Can't go wrong as sql_field->def is not defined */
-  (void) prepare_blob_field(thd, sql_field);
+  /*
+    prepare_blob_field() can return an error on attempt to create a too long
+    VARCHAR/VARBINARY field when the current sql_mode does not allow automatic
+    conversion to TEXT/BLOB.
+  */
+  return prepare_blob_field(thd, sql_field);
 }
 
 

sql/sql_table.h

@@ -251,7 +251,8 @@ bool quick_rm_table(THD *thd, handlerton *base, const char *db,
                     const char *table_name, uint flags,
                     const char *table_path=0);
 void close_cached_table(THD *thd, TABLE *table);
-void sp_prepare_create_field(THD *thd, Column_definition *sql_field);
+bool sp_prepare_create_field(THD *thd, MEM_ROOT *mem_root,
+                             Column_definition *sql_field);
 int prepare_create_field(Column_definition *sql_field,
 			 uint *blob_columns,
 			 longlong table_flags);

sql/table.cc

@@ -3534,30 +3534,6 @@ fix_type_pointers(const char ***array, TYPELIB *point_to_type, uint types,
 } /* fix_type_pointers */
 
 
-TYPELIB *typelib(MEM_ROOT *mem_root, List<String> &strings)
-{
-  TYPELIB *result= (TYPELIB*) alloc_root(mem_root, sizeof(TYPELIB));
-  if (!result)
-    return 0;
-  result->count=strings.elements;
-  result->name="";
-  uint nbytes= (sizeof(char*) + sizeof(uint)) * (result->count + 1);
-  if (!(result->type_names= (const char**) alloc_root(mem_root, nbytes)))
-    return 0;
-  result->type_lengths= (uint*) (result->type_names + result->count + 1);
-  List_iterator<String> it(strings);
-  String *tmp;
-  for (uint i=0; (tmp=it++) ; i++)
-  {
-    result->type_names[i]= tmp->ptr();
-    result->type_lengths[i]= tmp->length();
-  }
-  result->type_names[result->count]= 0;		// End marker
-  result->type_lengths[result->count]= 0;
-  return result;
-}
-
-
 /*
  Search after a field with given start & length
  If an exact field isn't found, return longest field with starts

sql/table.h

@@ -2736,7 +2736,6 @@ inline bool is_infoschema_db(const char *name)
                         INFORMATION_SCHEMA_NAME.str, name);
 }
 
-TYPELIB *typelib(MEM_ROOT *mem_root, List<String> &strings);
 
 inline void mark_as_null_row(TABLE *table)
 {