Hi Varun, The patch is ok to push. One last question: why is it made against 10.2? I see the MDEV issue has AffectsVersion to start from 10.2, but to my knowledge the code and constants involved were not changed in 10.2, so the issue should be present in the earlier versions as well? On Mon, Nov 12, 2018 at 04:27:08PM +0530, Varun wrote:

revision-id: a9c1420233f3f65aee00a8e7bdd1c8481b0bbc37 (mariadb-10.2.18-74-ga9c1420233f) parent(s): b290ef8c76e2d7dfbae7a85766694a6fd4648eac author: Varun Gupta committer: Varun Gupta timestamp: 2018-11-12 16:25:25 +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.

--- mysql-test/r/func_group_innodb.result | 23 +++++++++++++++++++++++ mysql-test/t/func_group_innodb.test | 18 ++++++++++++++++++ sql/handler.h | 12 +++++++++--- sql/partition_info.cc | 4 ++-- sql/sql_const.h | 13 ++++++++++++- sql/table.cc | 2 +- 6 files changed, 65 insertions(+), 7 deletions(-)

diff --git a/mysql-test/r/func_group_innodb.result b/mysql-test/r/func_group_innodb.result index 52d5922df95..c2bfe9bf81c 100644 --- a/mysql-test/r/func_group_innodb.result +++ b/mysql-test/r/func_group_innodb.result @@ -246,4 +246,27 @@ EXPLAIN SELECT MIN(c) FROM t1 GROUP BY b; id select_type table type possible_keys key key_len ref rows Extra 1 SIMPLE t1 range NULL b 263 NULL 3 Using index for group-by DROP TABLE t1; +# +# MDEV-17589: Stack-buffer-overflow with indexed varchar (utf8) field +# +CREATE TABLE t1 (v1 varchar(1020), v2 varchar(2), v3 varchar(2), KEY k1 (v3,v2,v1)) ENGINE=InnoDB CHARACTER SET=utf8; +INSERT INTO t1 VALUES ('king', 'qu','qu'), ('bad','go','go'); +explain +SELECT MIN(t1.v1) FROM t1 where t1.v2='qu' and t1.v3='qu'; +id select_type table type possible_keys key key_len ref rows Extra +1 SIMPLE NULL NULL NULL NULL NULL NULL NULL Select tables optimized away +SELECT MIN(t1.v1) FROM t1 where t1.v2='qu' and t1.v3='qu'; +MIN(t1.v1) +king +drop table t1; +CREATE TABLE t1 (v1 varchar(1024) CHARACTER SET utf8, KEY v1 (v1)) ENGINE=InnoDB; +INSERT INTO t1 VALUES ('king'), ('bad'); +explain +SELECT MIN(x.v1) FROM (SELECT t1.* FROM t1 WHERE t1.v1 >= 'p') x; +id select_type table type possible_keys key key_len ref rows Extra +1 SIMPLE NULL NULL NULL NULL NULL NULL NULL No matching min/max row +SELECT MIN(x.v1) FROM (SELECT t1.* FROM t1 WHERE t1.v1 >= 'p') x; +MIN(x.v1) +NULL +drop table t1; End of 5.5 tests diff --git a/mysql-test/t/func_group_innodb.test b/mysql-test/t/func_group_innodb.test index c62d3d08496..0d24f37363b 100644 --- a/mysql-test/t/func_group_innodb.test +++ b/mysql-test/t/func_group_innodb.test @@ -192,4 +192,22 @@ EXPLAIN SELECT MIN(c) FROM t1 GROUP BY b;

DROP TABLE t1;

+--echo # +--echo # MDEV-17589: Stack-buffer-overflow with indexed varchar (utf8) field +--echo # + +CREATE TABLE t1 (v1 varchar(1020), v2 varchar(2), v3 varchar(2), KEY k1 (v3,v2,v1)) ENGINE=InnoDB CHARACTER SET=utf8; +INSERT INTO t1 VALUES ('king', 'qu','qu'), ('bad','go','go'); +explain +SELECT MIN(t1.v1) FROM t1 where t1.v2='qu' and t1.v3='qu'; +SELECT MIN(t1.v1) FROM t1 where t1.v2='qu' and t1.v3='qu'; +drop table t1; + +CREATE TABLE t1 (v1 varchar(1024) CHARACTER SET utf8, KEY v1 (v1)) ENGINE=InnoDB; +INSERT INTO t1 VALUES ('king'), ('bad'); +explain +SELECT MIN(x.v1) FROM (SELECT t1.* FROM t1 WHERE t1.v1 >= 'p') x; +SELECT MIN(x.v1) FROM (SELECT t1.* FROM t1 WHERE t1.v1 >= 'p') x; +drop table t1; + --echo End of 5.5 tests diff --git a/sql/handler.h b/sql/handler.h index ed2ef822c88..6745312a17d 100644 --- a/sql/handler.h +++ b/sql/handler.h @@ -378,6 +378,12 @@ enum enum_alter_inplace_result { #define HA_KEY_NULL_LENGTH 1 #define HA_KEY_BLOB_LENGTH 2

+/* Maximum length of any index lookup key, in bytes */ + +#define MAX_KEY_LENGTH (MAX_DATA_LENGTH_FOR_KEY \ + +(MAX_REF_PARTS \ + *(HA_KEY_NULL_LENGTH + HA_KEY_BLOB_LENGTH))) + #define HA_LEX_CREATE_TMP_TABLE 1U #define HA_CREATE_TMP_ALTER 8U

@@ -3421,14 +3427,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/partition_info.cc b/sql/partition_info.cc index f96882552fa..8be3f878f8d 100644 --- a/sql/partition_info.cc +++ b/sql/partition_info.cc @@ -1713,12 +1713,12 @@ bool partition_info::check_partition_field_length()

for (i= 0; i < num_part_fields; i++) store_length+= get_partition_field_store_length(part_field_array[i]); - if (store_length > MAX_KEY_LENGTH) + if (store_length > MAX_DATA_LENGTH_FOR_KEY) DBUG_RETURN(TRUE); store_length= 0; for (i= 0; i < num_subpart_fields; i++) store_length+= get_partition_field_store_length(subpart_field_array[i]); - if (store_length > MAX_KEY_LENGTH) + if (store_length > MAX_DATA_LENGTH_FOR_KEY) DBUG_RETURN(TRUE); DBUG_RETURN(FALSE); } diff --git a/sql/sql_const.h b/sql/sql_const.h index 7395ae3c08a..0c781855302 100644 --- a/sql/sql_const.h +++ b/sql/sql_const.h @@ -33,7 +33,18 @@ #define MAX_SYS_VAR_LENGTH 32 #define MAX_KEY MAX_INDEXES /* Max used keys */ #define MAX_REF_PARTS 32 /* Max parts used as ref */ -#define MAX_KEY_LENGTH 3072 /* max possible key */ + +/* + 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_DATA_LENGTH_FOR_KEY 3072 #if SIZEOF_OFF_T > 4 #define MAX_REFLENGTH 8 /* Max length for record ref */ #else diff --git a/sql/table.cc b/sql/table.cc index 2b278288784..5b163d501d1 100644 --- a/sql/table.cc +++ b/sql/table.cc @@ -2169,7 +2169,7 @@ int TABLE_SHARE::init_from_binary_frm_image(THD *thd, bool write, uint pk_part_length= key_first_info->key_part[i].store_length; if (keyinfo->ext_key_part_map & 1<<i) { - if (ext_key_length + pk_part_length > MAX_KEY_LENGTH) + if (ext_key_length + pk_part_length > MAX_DATA_LENGTH_FOR_KEY) { add_keyparts_for_this_key= i; break; _______________________________________________ commits mailing list commits@mariadb.org https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits

-- BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog