From e5dfe04da02244e592db0b5955a4d95148e0928a Mon Sep 17 00:00:00 2001 From: Alexander Barkov Date: Wed, 2 Nov 2016 18:04:35 +0400 Subject: [PATCH] MDEV-11146 SP variables of the SET data type erroneously allow values with comma There was a duplicate code to create TYPELIB from List: - 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 | 12 +++ mysql-test/r/sp-bugs.result | 11 +++ mysql-test/t/ctype_utf8.test | 17 ++++ mysql-test/t/sp-bugs.test | 14 ++++ sql/field.cc | 83 +++++++++++++++++++ sql/field.h | 96 +++++++++++++++++++++ sql/sp_head.cc | 59 +------------ sql/sql_table.cc | 147 +++++---------------------------- sql/sql_table.h | 3 +- sql/table.cc | 24 ------ sql/table.h | 1 - 11 files changed, 259 insertions(+), 208 deletions(-) diff --git a/mysql-test/r/ctype_utf8.result b/mysql-test/r/ctype_utf8.result index c3c94d4865f01..ce6b34fa731eb 100644 --- a/mysql-test/r/ctype_utf8.result +++ b/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 +# diff --git a/mysql-test/r/sp-bugs.result b/mysql-test/r/sp-bugs.result index ccccacd09a5db..3db1e68321c3a 100644 --- a/mysql-test/r/sp-bugs.result +++ b/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 diff --git a/mysql-test/t/ctype_utf8.test b/mysql-test/t/ctype_utf8.test index 352359a03b657..39a124bf7c52a 100644 --- a/mysql-test/t/ctype_utf8.test +++ b/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 # diff --git a/mysql-test/t/sp-bugs.test b/mysql-test/t/sp-bugs.test index 4671aee11e1e8..3239dfeaeec3d 100644 --- a/mysql-test/t/sp-bugs.test +++ b/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 ;$$ diff --git a/sql/field.cc b/sql/field.cc index 673bf22a4a591..6fa6838e063e0 100644 --- a/sql/field.cc +++ b/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 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; /* diff --git a/sql/field.h b/sql/field.h index fd62218f1444f..b55cc9a0c5851 100644 --- a/sql/field.h +++ b/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; } diff --git a/sql/sp_head.cc b/sql/sp_head.cc index 0b9b503aef3d7..c5d64618ea4d4 100644 --- a/sql/sp_head.cc +++ b/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 *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 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)) { diff --git a/sql/sql_table.cc b/sql/sql_table.cc index f8a61b641c0ba..fa99605bd6dc5 100644 --- a/sql/sql_table.cc +++ b/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 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); } diff --git a/sql/sql_table.h b/sql/sql_table.h index 628c51f678f28..0d1a2ccf45927 100644 --- a/sql/sql_table.h +++ b/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); diff --git a/sql/table.cc b/sql/table.cc index 71c70e2df2ed0..0d1286695cd52 100644 --- a/sql/table.cc +++ b/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 &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 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 diff --git a/sql/table.h b/sql/table.h index c2c523181a005..e68f4ec85639a 100644 --- a/sql/table.h +++ b/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 &strings); inline void mark_as_null_row(TABLE *table) {