Hi Varun, On Mon, Nov 05, 2018 at 07:36:17PM +0530, Varun wrote: > revision-id: 0187bb03884a34ca8de96e47f4cb1f3d860e6db9 > (mariadb-10.2.16-224-g0187bb03884) > parent(s): 8a346f31b913daa011085afec2b2d38450c73e00 > author: Varun Gupta > committer: Varun Gupta > timestamp: 2018-11-05 19:32:26 +0530 > message: > > MDEV_-17589: Stack-buffer-overflow with indexed varchar (utf8) field > > Create a new constant MAX_DATA_LENGTH_FOR_KEY. > Replace the value of MAX_KEY_LENGTH to also include the LENGTH and NULL BYTES. > ...
> diff --git a/mysql-test/r/partition_datatype.result > b/mysql-test/r/partition_datatype.result > index 2e518c194f0..66d003c97cb 100644 > --- a/mysql-test/r/partition_datatype.result > +++ b/mysql-test/r/partition_datatype.result > @@ -314,7 +314,7 @@ bbbb > drop table t1; > set sql_mode=''; > create table t1 (a varchar(3070)) partition by key (a); > -ERROR HY000: The total length of the partitioning fields is too large > +drop table t1; I don't think it's a good idea that after this patch we allow the table definitions that we didn't allow before. This raises the question of datadir downgrades, etc. I think we should only remove the buffer overrun. Here's patch how to avoid the above: https://gist.github.com/spetrunia/7e7e3fcc60a81a6cb854e77c12c367d3 > create table t1 (a varchar(65532) not null) partition by key (a); > ERROR HY000: The total length of the partitioning fields is too large > create table t1 (a varchar(65533)) partition by key (a); ... > diff --git a/mysql-test/r/innodb_ext_key.result > b/mysql-test/r/innodb_ext_key.result > index c55e8d138f8..c76c8613c57 100644 > --- a/mysql-test/r/innodb_ext_key.result > +++ b/mysql-test/r/innodb_ext_key.result > @@ -1168,8 +1168,8 @@ EXPLAIN > "access_type": "range", > "possible_keys": ["f2"], > "key": "f2", > - "key_length": "3070", > - "used_key_parts": ["f2", "pk1"], > + "key_length": "3074", > + "used_key_parts": ["f2", "pk1", "pk2"], Same reaction as above: This change shows that "Extended keys" feature now allows bigger set of key extensions than before. Can we avoid making this change by changing MAX_KEY_LENGTH to MAX_DATA_LENGTH_FOR_KEY somewhere? > "rows": 1, > "filtered": 100, > "index_condition": "t1.pk1 <= 5 and t1.pk2 <= 5 and t1.f2 = 'abc'", > diff --git a/sql/handler.h b/sql/handler.h > index ed2ef822c88..75f72bdfb53 100644 > --- a/sql/handler.h > +++ b/sql/handler.h > @@ -378,6 +378,10 @@ enum enum_alter_inplace_result { > #define HA_KEY_NULL_LENGTH 1 > #define HA_KEY_BLOB_LENGTH 2 > > +#define MAX_KEY_LENGTH (MAX_DATA_LENGTH_FOR_KEY \ > + +(MAX_REF_PARTS \ > + *(HA_KEY_NULL_LENGTH + HA_KEY_BLOB_LENGTH))) > + I think this requires a comment. Please add something like /* Maximum length of any index lookup key, in bytes */ > #define HA_LEX_CREATE_TMP_TABLE 1U > #define HA_CREATE_TMP_ALTER 8U > > @@ -3421,14 +3425,14 @@ class handler :public Sql_alloc > uint max_key_parts() const > { return MY_MIN(MAX_REF_PARTS, max_supported_key_parts()); } > uint max_key_length() const > - { return MY_MIN(MAX_KEY_LENGTH, max_supported_key_length()); } > + { return MY_MIN(MAX_DATA_LENGTH_FOR_KEY, max_supported_key_length()); } > uint max_key_part_length() const > - { return MY_MIN(MAX_KEY_LENGTH, max_supported_key_part_length()); } > + { return MY_MIN(MAX_DATA_LENGTH_FOR_KEY, max_supported_key_part_length()); > } > > virtual uint max_supported_record_length() const { return > HA_MAX_REC_LENGTH; } > virtual uint max_supported_keys() const { return 0; } > virtual uint max_supported_key_parts() const { return MAX_REF_PARTS; } > - virtual uint max_supported_key_length() const { return MAX_KEY_LENGTH; } > + virtual uint max_supported_key_length() const { return > MAX_DATA_LENGTH_FOR_KEY; } > virtual uint max_supported_key_part_length() const { return 255; } > virtual uint min_record_length(uint options) const { return 1; } > > diff --git a/sql/sql_const.h b/sql/sql_const.h > index 7395ae3c08a..0be63a2a5ad 100644 > --- a/sql/sql_const.h > +++ b/sql/sql_const.h > @@ -33,7 +33,7 @@ > #define MAX_SYS_VAR_LENGTH 32 > #define MAX_KEY MAX_INDEXES /* Max used keys */ > #define MAX_REF_PARTS 32 /* Max parts used as ref */ Please add a comment, something like /* Maximum length of the data part of an index lookup key. The "data part" is defined as the value itself, not including the NULL-indicator bytes or varchar length bytes ("the Extras"). We need this value because there was a bug where length of the Extras were not counted. You probably need MAX_KEY_LENGTH, not this constant. */ > -#define MAX_KEY_LENGTH 3072 /* max possible key */ > +#define MAX_DATA_LENGTH_FOR_KEY 3072 > #if SIZEOF_OFF_T > 4 > #define MAX_REFLENGTH 8 /* Max length for > record ref */ > #else BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog _______________________________________________ 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
