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